From: Joshua Washington <joshwash@google.com>
To: Ferruh Yigit <ferruh.yigit@amd.com>
Cc: Rushil Gupta <rushilg@google.com>,
Stephen Hemminger <stephen@networkplumber.org>,
Junfeng Guo <junfeng.guo@intel.com>,
qi.z.zhang@intel.com, jingjing.wu@intel.com,
beilei.xing@intel.com, dev@dpdk.org, jeroendb@google.com,
jrkim@google.com, Xiaoyun Li <xiaoyun.li@intel.com>
Subject: Re: [PATCH] net/gve: add support for basic stats
Date: Thu, 2 Feb 2023 15:00:21 -0800 [thread overview]
Message-ID: <CALuQH+V60GJFY0UkS3ZyM+L_HD_ZvFMoRhS=sXaV1m8LtAsjzQ@mail.gmail.com> (raw)
In-Reply-To: <a440ab60-9624-f21e-396a-239bdf2aa1a1@amd.com>
[-- Attachment #1: Type: text/plain, Size: 12359 bytes --]
Tested-by: Joshua Washington <joshwash@google.com>
On Tue, Jan 31, 2023 at 2:05 AM Ferruh Yigit <ferruh.yigit@amd.com> wrote:
> On 1/31/2023 1:51 AM, Joshua Washington wrote:
> > Hello,
> >
> > I tested it out, and the updates to testpmd seem to work.
> >
>
> Hi Joshua,
>
> Thanks for testing, I will send a patch soon.
>
> But this was testpmd issue, do you have any objection with the net/gve
> patch, if not can you please record this with ack/review/tested-by tags,
> so I can proceed with it.
>
> > Before applying the second patch:
> > ---------------------- Forward statistics for port 0
> > ----------------------
> > RX-packets: 0 RX-dropped: 0 RX-total: 0
> > TX-packets: 43769238 TX-dropped: 62634 TX-total: 43831872
> >
> >
> ----------------------------------------------------------------------------
> >
> > ---------------------- Forward statistics for port 1
> > ----------------------
> > RX-packets: 0 RX-dropped: 0 RX-total: 0
> > TX-packets: 43761119 TX-dropped: 70753 TX-total: 43831872
> >
> >
> ----------------------------------------------------------------------------
> >
> > +++++++++++++++ Accumulated forward statistics for all
> > ports+++++++++++++++
> > RX-packets: 0 RX-dropped: 0 RX-total: 0
> > TX-packets: 87530357 TX-dropped: 157302 TX-total: 87687659
> >
> >
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >
> > 62634 + 70753 = 133387 != 157302
> >
> > After applying the second patch:
> > ---------------------- Forward statistics for port 0
> > ----------------------
> > RX-packets: 0 RX-dropped: 0 RX-total: 0
> > TX-packets: 12590721 TX-dropped: 36638 TX-total: 12627359
> >
> >
> ----------------------------------------------------------------------------
> >
> > ---------------------- Forward statistics for port 1
> > ----------------------
> > RX-packets: 0 RX-dropped: 0 RX-total: 0
> > TX-packets: 12596255 TX-dropped: 31746 TX-total: 12628001
> >
> >
> ----------------------------------------------------------------------------
> >
> > +++++++++++++++ Accumulated forward statistics for all
> > ports+++++++++++++++
> > RX-packets: 0 RX-dropped: 0 RX-total: 0
> > TX-packets: 25186976 TX-dropped: 68384 TX-total: 25255360
> >
> >
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >
> > Thanks,
> > Josh
> >
> > On Wed, Jan 18, 2023 at 8:22 AM Ferruh Yigit <ferruh.yigit@amd.com
> > <mailto:ferruh.yigit@amd.com>> wrote:
> >
> > On 12/19/2022 7:38 PM, Joshua Washington wrote:
> > > Hello,
> > >
> > > As it turns out, this error actually propagates to the "total"
> > stats as
> > > well, which I assume is just calculated by adding TX-packets and
> > > TX-dropped. Here are the full stats from the example that Rushil
> > mentioned:
> > >
> > > ---------------------- Forward statistics for port 0
> > > ----------------------
> > > RX-packets: 2453802 RX-dropped: 0 RX-total:
> > 2453802
> > > TX-packets: 34266881 TX-dropped: 447034 TX-total:
> > 34713915
> > >
> > >
> >
> ----------------------------------------------------------------------------
> > >
> > > ---------------------- Forward statistics for port 1
> > > ----------------------
> > > RX-packets: 34713915 RX-dropped: 0 RX-total:
> > 34713915
> > > TX-packets: 2453802 TX-dropped: 0 TX-total:
> > 2453802
> > >
> > >
> >
> ----------------------------------------------------------------------------
> > >
> > > +++++++++++++++ Accumulated forward statistics for all
> > > ports+++++++++++++++
> > > RX-packets: 37167717 RX-dropped: 0 RX-total:
> > 37167717
> > > TX-packets: 36720683 TX-dropped: 807630 TX-total:
> > 37528313
> > >
> > >
> >
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > >
> > > It can be seen that the stats for the individual ports are
> consistent,
> > > but the TX-total and TX-dropped are not consistent with the stats
> for
> > > the individual ports, as I believe that the TX-total and RX-total
> > > accumulated stats should be equal.
> > >
> >
> > Hi Joshua, Rushil,
> >
> > As I checked for it, issue may be related to testpmd stats display,
> >
> > While displaying per port TX-dropped value, it only takes
> > 'ports_stats[pt_id].tx_dropped' into account,
> > but for accumulated TX-dropped results it takes both
> > 'ports_stats[pt_id].tx_dropped' & 'stats.oerrors' into account.
> >
> > If you can reproduce it easily, can you please test with following
> > change:
> >
> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> > index 134d79a55547..49322d07d7d6 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -2056,6 +2056,8 @@ fwd_stats_display(void)
> > fwd_cycles += fs->core_cycles;
> > }
> > for (i = 0; i < cur_fwd_config.nb_fwd_ports; i++) {
> > + uint64_t tx_dropped = 0;
> > +
> > pt_id = fwd_ports_ids[i];
> > port = &ports[pt_id];
> >
> > @@ -2077,8 +2079,9 @@ fwd_stats_display(void)
> > total_recv += stats.ipackets;
> > total_xmit += stats.opackets;
> > total_rx_dropped += stats.imissed;
> > - total_tx_dropped += ports_stats[pt_id].tx_dropped;
> > - total_tx_dropped += stats.oerrors;
> > + tx_dropped += ports_stats[pt_id].tx_dropped;
> > + tx_dropped += stats.oerrors;
> > + total_tx_dropped += tx_dropped;
> > total_rx_nombuf += stats.rx_nombuf;
> >
> > printf("\n %s Forward statistics for port %-2d
> %s\n",
> > @@ -2105,8 +2108,8 @@ fwd_stats_display(void)
> >
> > printf(" TX-packets: %-14"PRIu64" TX-dropped:
> > %-14"PRIu64
> > "TX-total: %-"PRIu64"\n",
> > - stats.opackets,
> ports_stats[pt_id].tx_dropped,
> > - stats.opackets +
> ports_stats[pt_id].tx_dropped);
> > + stats.opackets, tx_dropped,
> > + stats.opackets + tx_dropped);
> >
> > if (record_burst_stats) {
> > if (ports_stats[pt_id].rx_stream)
> >
> >
> > >
> > > On Mon, Dec 19, 2022 at 11:17 AM Rushil Gupta <rushilg@google.com
> > <mailto:rushilg@google.com>
> > > <mailto:rushilg@google.com <mailto:rushilg@google.com>>> wrote:
> > >
> > > Hi all
> > > Josh just found out some inconsistencies in the Tx/Rx
> > statistics sum
> > > for all ports. Not sure if we can screenshot here but it goes
> like
> > > this:
> > > Tx-dropped for port0: 447034
> > > Tx-dropped for port1: 0
> > > Accumulated forward statistics for all ports: 807630
> > >
> > > Please note that this issue is only with Tx-dropped (not
> > > Tx-packets/Tx-total).
> > >
> > >
> > > On Wed, Dec 7, 2022 at 8:39 AM Stephen Hemminger
> > > <stephen@networkplumber.org
> > <mailto:stephen@networkplumber.org>
> > <mailto:stephen@networkplumber.org
> > <mailto:stephen@networkplumber.org>>> wrote:
> > > >
> > > > On Wed, 7 Dec 2022 15:09:08 +0000
> > > > Ferruh Yigit <ferruh.yigit@amd.com
> > <mailto:ferruh.yigit@amd.com> <mailto:ferruh.yigit@amd.com
> > <mailto:ferruh.yigit@amd.com>>>
> > > wrote:
> > > >
> > > > > On 11/24/2022 7:33 AM, Junfeng Guo wrote:
> > > > > > Add support for dev_ops of stats_get and stats_reset.
> > > > > >
> > > > > > Queue stats update will be moved into xstat [1], but the
> > basic
> > > stats
> > > > > > items may still be required. So we just keep the
> remaining
> > > ones and
> > > > > > will implement the queue stats via xstats in the coming
> > release.
> > > > > >
> > > > > > [1]
> > > > > > https://elixir.bootlin.com/dpdk/v22.07/
> > <https://elixir.bootlin.com/dpdk/v22.07/>
> > > <https://elixir.bootlin.com/dpdk/v22.07/
> > <https://elixir.bootlin.com/dpdk/v22.07/>> \
> > > > > > source/doc/guides/rel_notes/deprecation.rst#L118
> > > > > >
> > > > > > Signed-off-by: Xiaoyun Li <xiaoyun.li@intel.com
> > <mailto:xiaoyun.li@intel.com>
> > > <mailto:xiaoyun.li@intel.com <mailto:xiaoyun.li@intel.com>>>
> > > > > > Signed-off-by: Junfeng Guo <junfeng.guo@intel.com
> > <mailto:junfeng.guo@intel.com>
> > > <mailto:junfeng.guo@intel.com <mailto:junfeng.guo@intel.com>>>
> > > > >
> > > > > <...>
> > > > >
> > > > > > +static int
> > > > > > +gve_dev_stats_get(struct rte_eth_dev *dev, struct
> > > rte_eth_stats *stats)
> > > > > > +{
> > > > > > + uint16_t i;
> > > > > > +
> > > > > > + for (i = 0; i < dev->data->nb_tx_queues; i++) {
> > > > > > + struct gve_tx_queue *txq =
> > dev->data->tx_queues[i];
> > > > > > + if (txq == NULL)
> > > > > > + continue;
> > > > > > +
> > > > > > + stats->opackets += txq->packets;
> > > > > > + stats->obytes += txq->bytes;
> > > > > > + stats->oerrors += txq->errors;
> > > > >
> > > > > Hi Junfeng, Qi, Jingjing, Beilei,
> > > > >
> > > > > Above logic looks wrong to me, did you test it?
> > > > >
> > > > > If the 'gve_dev_stats_get()' called multiple times (without
> > > stats reset
> > > > > in between), same values will be keep added to stats.
> > > > > Some hw based implementations does this, because reading
> > from stats
> > > > > registers automatically reset those registers but this
> > shouldn't
> > > be case
> > > > > for this driver.
> > > > >
> > > > > I expect it to be something like:
> > > > >
> > > > > local_stats = 0
> > > > > foreach queue
> > > > > local_stats += queue->stats
> > > > > stats = local_stats
> > > >
> > > > The zero of local_stats is unnecessary.
> > > > The only caller of the PMD stats_get is rte_ethdev_stats_get
> > > > and it zeros the stats structure before calling the PMD.
> > > >
> > > >
> > > > int
> > > > rte_eth_stats_get(uint16_t port_id, struct rte_eth_stats
> *stats)
> > > > {
> > > > struct rte_eth_dev *dev;
> > > >
> > > > RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -ENODEV);
> > > > dev = &rte_eth_devices[port_id];
> > > >
> > > > memset(stats, 0, sizeof(*stats));
> > > > ...
> > > > stats->rx_nombuf = dev->data->rx_mbuf_alloc_failed;
> > > > return eth_err(port_id,
> (*dev->dev_ops->stats_get)(dev,
> > > stats));
> > > >
> > > > If any PMD has extra memset in their stats get that could be
> > removed.
> > >
> > >
> > >
> > > --
> > >
> > > Joshua Washington | Software Engineer | joshwash@google.com
> > <mailto:joshwash@google.com>
> > > <mailto:joshwash@google.com <mailto:joshwash@google.com>> | (414)
> > 366-4423 <tel:(414)%20366-4423>
> > >
> >
> >
> >
> > --
> >
> > Joshua Washington | Software Engineer | joshwash@google.com
> > <mailto:joshwash@google.com> | (414) 366-4423
> >
>
>
--
Joshua Washington | Software Engineer | joshwash@google.com | (414) 366-4423
[-- Attachment #2: Type: text/html, Size: 20007 bytes --]
next prev parent reply other threads:[~2023-02-02 23:00 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-11-24 7:33 Junfeng Guo
2022-11-24 16:59 ` Stephen Hemminger
2022-11-24 17:26 ` Ferruh Yigit
2022-11-26 3:16 ` Rushil Gupta
2022-11-26 17:34 ` Stephen Hemminger
2022-11-28 11:12 ` Ferruh Yigit
2022-11-28 17:24 ` Stephen Hemminger
2022-11-29 1:42 ` Guo, Junfeng
2022-12-07 15:09 ` Ferruh Yigit
2022-12-07 16:39 ` Stephen Hemminger
2022-12-19 19:17 ` Rushil Gupta
2022-12-19 19:38 ` Joshua Washington
2023-01-18 16:22 ` Ferruh Yigit
2023-01-31 1:51 ` Joshua Washington
2023-01-31 10:05 ` Ferruh Yigit
2023-02-02 23:00 ` Joshua Washington [this message]
2023-02-03 17:39 ` Ferruh Yigit
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='CALuQH+V60GJFY0UkS3ZyM+L_HD_ZvFMoRhS=sXaV1m8LtAsjzQ@mail.gmail.com' \
--to=joshwash@google.com \
--cc=beilei.xing@intel.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@amd.com \
--cc=jeroendb@google.com \
--cc=jingjing.wu@intel.com \
--cc=jrkim@google.com \
--cc=junfeng.guo@intel.com \
--cc=qi.z.zhang@intel.com \
--cc=rushilg@google.com \
--cc=stephen@networkplumber.org \
--cc=xiaoyun.li@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).