From: Yuanhan Liu <yuanhan.liu@linux.intel.com>
To: "Yang, Zhiyong" <zhiyong.yang@intel.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>,
"thomas.monjalon@6wind.com" <thomas.monjalon@6wind.com>,
"pmatilai@redhat.com" <pmatilai@redhat.com>
Subject: Re: [dpdk-dev] [PATCH v2] net/vhost: add pmd xstats
Date: Sun, 18 Sep 2016 21:16:48 +0800 [thread overview]
Message-ID: <20160918131648.GG23158@yliu-dev.sh.intel.com> (raw)
In-Reply-To: <E182254E98A5DA4EB1E657AC7CB9BD2A3EB01AD8@BGSMSX101.gar.corp.intel.com>
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 <zhiyong.yang@intel.com>
> > 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
next prev parent reply other threads:[~2016-09-18 13:16 UTC|newest]
Thread overview: 57+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-08-19 12:16 [dpdk-dev] [PATCH] vhost: " Zhiyong Yang
2016-08-22 7:52 ` Panu Matilainen
2016-08-23 8:04 ` Yang, Zhiyong
2016-08-23 9:45 ` Panu Matilainen
2016-08-24 5:46 ` Yuanhan Liu
2016-08-24 8:44 ` Thomas Monjalon
2016-08-24 12:37 ` Panu Matilainen
2016-08-25 9:21 ` Yang, Zhiyong
2016-08-30 2:45 ` Yao, Lei A
2016-08-30 3:03 ` Xu, Qian Q
2016-08-30 3:21 ` Yao, Lei A
2016-08-31 7:18 ` Yang, Zhiyong
2016-09-01 6:37 ` Yang, Zhiyong
2016-09-09 8:15 ` [dpdk-dev] [PATCH v2] net/vhost: " Zhiyong Yang
2016-09-09 8:40 ` Van Haaren, Harry
2016-09-09 8:54 ` Yang, Zhiyong
2016-09-14 6:20 ` Yuanhan Liu
2016-09-14 7:43 ` Yang, Zhiyong
2016-09-18 13:16 ` Yuanhan Liu [this message]
2016-09-19 2:48 ` Yang, Zhiyong
2016-09-14 8:30 ` Yang, Zhiyong
2016-09-20 9:36 ` [dpdk-dev] [PATCH v3 0/2] net/vhost: add pmd xstats support Zhiyong Yang
2016-09-20 9:36 ` [dpdk-dev] [PATCH v3 1/2] net/vhost: add a new stats struct Zhiyong Yang
2016-09-20 10:44 ` Yuanhan Liu
2016-09-21 5:12 ` Yang, Zhiyong
2016-09-21 10:05 ` [dpdk-dev] [PATCH v4 0/2] net/vhost: add pmd xstats support Zhiyong Yang
2016-09-21 10:05 ` Zhiyong Yang
2016-09-21 10:05 ` [dpdk-dev] [PATCH v4 2/2] net/vhost: add pmd xstats Zhiyong Yang
2016-09-21 10:57 ` Yuanhan Liu
2016-09-22 1:42 ` Yang, Zhiyong
2016-09-22 2:09 ` Yuanhan Liu
2016-09-21 10:13 ` [dpdk-dev] [PATCH v4 1/2] net/vhost: add a new defined stats struct Zhiyong Yang
2016-09-22 8:19 ` [dpdk-dev] [PATCH v5 0/2] net/vhost: add pmd xstats support Zhiyong Yang
2016-09-22 8:19 ` [dpdk-dev] [PATCH v5 1/2] net/vhost: add a new defined stats struct Zhiyong Yang
2016-09-28 8:33 ` [dpdk-dev] [PATCH v6 0/2] net/vhost: add pmd xstats support Zhiyong Yang
2016-09-28 8:33 ` [dpdk-dev] [PATCH v6 1/2] net/vhost: add a new defined stats struct Zhiyong Yang
2016-09-28 13:26 ` [dpdk-dev] [PATCH v7 0/2] net/vhost: add pmd xstats support Zhiyong Yang
2016-09-28 13:26 ` [dpdk-dev] [PATCH v7 1/2] net/vhost: add a new defined stats struct Zhiyong Yang
2016-09-29 1:55 ` Yuanhan Liu
2016-09-29 12:35 ` [dpdk-dev] [PATCH v8 0/2] net/vhost: add pmd xstats support Zhiyong Yang
2016-09-29 12:35 ` [dpdk-dev] [PATCH v8 1/2] net/vhost: add a new defined stats struct Zhiyong Yang
2016-09-29 12:35 ` [dpdk-dev] [PATCH v8 2/2] net/vhost: add pmd xstats Zhiyong Yang
2016-09-29 13:04 ` Yuanhan Liu
2016-09-29 13:50 ` Yang, Zhiyong
2016-09-28 13:26 ` [dpdk-dev] [PATCH v7 " Zhiyong Yang
2016-09-29 8:48 ` Loftus, Ciara
2016-09-29 12:02 ` Yuanhan Liu
2016-09-29 12:22 ` Yang, Zhiyong
2016-09-28 8:33 ` [dpdk-dev] [PATCH v6 " Zhiyong Yang
2016-09-22 8:19 ` [dpdk-dev] [PATCH v5 " Zhiyong Yang
2016-09-23 3:56 ` [dpdk-dev] [PATCH v5 0/2] net/vhost: add pmd xstats support Yuanhan Liu
2016-09-28 2:35 ` Yuanhan Liu
2016-09-20 9:36 ` [dpdk-dev] [PATCH v3 2/2] net/vhost: add pmd xstats Zhiyong Yang
2016-09-20 10:56 ` Yuanhan Liu
2016-09-21 7:22 ` Yang, Zhiyong
2016-09-20 11:50 ` Yuanhan Liu
2016-09-21 6:15 ` Yang, Zhiyong
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160918131648.GG23158@yliu-dev.sh.intel.com \
--to=yuanhan.liu@linux.intel.com \
--cc=dev@dpdk.org \
--cc=pmatilai@redhat.com \
--cc=thomas.monjalon@6wind.com \
--cc=zhiyong.yang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).