Hi, yuanhan: Thanks so much for your detailed comments. > -----Original Message----- > From: Yuanhan Liu [mailto:yuanhan.liu at linux.intel.com] > Sent: Wednesday, September 14, 2016 2:20 PM > To: Yang, Zhiyong <zhiyong.yang at intel.com> > Cc: dev at dpdk.org; thomas.monjalon at 6wind.com; pmatilai at redhat.com > Subject: Re: [PATCH v2] net/vhost: add pmd xstats > > On Fri, Sep 09, 2016 at 04:15:27PM +0800, Zhiyong Yang wrote: > > +struct vhost_xstats { > > + uint64_t stat[16]; > > +}; > > + > > struct vhost_queue { > > int vid; > > rte_atomic32_t allow_queuing; > > @@ -85,7 +89,8 @@ struct vhost_queue { > > uint64_t missed_pkts; > > uint64_t rx_bytes; > > uint64_t tx_bytes; > > I'd suggest to put those statistic counters to vhost_stats struct, which could > simplify the xstats_reset code a bit. >
I consider this point when I define it, but those statistic counters are used by pmd stats, not only by pmd xstats, I'm not clear if I can modify those code. If permitted, I will do it as you said. > And please do it in two patches, one to introduce vhost_stats, another one > to add xstats. > Ok. > > -}; > > + struct vhost_xstats xstats; > > + }; > > A format issue here. > Naming issue? such as struct vhost_xstats vhost_stats;? > > > > struct pmd_internal { > > char *dev_name; > > @@ -127,6 +132,274 @@ struct rte_vhost_vring_state { > > > > static struct rte_vhost_vring_state *vring_states[RTE_MAX_ETHPORTS]; > > > > +enum rte_vhostqueue_rxtx { > > Don't use "rte_" prefix for internal function, vars, etc. "rte_" prefix is > reserved for those that will be exported for public use. > Ok, I will modify it. > > + RTE_VHOSTQUEUE_RX = 0, > > + RTE_VHOSTQUEUE_TX = 1 > > +}; > > + > > +#define RTE_ETH_VHOST_XSTATS_NAME_SIZE 64 > > ditto. > Ok, I will modify it. > > + > > +struct rte_vhost_xstats_name_off { > > ditto. > Ok, I will modify it. > > + char name[RTE_ETH_VHOST_XSTATS_NAME_SIZE]; > > + uint64_t offset; > > +}; > > + > > +/* [rt]_qX_ is prepended to the name string here */ static void > > +vhost_dev_xstats_reset(struct rte_eth_dev *dev) { > > + struct vhost_queue *vqrx = NULL; > > + struct vhost_queue *vqtx = NULL; > > + unsigned int i = 0; > > + > > + for (i = 0; i < dev->data->nb_rx_queues; i++) { > > + if (!dev->data->rx_queues[i]) > > + continue; > > + vqrx = (struct vhost_queue *)dev->data->rx_queues[i]; > > Unnecessary cast. > The definition of rx_queues is void **rx_queues; when we use it to access its Data member, Maybe explicit cast is needed, otherwise, we have to cast Every time when using it. > > + vqrx->rx_pkts = 0; > > + vqrx->rx_bytes = 0; > > + vqrx->missed_pkts = 0; > > + memset(&vqrx->xstats, 0, sizeof(vqrx->xstats)); > > + } > > + for (i = 0; i < dev->data->nb_tx_queues; i++) { > > + if (!dev->data->tx_queues[i]) > > + continue; > > + vqtx = (struct vhost_queue *)dev->data->tx_queues[i]; > > + vqtx->tx_pkts = 0; > > + vqtx->tx_bytes = 0; > > + vqtx->missed_pkts = 0; > > + memset(&vqtx->xstats, 0, sizeof(vqtx->xstats)); > > + } > > +} > > + > > +static int > > +vhost_dev_xstats_get_names(struct rte_eth_dev *dev, > > + struct rte_eth_xstat_name *xstats_names, > > + __rte_unused unsigned int limit) > > The typical way is to put __rte_unused after the key word. > Ok. > > +{ > > + unsigned int i = 0; > > + unsigned int t = 0; > > + int count = 0; > > + int nstats = dev->data->nb_rx_queues * VHOST_NB_RXQ_XSTATS > > + + dev->data->nb_tx_queues * > VHOST_NB_TXQ_XSTATS; > > + > > + if (xstats_names) { > > I know you are following virtio pmd style, but you don't have to. I'd suggest > to return early for (!xstats_names) case, then we could save one indention > level for following code block. > OK. > > + for (i = 0; i < dev->data->nb_rx_queues; i++) { > > + struct vhost_queue *rxvq = dev->data- > >rx_queues[i]; > > + > > + if (!rxvq) > > + continue; > > + for (t = 0; t < VHOST_NB_RXQ_XSTATS; t++) { > > + snprintf(xstats_names[count].name, > > + sizeof(xstats_names[count].name), > > + "rx_q%u_%s", i, > > + rte_vhost_rxq_stat_strings[t].name); > > + count++; > > + } > > + } > > + for (i = 0; i < dev->data->nb_tx_queues; i++) { > > + struct vhost_queue *txvq = dev->data- > >tx_queues[i]; > > + > > + if (!txvq) > > + continue; > > + for (t = 0; t < VHOST_NB_TXQ_XSTATS; t++) { > > + snprintf(xstats_names[count].name, > > + sizeof(xstats_names[count].name), > > + "tx_q%u_%s", i, > > + rte_vhost_txq_stat_strings[t].name); > > + count++; > > + } > > + } > > + return count; > > + } > > + return nstats; > > +} > > + > > +static int > > +vhost_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat > *xstats, > > + unsigned int n) > > +{ > > + unsigned int i; > > + unsigned int t; > > + unsigned int count = 0; > > + > > + unsigned int nxstats = dev->data->nb_rx_queues * > VHOST_NB_RXQ_XSTATS > > + + dev->data->nb_tx_queues * > VHOST_NB_TXQ_XSTATS; > > + > > + if (n < nxstats) > > + return nxstats; > > + > > + for (i = 0; i < dev->data->nb_rx_queues; i++) { > > + struct vhost_queue *rxvq = > > + (struct vhost_queue *)dev->data->rx_queues[i]; > > Unnecessary cast. > Ok. I will remove it. > > + > > + if (!rxvq) > > + continue; > > + > > + for (t = 0; t < VHOST_NB_RXQ_XSTATS; t++) { > > + xstats[count].value = *(uint64_t *)(((char *)rxvq) > > + + rte_vhost_rxq_stat_strings[t].offset); > > + count++; > > + } > > + } > > + > > + for (i = 0; i < dev->data->nb_tx_queues; i++) { > > + struct vhost_queue *txvq = > > + (struct vhost_queue *)dev->data->tx_queues[i]; > > + > > + if (!txvq) > > + continue; > > + > > + for (t = 0; t < VHOST_NB_TXQ_XSTATS; t++) { > > + xstats[count].value = *(uint64_t *)(((char *)txvq) > > + + rte_vhost_txq_stat_strings[t].offset); > > + count++; > > + } > > + } > > + > > + return count; > > +} > > + > > +static void > > +vhost_update_packet_xstats(struct vhost_queue *vq, > > + struct rte_mbuf **bufs, > > + uint16_t nb_rxtx, > > + uint16_t nb_bufs, > > + enum rte_vhostqueue_rxtx vqueue_rxtx) { > > + uint32_t pkt_len = 0; > > + uint64_t i = 0; > > + uint64_t index; > > + struct ether_addr *ea = NULL; > > + struct vhost_xstats *xstats_update = &vq->xstats; > > + > > + for (i = 0; i < nb_rxtx ; i++) { > > + pkt_len = bufs[i]->pkt_len; > > + if (pkt_len == 64) { > > + xstats_update->stat[1]++; > > + > Unnecessary blank line. > Ok. I will remove it. > > + } else if (pkt_len > 64 && pkt_len < 1024) { > > + index = (sizeof(pkt_len) * 8) > > + - __builtin_clz(pkt_len) - 5; > > + xstats_update->stat[index]++; > > + } else { > > + if (pkt_len < 64) > > + xstats_update->stat[0]++; > > + else if (pkt_len <= 1522) > > + xstats_update->stat[6]++; > > + else if (pkt_len > 1522) > > + xstats_update->stat[7]++; > > + } > > + > > + ea = rte_pktmbuf_mtod(bufs[i], struct ether_addr *); > > + if (is_multicast_ether_addr(ea)) { > > + if (is_broadcast_ether_addr(ea)) > > + /* broadcast++; */ > > + xstats_update->stat[8]++; > > + else > > + /* multicast++; */ > > + xstats_update->stat[9]++; > > The comment could be avoided if you define a field in vhost_stats struct like > "broadcast" or "multicast". I don't object the way Harry proposed tough, to > use enum to access the stat array. > I agree with you and Harry and will do like that. > > + } > > + } > > + /* non-multi/broadcast, multi/broadcast, including those > > + * that were discarded or not sent. > > Hmmm, I don't follow it. You may want to reword it. > > > from rfc2863 > > Which section and which page? > The packets received are not considered "discarded", because receiving packets via Memory, not by physical NIC. Mainly for the number of transmit the packets RFC2863 page 35 ifOutUcastPkts(non-multi/broadcast) Page 42 ifHCOutMulticastPkts Page 42 ifHCOutBroadcastPkts > --yliu