[dpdk-dev] [PATCH v2] net/vhost: add pmd xstats

Yuanhan Liu yuanhan.liu at linux.intel.com
Sun Sep 18 15:16:48 CEST 2016


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).

> > > +	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.

> > > +		}
> > > +	}
> > > +	/* 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.

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.
     */

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.

Feel free to phone me if you have any doubts.

	--yliu


More information about the dev mailing list