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 B9A3941CFD; Tue, 21 Feb 2023 12:12:08 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 9E3B3431C3; Tue, 21 Feb 2023 12:12:08 +0100 (CET) Received: from mail-ed1-f46.google.com (mail-ed1-f46.google.com [209.85.208.46]) by mails.dpdk.org (Postfix) with ESMTP id D4D5D406A2 for ; Tue, 21 Feb 2023 12:12:07 +0100 (CET) Received: by mail-ed1-f46.google.com with SMTP id s26so15573663edw.11 for ; Tue, 21 Feb 2023 03:12:07 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=to:references:message-id:content-transfer-encoding:cc:date :in-reply-to:from:subject:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=3ET48aZdhMozNmyDG/d3T/0BxjdL9Q8T48oQZmyjY3s=; b=Q41ORmlCq3OsVo+IfgUe9dZrju4pkWfOuTYukvEzu2/ldHvK3+tgZJtS9tXaeKy8U3 f7e8WZaj58yu7v9jpT3Ufc3nUc4g7biP4p9NFWAU2msd3qS5+o/8CgBJ9zWXzCbYBsvF muHhPiS6MqxoXsY7Ef7qR2wzUtsJeEdiSPsk99am4YU1DR/mRebod8GZ9cQlu+kDu4ts motkWCAt0JL2jbV+wTzRIGFNv/Nddfy3nZOb5Q4sZmkGT34GAdt/+ELu5gd8/Gq9Ixds 7j0zLNxZiJIJSxNNmEDIJC/Hifw3maqDao7NiKA+pkVA/j2aCMsNBYYuQdexMwaVpwMJ N2mQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=to:references:message-id:content-transfer-encoding:cc:date :in-reply-to:from:subject:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=3ET48aZdhMozNmyDG/d3T/0BxjdL9Q8T48oQZmyjY3s=; b=XFmjoXqq7FlTm+ScsdwWVgNuzxipiC2tlqq21gy2gP5xBWXD0OaLLiFCZ5upxlmIvG UV8aYokytC5o9CSFnaudExABIsaq2hB7cyS+dRbgcRCiz/BcFs2uU0bs70dauiwq6Y9l gKtHN9HE/0lJnqCP4nXY4ydjPPkTzclHc8aKxvyK9uNUtQm2vGzkybrdAUTLKLovJraY abxGrm3XisMIV6Vhc8rU/YoHCSsXNeW/nJf1gVcnHiLXxNQgzWXUAy+e4NkC5miHWgwC RgTAvxJWFGWVBzK82p7KkU9BlZWMQlyGRPzlycV6G1U5XirFMvjY3oUrUp3//sj1wmsm 2bvw== X-Gm-Message-State: AO0yUKVDHTRsxx4Q198eSr1ZCLwMPGYH18/OoJ0DXyyFFArV4bc3k9xn bOGprMPJPOoUKwUPveWL+IE= X-Google-Smtp-Source: AK7set9VvbTejTLTxWMMFFnsKA+VgB5rguEAE/iw6YpBHNt6KBXVN8hJeJH6l4xB3RgiABJoX9gNcg== X-Received: by 2002:aa7:cc9a:0:b0:4aa:c77d:fa0a with SMTP id p26-20020aa7cc9a000000b004aac77dfa0amr3471732edt.22.1676977927473; Tue, 21 Feb 2023 03:12:07 -0800 (PST) Received: from smtpclient.apple ([176.41.28.141]) by smtp.gmail.com with ESMTPSA id b2-20020a50b402000000b004aef4f32edesm1372927edh.88.2023.02.21.03.12.06 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Tue, 21 Feb 2023 03:12:07 -0800 (PST) Content-Type: text/plain; charset=utf-8 Mime-Version: 1.0 (Mac OS X Mail 16.0 \(3731.300.101.1.3\)) Subject: Re: [PATCH v3 2/2] net/gve: add Rx/Tx queue stats as extended stats From: Levend Sayar In-Reply-To: Date: Tue, 21 Feb 2023 14:11:56 +0300 Cc: "Guo, Junfeng" , dev@dpdk.org Content-Transfer-Encoding: quoted-printable Message-Id: <23F32959-1B37-4453-B69C-ACD33769C8D2@gmail.com> References: <20230220151936.2716-1-levendsayar@gmail.com> <20230220211103.8282-1-levendsayar@gmail.com> <20230220211103.8282-2-levendsayar@gmail.com> To: Ferruh Yigit 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 Thanks Ferruh for the review. My comments are inlined. > On 21 Feb 2023, at 01:57, Ferruh Yigit wrote: >=20 > On 2/20/2023 9:11 PM, Levend Sayar wrote: >> Google Virtual NIC rx/tx queue stats are added as extended stats. >>=20 >> Signed-off-by: Levend Sayar >=20 > Thank you for the update, mainly looks good to me, there area a few > minor questions/comments below. >=20 > <...> >> +static const struct gve_xstats_name_offset tx_xstats_name_offset[] =3D= { >> + { "packets", offsetof(struct gve_tx_stats, packets) }, >> + { "bytes", offsetof(struct gve_tx_stats, bytes) }, >> + { "errors", offsetof(struct gve_tx_stats, errors) }, >> +}; >> + >=20 > It is possible to create macros to get offsets to prevent any mistakes > but not quite sure if it is needed with above limited number of items, > so up to you, I mean something like: >=20 > #define RX_QUEUE_STATS_OFFSET(x) offsetof(struct gve_rx_stats, X) > #define TX_QUEUE_STATS_OFFSET(x) offsetof(struct gve_tx_stats, X) >=20 LS: I take dpdk code as reference and mimicked the usage on rte_ethdev.c = here.=20 Your proposal surely applicable. >> +static const struct gve_xstats_name_offset rx_xstats_name_offset[] =3D= { >> + { "packets", offsetof(struct gve_rx_stats, packets) }, >> + { "bytes", offsetof(struct gve_rx_stats, bytes) }, >> + { "errors", offsetof(struct gve_rx_stats, errors) }, >> +}; >> + >=20 > Is 'no_mbufs' omitted intentionally? >=20 > <...> LS: no_mbufs are accumulated as rx_mbuf_allocation_errors on basic = stats. But yes, it can be queue based also.=20 >>=20 >> +static int >> +gve_xstats_count(struct rte_eth_dev *dev) >> +{ >> + uint16_t i, count =3D 0; >> + >> + for (i =3D 0; i < dev->data->nb_tx_queues; i++) { >> + if (dev->data->tx_queues[i]) >=20 > Normally 'dev->data->tx_queues[i]' shouldn't be NULL, but I can see > driver checks this in a few locations. >=20 > Is there a case that 'dev->data->tx_queues[i]' can be NULL where "0 <=3D= i > < dev->data->nb_tx_queues" ? LS: You=E2=80=99re right. On my previous patches I erased that checks = but that parts are reviewed as noise. So I am aligned with the rest of the code. In fact, there is no = gap like that. In dev_start, Queues are created and whenever queue can not be created, it = returns error at that point. So dev_start will return an error at the end. So if device = successfully started, all rx/tx queues must be created Successfully. Therefore no need to check.=20 >=20 >> + count +=3D RTE_DIM(tx_xstats_name_offset); >> + } >> + >> + for (i =3D 0; i < dev->data->nb_rx_queues; i++) { >> + if (dev->data->rx_queues[i]) >> + count +=3D RTE_DIM(rx_xstats_name_offset); >> + } >> + >> + return count; >> +} >> + >> +static int >> +gve_xstats_get(struct rte_eth_dev *dev, >> + struct rte_eth_xstat *xstats, >> + unsigned int size) >> +{ >> + uint16_t i, count =3D 0; >> + >> + if (!xstats) >> + return (size =3D=3D 0) ? gve_xstats_count(dev) : = -EINVAL; >=20 > Error case (xstats =3D=3D NULL && size !=3D 0) should be handled by = ethdev, so > driver can only check "xstats =3D=3D NULL" case. >=20 > btw, although we have mixed usage and not very strict on it, coding > convention requires testing against NULL [1] instead of '!'. >=20 > [1] > = https://doc.dpdk.org/guides/contributing/coding_style.html#null-pointers LS: You=E2=80=99re right. rte_eth_xstats_get checks for (xstats =3D=3D = NULL && size !=3D 0). Let me correct that part and use NULL. =20 >=20 >> + >> + for (i =3D 0; i < dev->data->nb_tx_queues; i++) { >> + const struct gve_tx_queue *txq =3D = dev->data->tx_queues[i]; >> + if (!txq) >> + continue; >> + >=20 > similar to above comment, I expect 'txq' not to be NULL, isn't this > correct for the driver? >=20 >> + if (count >=3D size) >> + break; >> + >=20 > Instead of above check in a loop, it is possible to check once at the > beginning of the function like >=20 > count =3D gve_xstats_count(dev) > if (size < count) > return count; >=20 > Because when there is not enough room in the xstats array, API doesn't > expect existing elements of array to be correct, returning a value > bigger than 'size' indicates error case and content of xstats is = invalid > anyway. LS: Let=E2=80=99s say NIC has 20 xstats. Your application allocates = xstats memory enough to hold 30 and request 30 xtstats. NIC will return 20 xstats. That=E2=80=99s ok. = But if application allocates memory For 10 xstats and request 10, it=E2=80=99s taken as error although Nic = can fill first 10 xstats. So size must be higher or equal to number of xstats. Right? =20 >=20 >> + uint16_t j =3D 0; >> + const char *stats =3D (const char *)&txq->stats; >=20 > Can you please move variable declarations at the beginning of the = scope, > for above variables it is just after for statement, according coding > convention. LS: Sure I can do. I supposed minimizing scope is better approach. But not here I guess. What is the rationale behind not declaring a = variable in a for loop like? for (int i =3D0 ... That=E2=80=99s a quite surprise for me when I got an error from = checkpatches.sh.=20 >=20 >> + for (j =3D 0; j < RTE_DIM(tx_xstats_name_offset); j++, = count++) { >> + xstats[count].id =3D count; >> + xstats[count].value =3D *(const uint64_t *) >> + (stats + = tx_xstats_name_offset[j].offset); >> + } >> + } >> + >> + for (i =3D 0; i < dev->data->nb_rx_queues; i++) { >> + const struct gve_rx_queue *rxq =3D = dev->data->rx_queues[i]; >> + if (!rxq) >> + continue; >> + >> + if (count >=3D size) >> + break; >> + >> + uint16_t j =3D 0; >> + const char *stats =3D (const char *)&rxq->stats; >> + for (j =3D 0; j < RTE_DIM(rx_xstats_name_offset); j++, = count++) { >> + xstats[count].id =3D count; >> + xstats[count].value =3D *(const uint64_t *) >> + (stats + = rx_xstats_name_offset[j].offset); >> + } >> + } >> + >> + return count; >> +} >> + >> +static int >> +gve_xstats_get_names(struct rte_eth_dev *dev, >> + struct rte_eth_xstat_name *xstats_names, >> + unsigned int size) >> +{ >> + uint16_t i, count =3D 0; >> + >> + if (!xstats_names) >> + return (size =3D=3D 0) ? gve_xstats_count(dev) : = -EINVAL; >> + >> + for (i =3D 0; i < dev->data->nb_tx_queues; i++) { >> + if (!dev->data->tx_queues[i]) >> + continue; >> + >> + if (count >=3D size) >> + break; >> + >> + uint16_t j =3D 0; >> + for (; j < RTE_DIM(tx_xstats_name_offset); j++) >> + snprintf(xstats_names[count++].name, >> + RTE_ETH_XSTATS_NAME_SIZE, >> + "tx_q%u_%s", i, = tx_xstats_name_offset[j].name); >> + } >> + >> + for (i =3D 0; i < dev->data->nb_rx_queues; i++) { >> + if (!dev->data->rx_queues[i]) >> + continue; >> + >> + if (count >=3D size) >> + break; >> + >> + uint16_t j =3D 0; >> + for (; j < RTE_DIM(rx_xstats_name_offset); j++) >> + snprintf(xstats_names[count++].name, >> + RTE_ETH_XSTATS_NAME_SIZE, >> + "rx_q%u_%s", i, = rx_xstats_name_offset[j].name); >> + } >> + >> + return count; >> +} >> + >=20 > comments on 'gve_xstats_get()' valid here too. >=20