From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by dpdk.org (Postfix) with ESMTP id 483802BB5 for ; Sun, 18 Sep 2016 15:16:19 +0200 (CEST) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by orsmga101.jf.intel.com with ESMTP; 18 Sep 2016 06:16:18 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.30,356,1470726000"; d="scan'208";a="762901462" Received: from yliu-dev.sh.intel.com (HELO yliu-dev) ([10.239.67.162]) by FMSMGA003.fm.intel.com with ESMTP; 18 Sep 2016 06:16:17 -0700 Date: Sun, 18 Sep 2016 21:16:48 +0800 From: Yuanhan Liu To: "Yang, Zhiyong" Cc: "dev@dpdk.org" , "thomas.monjalon@6wind.com" , "pmatilai@redhat.com" Message-ID: <20160918131648.GG23158@yliu-dev.sh.intel.com> References: <1471608966-39077-1-git-send-email-zhiyong.yang@intel.com> <1473408927-40364-1-git-send-email-zhiyong.yang@intel.com> <20160914062021.GZ23158@yliu-dev.sh.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Subject: Re: [dpdk-dev] [PATCH v2] net/vhost: add pmd xstats X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 18 Sep 2016 13:16:20 -0000 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@linux.intel.com] > > Sent: Wednesday, September 14, 2016 2:20 PM > > To: Yang, Zhiyong > > Cc: dev@dpdk.org; thomas.monjalon@6wind.com; pmatilai@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