From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 379FEA034C; Wed, 21 Dec 2022 11:18:52 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 9B15342D3B; Wed, 21 Dec 2022 11:18:05 +0100 (CET) Received: from mail-vs1-f46.google.com (mail-vs1-f46.google.com [209.85.217.46]) by mails.dpdk.org (Postfix) with ESMTP id 69C024021D for ; Mon, 19 Dec 2022 20:39:08 +0100 (CET) Received: by mail-vs1-f46.google.com with SMTP id a66so9766959vsa.6 for ; Mon, 19 Dec 2022 11:39:08 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=AMBizSBwr4Y+0kg7Scz32R+yKb76ffUBJaMaGOvmPiI=; b=cCjxXiYB3yPLvMkgTJHPQWXIy4Omq+GA8JbBQd691Xvp8WOZ5kE6jKIrown3C+qlPe j7Q0PbsRFKmv4nidtGW0F6o2Ra8xCjW/zDFkB0442Ysmope1IhIl583tVpzeYHsm0OIJ y0AnVYdJU4pfIZ2ZtChDH9vy5fod63Yb/lc9+X5OpHdKxdzLObRfjCGIkRaYuN92rv3V +MTQKhz4KDIg/5pY0qZOJx9G2IjFsfk3VlmZ/k0oZja1VHgjGwLYXkC2QYNUs2e3y3Gm NBC29xnolKznr0v7mFxYl9zyTgAW2u1YRZGJ4se0VTYX+iGR8rEr4+ARMZtiLRkCej4Z BnXQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=AMBizSBwr4Y+0kg7Scz32R+yKb76ffUBJaMaGOvmPiI=; b=bSXb+GSvPKkGidmz+oJ3DuTSfp5A7CYbYl9z6Fm0o2KugKOuaCbM52c1BU4G664BYs Qn35RBf4WZiIhDihib5K18zYYXqvAAL9DTNY0fnaxx/WP8CGjE5sctn9rTpvehtg9PKS vFeRECy/VyHX8QcXX2zvzctplyQU0zVgxWU+P4dtg8yaOUlz4X5g8LZCNm8fdNSojyVr 1qDPIomVt1WUakAcIWugx2SBGIzJObI7IuTgR3HKsVZHdctRAADCneag+yCrkyhNoTvp igHddaTDeEMqw8kCcifRde2Q8nlgJLxLqFnkrw2Dew4CUlDd0PAemzzotF+VAoMKyRAB Ta7w== X-Gm-Message-State: AFqh2kodAnL5bq1jO5a+weRRS5e7tUvcmMuVwDLjulvqxFXIzqkPmvJC eaSn62OvFrHKkEkNQ3aJ6/mbKEaxlPjnNPMY+hPk5Q== X-Google-Smtp-Source: AMrXdXtOxK/KrKOPxb/z04SLhPwSh1rUdcgLE4U9sr4Fnc7441yPu9ss3e+pqQ7PYFWvC2ih39EEhv4D4N2NZdTBG7Q= X-Received: by 2002:a05:6102:20da:b0:3bc:7:6257 with SMTP id i26-20020a05610220da00b003bc00076257mr1070657vsr.10.1671478747659; Mon, 19 Dec 2022 11:39:07 -0800 (PST) MIME-Version: 1.0 References: <20221124073335.3985214-1-junfeng.guo@intel.com> <20221207083948.22af7cae@hermes.local> In-Reply-To: From: Joshua Washington Date: Mon, 19 Dec 2022 11:38:56 -0800 Message-ID: Subject: Re: [PATCH] net/gve: add support for basic stats To: Rushil Gupta Cc: Stephen Hemminger , Ferruh Yigit , Junfeng Guo , qi.z.zhang@intel.com, jingjing.wu@intel.com, beilei.xing@intel.com, dev@dpdk.org, jeroendb@google.com, jrkim@google.com, Xiaoyun Li Content-Type: multipart/alternative; boundary="000000000000015a4e05f0337965" X-Mailman-Approved-At: Wed, 21 Dec 2022 11:17:53 +0100 X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org --000000000000015a4e05f0337965 Content-Type: text/plain; charset="UTF-8" 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. On Mon, Dec 19, 2022 at 11:17 AM Rushil Gupta 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 > wrote: > > > > On Wed, 7 Dec 2022 15:09:08 +0000 > > Ferruh Yigit 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/ \ > > > > source/doc/guides/rel_notes/deprecation.rst#L118 > > > > > > > > Signed-off-by: Xiaoyun Li > > > > Signed-off-by: Junfeng Guo > > > > > > <...> > > > > > > > +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 | (414) 366-4423 --000000000000015a4e05f0337965 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hello,

As it turns out, this error actu= ally propagates to the "total" stats as well,=C2=A0which I assume= is just calculated by adding TX-packets and TX-dropped. Here are the full = stats from the example that Rushil mentioned:

=C2=A0 ---------------------- Forward statistics for p= ort 0 =C2=A0----------------------
=C2=A0 RX-packets: 2453802 =C2=A0 =C2= =A0 =C2=A0 =C2=A0RX-dropped: 0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 RX= -total: 2453802
=C2=A0 TX-packets: 34266881 =C2=A0 =C2=A0 =C2=A0 TX-drop= ped: 447034 =C2=A0 =C2=A0 =C2=A0 =C2=A0TX-total: 34713915
=C2=A0 -------= ---------------------------------------------------------------------
=C2=A0 ---------------------- Forward statistics for port 1 =C2=A0-------= ---------------
=C2=A0 RX-packets: 34713915 =C2=A0 =C2=A0 =C2=A0 RX-drop= ped: 0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 RX-total: 34713915
=C2= =A0 TX-packets: 2453802 =C2=A0 =C2=A0 =C2=A0 =C2=A0TX-dropped: 0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 TX-total: 2453802
=C2=A0 ---------------= -------------------------------------------------------------

=C2=A0= +++++++++++++++ Accumulated forward statistics for all ports++++++++++++++= +
=C2=A0 RX-packets: 37167717 =C2=A0 =C2=A0 =C2=A0 RX-dropped: 0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 RX-total: 37167717
=C2=A0 TX-packets:= 36720683 =C2=A0 =C2=A0 =C2=A0 TX-dropped: 807630 =C2=A0 =C2=A0 =C2=A0 =C2= =A0TX-total: 37528313
=C2=A0 +++++++++++++++++++++++++++++++++++++++++++= +++++++++++++++++++++++++++++++++

It can be seen = that the stats for the individual ports are consistent, but the TX-total an= d TX-dropped are not consistent with the stats for the individual ports,=C2= =A0as I believe that the TX-total and RX-total accumulated stats should be = equal.

On Mon, D= ec 19, 2022 at 11:17 AM Rushil Gupta <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-tota= l).


On Wed, Dec 7, 2022 at 8:39 AM Stephen Hemminger
<stephen= @networkplumber.org> wrote:
>
> On Wed, 7 Dec 2022 15:09:08 +0000
> Ferruh Yigit <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 bas= ic stats
> > > items may still be required. So we just keep the remaining o= nes and
> > > will implement the queue stats via xstats in the coming rele= ase.
> > >
> > > [1]
> > > https://elixir.bootlin.com/dpdk/v22.07/ \<= br> > > >=C2=A0 =C2=A0 =C2=A0source/doc/guides/rel_notes/deprecation.r= st#L118
> > >
> > > Signed-off-by: Xiaoyun Li <xiaoyun.li@intel.com>
> > > Signed-off-by: Junfeng Guo <junfeng.guo@intel.com>
> >
> > <...>
> >
> > > +static int
> > > +gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_s= tats *stats)
> > > +{
> > > +=C2=A0 =C2=A0uint16_t i;
> > > +
> > > +=C2=A0 =C2=A0for (i =3D 0; i < dev->data->nb_tx_qu= eues; i++) {
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct gve_tx_queu= e *txq =3D dev->data->tx_queues[i];
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (txq =3D=3D NUL= L)
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0continue;
> > > +
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0stats->opackets= +=3D txq->packets;
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0stats->obytes += =3D txq->bytes;
> > > +=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0stats->oerrors = +=3D 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 (witho= ut stats reset
> > in between), same values will be keep added to stats.
> > Some hw based implementations does this, because reading from sta= ts
> > registers automatically reset those registers but this shouldn= 9;t be case
> > for this driver.
> >
> > I expect it to be something like:
> >
> > local_stats =3D 0
> > foreach queue
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0local_stats +=3D queue->stats
> > stats =3D 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)
> {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0struct rte_eth_dev *dev;
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0RTE_ETH_VALID_PORTID_OR_ERR_RET(port_= id, -ENODEV);
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0dev =3D &rte_eth_devices[port_id]= ;
>
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0memset(stats, 0, sizeof(*stats));
> ...
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0stats->rx_nombuf =3D dev->data-= >rx_mbuf_alloc_failed;
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return 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.<= br>


--

Joshua Washington=C2=A0|= =C2=A0Software Engineer= |=C2=A0joshwash@google.com=C2=A0|=C2=A0(414) 366-4423<= /div>= =C2=A0
--000000000000015a4e05f0337965--