Hi, Yuanhan: Thanks a lot for your comments and suggestion in so detail. I will send v3 patch as soon as possible.
--Zhiyong > -----Original Message----- > From: Yuanhan Liu [mailto:yuanhan.liu at linux.intel.com] > Sent: Sunday, September 18, 2016 9:17 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 Wed, Sep 14, 2016 at 07:43:36AM +0000, Yang, Zhiyong wrote: > > 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. > > For sure you can modify the code :) I just would suggest to do in an single > patch, as stated before (and below). > Ok. I will do that. > > > > + 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; > > Yes, but rx_queues[i] is with type "void *", so the cast is not necessary. > I'm confused if void * ptr can access a member of struct directly. Or Need I explicitly cast it when using it? > > > > + } > > > > + } > > > > + /* 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 > > It still took me some time to understand you. > > > RFC2863 page 35 ifOutUcastPkts(non-multi/broadcast) > > So, by your term, unicast == non-multicast/broadcast? If so, I'd suggest to > use term "unicast" but not something else: it just introduces confusions. > You are right . non-multicast/broadcast is equal to unicast. I agree with you And will modify it as you say. > And in this case, I guess you were trying to say: > > /* > * According to RFC 2863 section ifHCOutMulticastPkts, ... and ..., > * the counter "unicast", "multicast" and "broadcast" are also > * increased when packets are not transmitted successfully. > */ > Yes. > Well, you might still need reword it. > > After taking a bit closer look at the code, I'd suggest to do the countings > like > following: > > - introduce a help function to increase the "broadcast" or "multicast" > counter, say "count_multicast_broadcast(struct vhost_stats *stats, mbuf)". > > - introduce a generic function to update the generic counters: this > function just counts those packets have been successfully transmitted > or received. > > It also invoke above helper function for multicast counting. > > It basically looks like the function vhost_update_packet_xstats, > execpt that it doesn't handle those failure packets: it will be > handled in following part. > > - since the case "couting multicast and broadcast with failure packts" > only happens in the Tx side, we could do those countings only in > eth_vhost_tx(): > > nb_tx = rte_vhost_enqueue_burst(r->vid, > r->virtqueue_id, bufs, nb_bufs); > > missed = nb_bufs - nb_tx; > /* put above comment here */ > if (missed) { > for_each_mbuf(mbuf) { > count_multicast_broadcast(&vq->stats, mbuf); > } > } > > - there is no need to update "stat[10]" (unicast), since it can be calculated > from other counters, meaning we could just get the right value on query. > > This could save some cycles. > Ok, Your comments are detailed and clear. I will consider to remove unicast and modify update functions. > Feel free to phone me if you have any doubts. > > --yliu
