<div dir="ltr">Tested-by: Joshua Washington <<a href="mailto:joshwash@google.com">joshwash@google.com</a>></div> <div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Jan 31, 2023 at 2:05 AM Ferruh Yigit <<a href="mailto:ferruh.yigit@amd.com">ferruh.yigit@amd.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">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 <<a href="mailto:ferruh.yigit@amd.com" target="_blank">ferruh.yigit@amd.com</a> > <mailto:<a href="mailto:ferruh.yigit@amd.com" target="_blank">ferruh.yigit@amd.com</a>>> 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 <<a href="mailto:rushilg@google.com" target="_blank">rushilg@google.com</a> > <mailto:<a href="mailto:rushilg@google.com" target="_blank">rushilg@google.com</a>> > > <mailto:<a href="mailto:rushilg@google.com" target="_blank">rushilg@google.com</a> <mailto:<a href="mailto:rushilg@google.com" target="_blank">rushilg@google.com</a>>>> 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 > > <<a href="mailto:stephen@networkplumber.org" target="_blank">stephen@networkplumber.org</a> > <mailto:<a href="mailto:stephen@networkplumber.org" target="_blank">stephen@networkplumber.org</a>> > <mailto:<a href="mailto:stephen@networkplumber.org" target="_blank">stephen@networkplumber.org</a> > <mailto:<a href="mailto:stephen@networkplumber.org" target="_blank">stephen@networkplumber.org</a>>>> wrote: > > > > > > On Wed, 7 Dec 2022 15:09:08 +0000 > > > Ferruh Yigit <<a href="mailto:ferruh.yigit@amd.com" target="_blank">ferruh.yigit@amd.com</a> > <mailto:<a href="mailto:ferruh.yigit@amd.com" target="_blank">ferruh.yigit@amd.com</a>> <mailto:<a href="mailto:ferruh.yigit@amd.com" target="_blank">ferruh.yigit@amd.com</a> > <mailto:<a href="mailto:ferruh.yigit@amd.com" target="_blank">ferruh.yigit@amd.com</a>>>> > > 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] > > > > > <a href="https://elixir.bootlin.com/dpdk/v22.07/" rel="noreferrer" target="_blank">https://elixir.bootlin.com/dpdk/v22.07/</a> > <<a href="https://elixir.bootlin.com/dpdk/v22.07/" rel="noreferrer" target="_blank">https://elixir.bootlin.com/dpdk/v22.07/</a>> > > <<a href="https://elixir.bootlin.com/dpdk/v22.07/" rel="noreferrer" target="_blank">https://elixir.bootlin.com/dpdk/v22.07/</a> > <<a href="https://elixir.bootlin.com/dpdk/v22.07/" rel="noreferrer" target="_blank">https://elixir.bootlin.com/dpdk/v22.07/</a>>> \ > > > > > source/doc/guides/rel_notes/deprecation.rst#L118 > > > > > > > > > > Signed-off-by: Xiaoyun Li <<a href="mailto:xiaoyun.li@intel.com" target="_blank">xiaoyun.li@intel.com</a> > <mailto:<a href="mailto:xiaoyun.li@intel.com" target="_blank">xiaoyun.li@intel.com</a>> > > <mailto:<a href="mailto:xiaoyun.li@intel.com" target="_blank">xiaoyun.li@intel.com</a> <mailto:<a href="mailto:xiaoyun.li@intel.com" target="_blank">xiaoyun.li@intel.com</a>>>> > > > > > Signed-off-by: Junfeng Guo <<a href="mailto:junfeng.guo@intel.com" target="_blank">junfeng.guo@intel.com</a> > <mailto:<a href="mailto:junfeng.guo@intel.com" target="_blank">junfeng.guo@intel.com</a>> > > <mailto:<a href="mailto:junfeng.guo@intel.com" target="_blank">junfeng.guo@intel.com</a> <mailto:<a href="mailto:junfeng.guo@intel.com" target="_blank">junfeng.guo@intel.com</a>>>> > > > > > > > > <...> > > > > > > > > > +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 | <a href="mailto:joshwash@google.com" target="_blank">joshwash@google.com</a> > <mailto:<a href="mailto:joshwash@google.com" target="_blank">joshwash@google.com</a>> > > <mailto:<a href="mailto:joshwash@google.com" target="_blank">joshwash@google.com</a> <mailto:<a href="mailto:joshwash@google.com" target="_blank">joshwash@google.com</a>>> | (414) > 366-4423 <tel:(414)%20366-4423> > > > > > > -- > > Joshua Washington | Software Engineer | <a href="mailto:joshwash@google.com" target="_blank">joshwash@google.com</a> > <mailto:<a href="mailto:joshwash@google.com" target="_blank">joshwash@google.com</a>> | <a href="tel:(414)%20366-4423" value="+14143664423" target="_blank">(414) 366-4423</a> > </blockquote></div> <div> </div>-- <div dir="ltr" class="gmail_signature"><div dir="ltr"> <div style="line-height:1.5em;padding-top:10px;margin-top:10px;color:rgb(85,85,85);font-family:sans-serif">Joshua Washington | Software Engineer | <a href="mailto:joshwash@google.com" target="_blank">joshwash@google.com</a> | (414) 366-4423</div> </div></div>