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 3815D41C71; Sun, 19 Feb 2023 01:26:50 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C8AC640C35; Sun, 19 Feb 2023 01:26:49 +0100 (CET) Received: from mail-wm1-f41.google.com (mail-wm1-f41.google.com [209.85.128.41]) by mails.dpdk.org (Postfix) with ESMTP id 9D91C40698 for ; Sun, 19 Feb 2023 01:26:48 +0100 (CET) Received: by mail-wm1-f41.google.com with SMTP id bg25-20020a05600c3c9900b003e21af96703so836263wmb.2 for ; Sat, 18 Feb 2023 16:26:48 -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=85bODOxcLr6qiEpK1FKj2SVt1gmokUdLmEy0KNTOw9E=; b=O1bYPzfAKBQ2JDHYKJueGdQpiSUQl6RGLEfIeYNygZ2Rtgr4cqce31WZC8dewcODVQ JS8Md+uR5Jp/oKbxwUohaAeCwU7jwEiS/NmTQ5O5NJZAj0AWUAKXSzRfv9o535qQc+FI eZTnOhxMhXFHq6Zae1uVSWOq6gAITdOUX9/lwVhEfUFqLmcMesI4ZaawF1UrlLu/wS2f EkEkL4YP/rfnze2Jy3tttrzBkE9ahqXp5RcEoYl70XEg1Jn8eNqP4sgl7+RHG5f9bhgF IU8WrSAMFVkmkC/K9Q5YRTfHHSIjhq/Zy0CG/2eOMQXj/+j0GKKe/zGPccJ2COWdBC6Q HM9A== 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=85bODOxcLr6qiEpK1FKj2SVt1gmokUdLmEy0KNTOw9E=; b=Z4GjvFRMy3zv3RKwYCqZZ/quHTd2qcAEUrgE+FZB5ifr7P7F4rFqgeFGleVTBUxMXZ nR7kZpSLVID5PCmIbir6Id9eAZFlhOOhwlA6izWDRLN20fyzgbFlctoRyTgQa1eSiO/Y xknOggSSZiD0eJcu6a6mr3+h3VEnwv9NmHtvTySPkCQUGEKR/HVuadyF76D7f6yI9BTH Q7bZbjLt0W9wKsUw16JDjdLveVNBCUzQEorOiP1DD8RQC8R4TrcpQqTvpeOIYwg0x8kG nDjz1beh55OFCpGZqCcfQ0Mz/h//99sMjveNGRvw3/Syq2dCh3lrDi8jytg9MxF5UfeW TKvg== X-Gm-Message-State: AO0yUKVJG23OfPKJKfSSGDOFM0F2SG9VEah0cAfcnYq/REWVFgkxlzZ1 mgvNcz6WsMprGjfjfsd4LyU= X-Google-Smtp-Source: AK7set8vFMVAJW2o09dMzcwgahTnfafVZFoGOKI7BHBKIV+hta8nLMXtLLYafgxGNxNEnsCZMyuAzw== X-Received: by 2002:a05:600c:c07:b0:3df:9858:c030 with SMTP id fm7-20020a05600c0c0700b003df9858c030mr1313111wmb.5.1676766407939; Sat, 18 Feb 2023 16:26:47 -0800 (PST) Received: from smtpclient.apple ([176.41.28.141]) by smtp.gmail.com with ESMTPSA id m26-20020a05600c3b1a00b003e21a8d30c9sm9016557wms.37.2023.02.18.16.26.46 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Sat, 18 Feb 2023 16:26:47 -0800 (PST) From: Levend Sayar Message-Id: <2DEB3F1C-CEDE-49CF-B22D-B1F2D9A86203@gmail.com> Content-Type: multipart/alternative; boundary="Apple-Mail=_6FEDF263-0D42-496E-8E6C-EAF27E0098E7" 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 03:26:35 +0300 In-Reply-To: <6e18557e-0d7d-c8f8-13f5-4523a1226431@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> 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=_6FEDF263-0D42-496E-8E6C-EAF27E0098E7 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=utf-8 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. And also noticed a missing part at rx no_mbufs counting. Best, Levend =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; --Apple-Mail=_6FEDF263-0D42-496E-8E6C-EAF27E0098E7 Content-Transfer-Encoding: quoted-printable Content-Type: text/html; charset=utf-8 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.

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=_6FEDF263-0D42-496E-8E6C-EAF27E0098E7--