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 DDF9C41CE3; Sun, 19 Feb 2023 21:38:18 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C0E8E42D39; Sun, 19 Feb 2023 21:38:18 +0100 (CET) Received: from mail-ed1-f48.google.com (mail-ed1-f48.google.com [209.85.208.48]) by mails.dpdk.org (Postfix) with ESMTP id 91EFA40698 for ; Sun, 19 Feb 2023 21:38:16 +0100 (CET) Received: by mail-ed1-f48.google.com with SMTP id en26so4053916edb.13 for ; Sun, 19 Feb 2023 12:38:16 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=references:to:cc:in-reply-to:date:subject:mime-version:message-id :from:from:to:cc:subject:date:message-id:reply-to; bh=u37rLbHBzXSNRIYPX94xrR9rUoaXsgGhgy9d4rRp1WM=; b=GIwvV1x0z2xolEB/N47Bz8X00E+B42+28YiueI/CoeqrFgZ3wlr9AVXWeUU2riJ2mk MNHswSL5jrrHrWAyYYxQ/c90uyOW4EahGU74JaRA4DK1Hh1qxiKCvQuiuj2/qylUV7y4 K2DhFeeLjaEfuLYwhp77GbaDS/wvwGEmbT5ZdbujIw1ngh4cRImIbFAl6dmbPntOYd+6 kHHm0Nd9xfKkGum9+7TvBI6zjy+ti6Kady1VO2Td48a7yrNJ+XuK4LCRq0JL7Dh2iXkf h5E3lTR5mwMkm8K+oBI0kan8vACyO+vIJ6xLA/ROPNR06/dvmfnJfhPOdLarp4JWijLu UhWA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=references:to:cc:in-reply-to:date:subject:mime-version:message-id :from:x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=u37rLbHBzXSNRIYPX94xrR9rUoaXsgGhgy9d4rRp1WM=; b=SBrCSNtoQ5LMuyqqlzOguBl3hAJAVgz9ia0t5hO5YFT+KseQeZA4HtHCX5A1tsLHvm 21/zxjNG8XM93YQPBTmZK+/Of3vC2irZwDNdmk1owyQbCzKEalB7R/L5I+aFnIJNhlNR jYfKOdhlZ32iXIaJ/aPjhLGjxb7Gl3Ham3IUI8XyYJwtGnuAdLALeMdTiwNfetytK6Rj N000S67VSbeeol8VnDxWVj4nLBq7aJsAtacmKAPfwY9tL2l/HWj2P9I21LxHMw5/8xo/ ar6wDGn5ZP/GmkVBAEqVFyGKS9goG/gmy+A62rcrB+a5fzWV25G31RtKM4/zSadALXI5 ZjDA== X-Gm-Message-State: AO0yUKXtizo+czVunWGm4kLf/vtslE1llpE1WlosxYEzM/Ryn8n4KNgG xmGiseKG9iG+P8EWDAH0z+o= X-Google-Smtp-Source: AK7set+lPUGUHPrDnn+qVIDrxyHBx7pUk2CerlEnsXwj3c6TYiE2oRis1bLcqWfY5+IDeGfFPNDZsw== X-Received: by 2002:a17:907:6e0d:b0:880:50de:5e86 with SMTP id sd13-20020a1709076e0d00b0088050de5e86mr15874915ejc.3.1676839096046; Sun, 19 Feb 2023 12:38:16 -0800 (PST) Received: from smtpclient.apple ([176.41.28.141]) by smtp.gmail.com with ESMTPSA id i15-20020a17090685cf00b008b1435bd1cbsm4834303ejy.105.2023.02.19.12.37.42 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Sun, 19 Feb 2023 12:38:15 -0800 (PST) From: Levend Sayar Message-Id: <724D6E3F-25FA-4E65-A963-41EAED39CE77@gmail.com> Content-Type: multipart/alternative; boundary="Apple-Mail=_A94BE9DE-D586-4151-8A8A-BA18CA4410AA" Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.300.101.1.3\)) Subject: Re: [PATCH 2/2] net/gve: add extended statistics Date: Sun, 19 Feb 2023 23:37:12 +0300 In-Reply-To: <0123d577-39f5-7213-07b3-04ef3918edda@amd.com> Cc: "Guo, Junfeng" , dev@dpdk.org To: Ferruh Yigit References: <20230216185814.27830-1-levendsayar@gmail.com> <20230216185814.27830-2-levendsayar@gmail.com> <6e18557e-0d7d-c8f8-13f5-4523a1226431@amd.com> <2DEB3F1C-CEDE-49CF-B22D-B1F2D9A86203@gmail.com> <0123d577-39f5-7213-07b3-04ef3918edda@amd.com> X-Mailer: Apple Mail (2.3731.300.101.1.3) 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 --Apple-Mail=_A94BE9DE-D586-4151-8A8A-BA18CA4410AA Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 Hi Ferruh, Opps, I was not aware of that rejection. Thanks for notification. =20 Let me supersede this patch. And create a new one.=20 Best, Levend > On 19 Feb 2023, at 23:09, Ferruh Yigit wrote: >=20 > On 2/19/2023 12:26 AM, Levend Sayar wrote: >> Ferruh, >>=20 >> Thanks for this detailed review. >> I am setting superseded this patch and create a new one. >> You=E2=80=99re right at all points. >> For rx.no_mbufs counting, I probably overlooked while rebasing my = patch >> and it is mixed with newly came patch. >>=20 >> When I check ethdev layer again, I noticed that when dev_flags >> has RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS >> Rx/tx queue stats are already added. I am pushing a fresh patch for >> adding rx/tx queue stats. >>=20 >=20 > Hi Levend, >=20 > You are right BUT, 'RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS' is a temporary > solution and plan is to remove it [1]. >=20 > Background is, queue stats in "struct rte_eth_stats" has fixed size = and > as number of queues supported by devices increase these fields getting > bigger and bigger, the solution we came was to completely remove these > fields from stats struct and get queue statistics via xstats. >=20 > During transition 'RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS' is introduced > until all drivers implement queue stats in xstats. We are not pushing > hard for existing drivers to update but at least requiring new drivers > to implement xstats method. >=20 > That is why for net/gve updating queue stats in 'gve_dev_stats_get()' > rejected before, and xstats implementation is requested. >=20 >=20 > [1] > = https://elixir.bootlin.com/dpdk/v22.11.1/source/doc/guides/rel_notes/depre= cation.rst#L88 >=20 >=20 >> And also noticed a missing part at rx no_mbufs counting. >>=20 >> Best, >> Levend >> =20 >>=20 >>> On 17 Feb 2023, at 15:34, Ferruh Yigit wrote: >>>=20 >>> On 2/16/2023 6:58 PM, Levend Sayar wrote: >>>> Google Virtual NIC PMD is enriched with extended statistics info. >>>=20 >>> Only queue stats added to xstats, can you please highlight this in = the >>> commit log? >>>=20 >>>> eth_dev_ops callback names are also synched with eth_dev_ops field = names >>>>=20 >>>=20 >>> Renaming eth_dev_ops is not related to xstats, and I am not sure if = the >>> change is necessary, can you please drop it from this patch? >>>=20 >>>> Signed-off-by: Levend Sayar >>>> --- >>>> drivers/net/gve/gve_ethdev.c | 152 = ++++++++++++++++++++++++++++++----- >>>> drivers/net/gve/gve_rx.c | 8 +- >>>> 2 files changed, 138 insertions(+), 22 deletions(-) >>>>=20 >>>> diff --git a/drivers/net/gve/gve_ethdev.c = b/drivers/net/gve/gve_ethdev.c >>>> index fef2458a16..e31fdce960 100644 >>>> --- a/drivers/net/gve/gve_ethdev.c >>>> +++ b/drivers/net/gve/gve_ethdev.c >>>> @@ -266,7 +266,7 @@ gve_dev_close(struct rte_eth_dev *dev) >>>> } >>>>=20 >>>> static int >>>> -gve_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info >>>> *dev_info) >>>> +gve_dev_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info >>>> *dev_info) >>>> { >>>> struct gve_priv *priv =3D dev->data->dev_private; >>>>=20 >>>> @@ -319,15 +319,12 @@ gve_dev_info_get(struct rte_eth_dev *dev, >>>> struct rte_eth_dev_info *dev_info) >>>> } >>>>=20 >>>> static int >>>> -gve_dev_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats = *stats) >>>> +gve_stats_get(struct rte_eth_dev *dev, struct rte_eth_stats = *stats) >>>> { >>>> uint16_t i; >>>>=20 >>>> for (i =3D 0; i < dev->data->nb_tx_queues; i++) { >>>> struct gve_tx_queue *txq =3D dev->data->tx_queues[i]; >>>> -if (txq =3D=3D NULL) >>>> -continue; >>>> - >>>=20 >>> I assume check is removed because it is unnecessary, but again not >>> directly related with the patch, can you also drop these from the = patch >>> to reduce the noise. >>>=20 >>>> stats->opackets +=3D txq->packets; >>>> stats->obytes +=3D txq->bytes; >>>> stats->oerrors +=3D txq->errors; >>>> @@ -335,9 +332,6 @@ gve_dev_stats_get(struct rte_eth_dev *dev, = struct >>>> rte_eth_stats *stats) >>>>=20 >>>> for (i =3D 0; i < dev->data->nb_rx_queues; i++) { >>>> struct gve_rx_queue *rxq =3D dev->data->rx_queues[i]; >>>> -if (rxq =3D=3D NULL) >>>> -continue; >>>> - >>>> stats->ipackets +=3D rxq->packets; >>>> stats->ibytes +=3D rxq->bytes; >>>> stats->ierrors +=3D rxq->errors; >>>> @@ -348,15 +342,12 @@ gve_dev_stats_get(struct rte_eth_dev *dev, >>>> struct rte_eth_stats *stats) >>>> } >>>>=20 >>>> static int >>>> -gve_dev_stats_reset(struct rte_eth_dev *dev) >>>> +gve_stats_reset(struct rte_eth_dev *dev) >>>> { >>>> uint16_t i; >>>>=20 >>>> for (i =3D 0; i < dev->data->nb_tx_queues; i++) { >>>> struct gve_tx_queue *txq =3D dev->data->tx_queues[i]; >>>> -if (txq =3D=3D NULL) >>>> -continue; >>>> - >>>> txq->packets =3D 0; >>>> txq->bytes =3D 0; >>>> txq->errors =3D 0; >>>> @@ -364,9 +355,6 @@ gve_dev_stats_reset(struct rte_eth_dev *dev) >>>>=20 >>>> for (i =3D 0; i < dev->data->nb_rx_queues; i++) { >>>> struct gve_rx_queue *rxq =3D dev->data->rx_queues[i]; >>>> -if (rxq =3D=3D NULL) >>>> -continue; >>>> - >>>> rxq->packets =3D 0; >>>> rxq->bytes =3D 0; >>>> rxq->errors =3D 0; >>>> @@ -377,7 +365,7 @@ gve_dev_stats_reset(struct rte_eth_dev *dev) >>>> } >>>>=20 >>>> static int >>>> -gve_dev_mtu_set(struct rte_eth_dev *dev, uint16_t mtu) >>>> +gve_mtu_set(struct rte_eth_dev *dev, uint16_t mtu) >>>> { >>>> struct gve_priv *priv =3D dev->data->dev_private; >>>> int err; >>>> @@ -403,20 +391,144 @@ gve_dev_mtu_set(struct rte_eth_dev *dev, >>>> uint16_t mtu) >>>> return 0; >>>> } >>>>=20 >>>> +static int >>>> +gve_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat >>>> *xstats, unsigned int n) >>>> +{ >>>> +if (xstats) { >>>=20 >>> To reduce indentation (and increase readability) you can prefer: >>> `` >>> if !xstats >>> return count; >>>=20 >>> >>> `` >>>=20 >>>> +uint requested =3D n; >>>=20 >>> uint? C#? Please prefer standard "unsigned int" type. >>>=20 >>>> +uint64_t indx =3D 0; >>>> +struct rte_eth_xstat *xstat =3D xstats; >>>> +uint16_t i; >>>> + >>>> +for (i =3D 0; i < dev->data->nb_rx_queues; i++) { >>>> +struct gve_rx_queue *rxq =3D dev->data->rx_queues[i]; >>>> +xstat->id =3D indx++; >>>> +xstat->value =3D rxq->packets; >>>> +if (--requested =3D=3D 0) >>>> +return n; >>>=20 >>> And in this case, ethdev layer does the checks and return = accordingly, >>> so need to try to fill the stats as much as possible, can you please >>> double check the ethdev API? >>>=20 >>>> +xstat++; >>>> + >>>> +xstat->id =3D indx++; >>>> +xstat->value =3D rxq->bytes; >>>> +if (--requested =3D=3D 0) >>>> +return n; >>>> +xstat++; >>>> + >>>> +xstat->id =3D indx++; >>>> +xstat->value =3D rxq->errors; >>>> +if (--requested =3D=3D 0) >>>> +return n; >>>> +xstat++; >>>> + >>>> +xstat->id =3D indx++; >>>> +xstat->value =3D rxq->no_mbufs; >>>> +if (--requested =3D=3D 0) >>>> +return n; >>>> +xstat++; >>>> +} >>>> + >>>> +for (i =3D 0; i < dev->data->nb_tx_queues; i++) { >>>> +struct gve_tx_queue *txq =3D dev->data->tx_queues[i]; >>>> +xstat->id =3D indx++; >>>> +xstat->value =3D txq->packets; >>>> +if (--requested =3D=3D 0) >>>> +return n; >>>> +xstat++; >>>> + >>>> +xstat->id =3D indx++; >>>> +xstat->value =3D txq->bytes; >>>> +if (--requested =3D=3D 0) >>>> +return n; >>>> +xstat++; >>>> + >>>> +xstat->id =3D indx++; >>>> +xstat->value =3D txq->errors; >>>> +if (--requested =3D=3D 0) >>>> +return n; >>>> +xstat++; >>>> +} >>>=20 >>>=20 >>> This approach is error to prone and close to extension, >>> many inplementations prefer to have an itnernal struct to link = between >>> names and values, something like: >>> struct name_offset { >>> char name[RTE_ETH_XSTATS_NAME_SIZE]; >>> uint32_t offset; >>> }; >>>=20 >>> 'name' holds the xstat name, for this patch it will be only = repeating >>> part of name like 'packets', 'bytes', etc.. and you need to = construct >>> the full name on the fly, that is why it you may prefer to add type = to >>> above struct to diffrenciate Rx and Tx and use it for name creation, = up >>> to you. >>>=20 >>>=20 >>> 'offset' holds the offset of corresponding value in a struct, for = you it >>> is "struct gve_rx_queue" & "struct gve_tx_queue", since there are = two >>> different structs it helps to create helper macro that gets offset = from >>> correct struct. >>>=20 >>> struct name_offset rx_name_offset[] =3D { >>> { "packets", offsetof(struct gve_rx_queue, packets) }, >>> .... >>> } >>>=20 >>>=20 >>> for (i =3D 0; i < dev->data->nb_rx_queues; i++) { >>> struct gve_rx_queue *rxq =3D dev->data->rx_queues[i]; >>> addr =3D (char *)rxq + rx_name_offset[i].offset; >>> xstats[index++].value =3D *addr; >>> } >>> for (i =3D 0; i < dev->data->nb_tx_queues; i++) { >>> struct gve_tx_queue *txq =3D dev->data->tx_queues[i]; >>> addr =3D (char *)txq + tx_name_offset[i].offset; >>> xstats[index++].value =3D *addr; >>> } >>>=20 >>> There are many sample of this in other drivers. >>>=20 >>> And since for this case instead of having fixed numbe of names, = there >>> are dynamiccaly changing queue names, >>>=20 >>>=20 >>>> +} >>>> + >>>> +return (dev->data->nb_rx_queues * 4) + (dev->data->nb_tx_queues * = 3); >>>=20 >>> When above suggested 'name_offset' struct used, you can use size of = it >>> instead of hardcoded 4 & 3 values. >>>=20 >>> With above sample, it becomes: >>>=20 >>> return (dev->data->nb_rx_queues * RTE_DIM(rx_name_offset)) + >>> (dev->data->nb_tx_queues * RTE_DIM(tx_name_offset)); >>>=20 >>>=20 >>>> +} >>>> + >>>> +static int >>>> +gve_xstats_reset(struct rte_eth_dev *dev) >>>> +{ >>>> +return gve_stats_reset(dev); >>>> +} >>>> + >>>> +static int >>>> +gve_xstats_get_names(struct rte_eth_dev *dev, struct >>>> rte_eth_xstat_name *xstats_names, >>>> +unsigned int n) >>>> +{ >>>> +if (xstats_names) { >>>> +uint requested =3D n; >>>> +struct rte_eth_xstat_name *xstats_name =3D xstats_names; >>>> +uint16_t i; >>>> + >>>> +for (i =3D 0; i < dev->data->nb_rx_queues; i++) { >>>> +snprintf(xstats_name->name, sizeof(xstats_name->name), >>>> +"rx_q%d_packets", i); >>>> +if (--requested =3D=3D 0) >>>> +return n; >>>> +xstats_name++; >>>> +snprintf(xstats_name->name, sizeof(xstats_name->name), >>>> +"rx_q%d_bytes", i); >>>> +if (--requested =3D=3D 0) >>>> +return n; >>>> +xstats_name++; >>>> +snprintf(xstats_name->name, sizeof(xstats_name->name), >>>> +"rx_q%d_errors", i); >>>> +if (--requested =3D=3D 0) >>>> +return n; >>>> +xstats_name++; >>>> +snprintf(xstats_name->name, sizeof(xstats_name->name), >>>> +"rx_q%d_no_mbufs", i); >>>> +if (--requested =3D=3D 0) >>>> +return n; >>>> +xstats_name++; >>>> +} >>>> + >>>=20 >>> And again according above samples this becomes: >>>=20 >>> for (i =3D 0; i < dev->data->nb_rx_queues; i++) { >>> for (j =3D 0; j < RTE_DIM(rx_name_offset); j++) { >>> snprintf(xstats_names[index++].name, sizeof(), "rx_q%d_%s", >>> i, rx_name_offset[j].name); >>> } >>>=20 >>>=20 >>>> +for (i =3D 0; i < dev->data->nb_tx_queues; i++) { >>>> +snprintf(xstats_name->name, sizeof(xstats_name->name), >>>> +"tx_q%d_packets", i); >>>> +if (--requested =3D=3D 0) >>>> +return n; >>>> +xstats_name++; >>>> +snprintf(xstats_name->name, sizeof(xstats_name->name), >>>> +"tx_q%d_bytes", i); >>>> +if (--requested =3D=3D 0) >>>> +return n; >>>> +xstats_name++; >>>> +snprintf(xstats_name->name, sizeof(xstats_name->name), >>>> +"tx_q%d_errors", i); >>>> +if (--requested =3D=3D 0) >>>> +return n; >>>> +xstats_name++; >>>> +} >>>> +} >>>> + >>>> +return (dev->data->nb_rx_queues * 4) + (dev->data->nb_tx_queues * = 3); >>>> +} >>>> + >>>> static const struct eth_dev_ops gve_eth_dev_ops =3D { >>>> .dev_configure =3D gve_dev_configure, >>>> .dev_start =3D gve_dev_start, >>>> .dev_stop =3D gve_dev_stop, >>>> .dev_close =3D gve_dev_close, >>>> -.dev_infos_get =3D gve_dev_info_get, >>>> +.dev_infos_get =3D gve_dev_infos_get, >>>> .rx_queue_setup =3D gve_rx_queue_setup, >>>> .tx_queue_setup =3D gve_tx_queue_setup, >>>> .rx_queue_release =3D gve_rx_queue_release, >>>> .tx_queue_release =3D gve_tx_queue_release, >>>> .link_update =3D gve_link_update, >>>> -.stats_get =3D gve_dev_stats_get, >>>> -.stats_reset =3D gve_dev_stats_reset, >>>> -.mtu_set =3D gve_dev_mtu_set, >>>> +.stats_get =3D gve_stats_get, >>>> +.stats_reset =3D gve_stats_reset, >>>> +.mtu_set =3D gve_mtu_set, >>>> +.xstats_get =3D gve_xstats_get, >>>> +.xstats_reset =3D gve_xstats_reset, >>>> +.xstats_get_names =3D gve_xstats_get_names, >>>> }; >>>>=20 >>>> static void >>>> diff --git a/drivers/net/gve/gve_rx.c b/drivers/net/gve/gve_rx.c >>>> index 66fbcf3930..7687977003 100644 >>>> --- a/drivers/net/gve/gve_rx.c >>>> +++ b/drivers/net/gve/gve_rx.c >>>> @@ -22,8 +22,10 @@ gve_rx_refill(struct gve_rx_queue *rxq) >>>> if (diag < 0) { >>>> for (i =3D 0; i < nb_alloc; i++) { >>>> nmb =3D rte_pktmbuf_alloc(rxq->mpool); >>>> -if (!nmb) >>>> +if (!nmb) { >>>> +rxq->no_mbufs++; >>>=20 >>> Why this is needed, original code is as following: >>>=20 >>> `` >>> for (i =3D 0; i < nb_alloc; i++) { >>> nmb =3D rte_pktmbuf_alloc(rxq->mpool); >>> if (!nmb) >>> break; >>> rxq->sw_ring[idx + i] =3D nmb; >>> } >>> if (i !=3D nb_alloc) { >>> rxq->no_mbufs +=3D nb_alloc - i; >>> nb_alloc =3D i; >>> } >>> `` >>>=20 >>> "if (i !=3D nb_alloc)" block already takes care of 'rxq->no_mbufs' = value, >>> is an additional increment required in the for loop? >>>=20 >>>=20 >>> And change is unrelated with the patch anyway. >>>=20 >>>> break; >>>> +} >>>> rxq->sw_ring[idx + i] =3D nmb; >>>> } >>>> if (i !=3D nb_alloc) { >>>> @@ -57,8 +59,10 @@ gve_rx_refill(struct gve_rx_queue *rxq) >>>> if (diag < 0) { >>>> for (i =3D 0; i < nb_alloc; i++) { >>>> nmb =3D rte_pktmbuf_alloc(rxq->mpool); >>>> -if (!nmb) >>>> +if (!nmb) { >>>> +rxq->no_mbufs++; >>>> break; >>>> +} >>>> rxq->sw_ring[idx + i] =3D nmb; >>>> } >>>> nb_alloc =3D i; >>=20 >=20 --Apple-Mail=_A94BE9DE-D586-4151-8A8A-BA18CA4410AA Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 Hi = Ferruh,

Opps, I was not aware of that = rejection.
Thanks for notification.
  
Let me supersede this = patch.
And create a new = one. 

Best,
Levend
=
On 19 Feb 2023, at 23:09, Ferruh = Yigit <ferruh.yigit@amd.com> wrote:

On 2/19/2023 12:26 AM, = Levend Sayar wrote:
Ferruh,

Thanks = for this detailed review.
I am setting superseded this patch and = create a new one.
You=E2=80=99re right at all points.
For = rx.no_mbufs counting, I probably overlooked while rebasing my = patch
and it is mixed with newly came patch.

When I check = ethdev layer again, I noticed that when = dev_flags
has RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS
Rx/tx queue = stats are already added. I am pushing a fresh patch for
adding rx/tx = queue stats.


Hi Levend,

You are right = BUT, 'RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS' is a temporary
solution and = plan is to remove it [1].

Background is, queue stats in "struct = rte_eth_stats" has fixed size and
as number of queues supported by = devices increase these fields getting
bigger and bigger, the solution = we came was to completely remove these
fields from stats struct and = get queue statistics via xstats.

During transition = 'RTE_ETH_DEV_AUTOFILL_QUEUE_XSTATS' is introduced
until all drivers = implement queue stats in xstats. We are not pushing
hard for existing = drivers to update but at least requiring new drivers
to implement = xstats method.

That is why for net/gve updating queue stats in = 'gve_dev_stats_get()'
rejected before, and xstats implementation is = requested.


[1]
https://elixir.bootlin.com/dpdk/v22.11.1/sour= ce/doc/guides/rel_notes/deprecation.rst#L88


And also noticed a missing part at rx no_mbufs = counting.

Best,
Levend
 

On 17 Feb 2023, at 15:34, Ferruh Yigit = <ferruh.yigit@amd.com> wrote:

On 2/16/2023 6:58 PM, Levend = Sayar wrote:
Google Virtual NIC PMD is = enriched with extended statistics info.

Only queue = stats added to xstats, can you please highlight this in the
commit = log?

eth_dev_ops callback names are = also synched with eth_dev_ops field = names


Renaming eth_dev_ops is not related to = xstats, and I am not sure if the
change is necessary, can you please = drop it from this patch?

Signed-off-by: = Levend Sayar = <levendsayar@gmail.com>
---
drivers/net/gve/gve_ethdev.c | = 152 ++++++++++++++++++++++++++++++-----
drivers/net/gve/gve_rx.c =     |   8 +-
2 files changed, 138 = insertions(+), 22 deletions(-)

diff --git = a/drivers/net/gve/gve_ethdev.c b/drivers/net/gve/gve_ethdev.c
index = fef2458a16..e31fdce960 100644
--- = a/drivers/net/gve/gve_ethdev.c
+++ = b/drivers/net/gve/gve_ethdev.c
@@ -266,7 +266,7 @@ = gve_dev_close(struct rte_eth_dev *dev)
}

static = int
-gve_dev_info_get(struct rte_eth_dev *dev, struct = rte_eth_dev_info
*dev_info)
+gve_dev_infos_get(struct rte_eth_dev = *dev, struct rte_eth_dev_info
*dev_info)
{
struct gve_priv = *priv =3D dev->data->dev_private;

@@ -319,15 +319,12 @@ = gve_dev_info_get(struct rte_eth_dev *dev,
struct rte_eth_dev_info = *dev_info)
}

static int
-gve_dev_stats_get(struct = rte_eth_dev *dev, struct rte_eth_stats *stats)
+gve_stats_get(struct = rte_eth_dev *dev, struct rte_eth_stats *stats)
{
uint16_t = i;

for (i =3D 0; i < dev->data->nb_tx_queues; i++) = {
struct gve_tx_queue *txq =3D dev->data->tx_queues[i];
-if = (txq =3D=3D NULL)
-continue;
-

I assume check = is removed because it is unnecessary, but again not
directly related = with the patch, can you also drop these from the patch
to reduce the = noise.

stats->opackets +=3D = txq->packets;
stats->obytes +=3D = txq->bytes;
stats->oerrors +=3D txq->errors;
@@ -335,9 = +332,6 @@ gve_dev_stats_get(struct rte_eth_dev *dev, = struct
rte_eth_stats *stats)

for (i =3D 0; i < = dev->data->nb_rx_queues; i++) {
struct gve_rx_queue *rxq =3D = dev->data->rx_queues[i];
-if (rxq =3D=3D = NULL)
-continue;
-
stats->ipackets +=3D = rxq->packets;
stats->ibytes +=3D = rxq->bytes;
stats->ierrors +=3D rxq->errors;
@@ -348,15 = +342,12 @@ gve_dev_stats_get(struct rte_eth_dev *dev,
struct = rte_eth_stats *stats)
}

static = int
-gve_dev_stats_reset(struct rte_eth_dev = *dev)
+gve_stats_reset(struct rte_eth_dev *dev)
{
uint16_t = i;

for (i =3D 0; i < dev->data->nb_tx_queues; i++) = {
struct gve_tx_queue *txq =3D dev->data->tx_queues[i];
-if = (txq =3D=3D NULL)
-continue;
-
txq->packets  =3D = 0;
txq->bytes =3D 0;
txq->errors =3D 0;
@@ -364,9 +355,6 = @@ gve_dev_stats_reset(struct rte_eth_dev *dev)

for (i =3D 0; i = < dev->data->nb_rx_queues; i++) {
struct gve_rx_queue *rxq =3D= dev->data->rx_queues[i];
-if (rxq =3D=3D = NULL)
-continue;
-
rxq->packets  =3D = 0;
rxq->bytes =3D 0;
rxq->errors =3D 0;
@@ -377,7 +365,7 = @@ gve_dev_stats_reset(struct rte_eth_dev *dev)
}

static = int
-gve_dev_mtu_set(struct rte_eth_dev *dev, uint16_t = mtu)
+gve_mtu_set(struct rte_eth_dev *dev, uint16_t = mtu)
{
struct gve_priv *priv =3D = dev->data->dev_private;
int err;
@@ -403,20 +391,144 @@ = gve_dev_mtu_set(struct rte_eth_dev *dev,
uint16_t mtu)
return = 0;
}

+static int
+gve_xstats_get(struct rte_eth_dev *dev, = struct rte_eth_xstat
*xstats, unsigned int n)
+{
+if (xstats) = {

To reduce indentation (and increase readability) = you can prefer:
``
if !xstats
return count;

<put rest = of logic here>
``

+uint requested = =3D n;

uint? C#? Please prefer standard "unsigned = int" type.

+uint64_t indx =3D = 0;
+struct rte_eth_xstat *xstat =3D xstats;
+uint16_t = i;
+
+for (i =3D 0; i < dev->data->nb_rx_queues; i++) = {
+struct gve_rx_queue *rxq =3D = dev->data->rx_queues[i];
+xstat->id =3D = indx++;
+xstat->value =3D rxq->packets;
+if (--requested =3D=3D= 0)
+return n;

And in this case, ethdev layer = does the checks and return accordingly,
so need to try to fill the = stats as much as possible, can you please
double check the ethdev = API?

+xstat++;
+
+xstat->id =3D = indx++;
+xstat->value =3D rxq->bytes;
+if (--requested =3D=3D = 0)
+return n;
+xstat++;
+
+xstat->id =3D = indx++;
+xstat->value =3D rxq->errors;
+if (--requested =3D=3D= 0)
+return n;
+xstat++;
+
+xstat->id =3D = indx++;
+xstat->value =3D rxq->no_mbufs;
+if (--requested =3D=3D= 0)
+return n;
+xstat++;
+}
+
+for (i =3D 0; i < = dev->data->nb_tx_queues; i++) {
+struct gve_tx_queue *txq =3D = dev->data->tx_queues[i];
+xstat->id =3D = indx++;
+xstat->value =3D txq->packets;
+if (--requested =3D=3D= 0)
+return n;
+xstat++;
+
+xstat->id =3D = indx++;
+xstat->value =3D txq->bytes;
+if (--requested =3D=3D = 0)
+return n;
+xstat++;
+
+xstat->id =3D = indx++;
+xstat->value =3D txq->errors;
+if (--requested =3D=3D= 0)
+return n;
+xstat++;
+}


This = approach is error to prone and close to extension,
many = inplementations prefer to have an itnernal struct to link = between
names and values, something like:
struct name_offset = {
char name[RTE_ETH_XSTATS_NAME_SIZE];
uint32_t = offset;
};

'name' holds the xstat name, for this patch it will = be only repeating
part of name like 'packets', 'bytes', etc.. and you = need to construct
the full name on the fly, that is why it you may = prefer to add type to
above struct to diffrenciate Rx and Tx and use = it for name creation, up
to you.


'offset' holds the offset = of corresponding value in a struct, for you it
is "struct = gve_rx_queue" & "struct gve_tx_queue", since there are = two
different structs it helps to create helper macro that gets = offset from
correct struct.

struct name_offset = rx_name_offset[] =3D {
{ "packets", offsetof(struct gve_rx_queue, = packets) = },
       ....
}


for = (i =3D 0; i < dev->data->nb_rx_queues; i++) {
struct = gve_rx_queue *rxq =3D dev->data->rx_queues[i];
addr =3D (char = *)rxq + rx_name_offset[i].offset;
xstats[index++].value =3D = *addr;
}
for (i =3D 0; i < dev->data->nb_tx_queues; i++) = {
struct gve_tx_queue *txq =3D dev->data->tx_queues[i];
addr = =3D (char *)txq + tx_name_offset[i].offset;
xstats[index++].value =3D = *addr;
}

There are many sample of this in other = drivers.

And since for this case instead of having fixed numbe of = names, there
are dynamiccaly changing queue = names,


+}
+
+return = (dev->data->nb_rx_queues * 4) + (dev->data->nb_tx_queues * = 3);

When above suggested 'name_offset' struct used, = you can use size of it
instead of hardcoded 4 & 3 = values.

With above sample, it becomes:

return = (dev->data->nb_rx_queues * RTE_DIM(rx_name_offset)) = +
(dev->data->nb_tx_queues * = RTE_DIM(tx_name_offset));


+}
+
+static int
+gve_xstats_reset(struct = rte_eth_dev *dev)
+{
+return = gve_stats_reset(dev);
+}
+
+static = int
+gve_xstats_get_names(struct rte_eth_dev *dev, = struct
rte_eth_xstat_name *xstats_names,
+unsigned int = n)
+{
+if (xstats_names) {
+uint requested =3D n;
+struct = rte_eth_xstat_name *xstats_name =3D xstats_names;
+uint16_t = i;
+
+for (i =3D 0; i < dev->data->nb_rx_queues; i++) = {
+snprintf(xstats_name->name, = sizeof(xstats_name->name),
+"rx_q%d_packets", i);
+if = (--requested =3D=3D 0)
+return = n;
+xstats_name++;
+snprintf(xstats_name->name, = sizeof(xstats_name->name),
+"rx_q%d_bytes", i);
+if = (--requested =3D=3D 0)
+return = n;
+xstats_name++;
+snprintf(xstats_name->name, = sizeof(xstats_name->name),
+"rx_q%d_errors", i);
+if = (--requested =3D=3D 0)
+return = n;
+xstats_name++;
+snprintf(xstats_name->name, = sizeof(xstats_name->name),
+"rx_q%d_no_mbufs", i);
+if = (--requested =3D=3D 0)
+return = n;
+xstats_name++;
+}
+

And again according = above samples this becomes:

for (i =3D 0; i < = dev->data->nb_rx_queues; i++) {
   for (j =3D 0; = j < RTE_DIM(rx_name_offset); j++) = {
snprintf(xstats_names[index++].name, sizeof(), "rx_q%d_%s",
i, = rx_name_offset[j].name);
}


+for = (i =3D 0; i < dev->data->nb_tx_queues; i++) = {
+snprintf(xstats_name->name, = sizeof(xstats_name->name),
+"tx_q%d_packets", i);
+if = (--requested =3D=3D 0)
+return = n;
+xstats_name++;
+snprintf(xstats_name->name, = sizeof(xstats_name->name),
+"tx_q%d_bytes", i);
+if = (--requested =3D=3D 0)
+return = n;
+xstats_name++;
+snprintf(xstats_name->name, = sizeof(xstats_name->name),
+"tx_q%d_errors", i);
+if = (--requested =3D=3D 0)
+return = n;
+xstats_name++;
+}
+}
+
+return = (dev->data->nb_rx_queues * 4) + (dev->data->nb_tx_queues * = 3);
+}
+
static const struct eth_dev_ops gve_eth_dev_ops =3D = {
.dev_configure        =3D = gve_dev_configure,
.dev_start =            =3D = gve_dev_start,
.dev_stop =             =3D= gve_dev_stop,
.dev_close =            =3D = gve_dev_close,
-.dev_infos_get =        =3D = gve_dev_info_get,
+.dev_infos_get =        =3D = gve_dev_infos_get,
.rx_queue_setup =       =3D = gve_rx_queue_setup,
.tx_queue_setup =       =3D = gve_tx_queue_setup,
.rx_queue_release     =3D = gve_rx_queue_release,
.tx_queue_release     =3D = gve_tx_queue_release,
.link_update =          =3D = gve_link_update,
-.stats_get =            =3D = gve_dev_stats_get,
-.stats_reset =          =3D = gve_dev_stats_reset,
-.mtu_set =             &n= bsp;=3D gve_dev_mtu_set,
+.stats_get =            =3D = gve_stats_get,
+.stats_reset =          =3D = gve_stats_reset,
+.mtu_set =             &n= bsp;=3D gve_mtu_set,
+.xstats_get =           =3D = gve_xstats_get,
+.xstats_reset =         =3D = gve_xstats_reset,
+.xstats_get_names     =3D = gve_xstats_get_names,
};

static void
diff --git = a/drivers/net/gve/gve_rx.c b/drivers/net/gve/gve_rx.c
index = 66fbcf3930..7687977003 100644
--- a/drivers/net/gve/gve_rx.c
+++ = b/drivers/net/gve/gve_rx.c
@@ -22,8 +22,10 @@ gve_rx_refill(struct = gve_rx_queue *rxq)
if (diag < 0) {
for (i =3D 0; i < = nb_alloc; i++) {
nmb =3D rte_pktmbuf_alloc(rxq->mpool);
-if = (!nmb)
+if (!nmb) {
+rxq->no_mbufs++;

Why = this is needed, original code is as following:

``
for (i =3D = 0; i < nb_alloc; i++) {
nmb =3D = rte_pktmbuf_alloc(rxq->mpool);
if = (!nmb)
break;
rxq->sw_ring[idx + i] =3D nmb;
}
if (i !=3D = nb_alloc) {
rxq->no_mbufs +=3D nb_alloc - i;
nb_alloc =3D = i;
}
``

"if (i !=3D nb_alloc)" block already takes care of = 'rxq->no_mbufs' value,
is an additional increment required in the = for loop?


And change is unrelated with the patch = anyway.

break;
+}
rxq->sw_ring[idx + i] =3D = nmb;
}
if (i !=3D nb_alloc) {
@@ -57,8 +59,10 @@ = gve_rx_refill(struct gve_rx_queue *rxq)
if (diag < 0) {
for (i = =3D 0; i < nb_alloc; i++) {
nmb =3D = rte_pktmbuf_alloc(rxq->mpool);
-if (!nmb)
+if (!nmb) = {
+rxq->no_mbufs++;
break;
+}
rxq->sw_ring[idx + i] =3D = nmb;
}
nb_alloc =3D = i;



= --Apple-Mail=_A94BE9DE-D586-4151-8A8A-BA18CA4410AA--