From: Levend Sayar <levendsayar@gmail.com>
To: Ferruh Yigit <ferruh.yigit@amd.com>
Cc: "Guo, Junfeng" <junfeng.guo@intel.com>, dev@dpdk.org
Subject: Re: [PATCH v3 2/2] net/gve: add Rx/Tx queue stats as extended stats
Date: Tue, 21 Feb 2023 14:11:56 +0300 [thread overview]
Message-ID: <23F32959-1B37-4453-B69C-ACD33769C8D2@gmail.com> (raw)
In-Reply-To: <d1cbcd33-0376-eac2-2140-6215455484e6@amd.com>
Thanks Ferruh for the review.
My comments are inlined.
> On 21 Feb 2023, at 01:57, Ferruh Yigit <ferruh.yigit@amd.com> wrote:
>
> On 2/20/2023 9:11 PM, Levend Sayar wrote:
>> Google Virtual NIC rx/tx queue stats are added as extended stats.
>>
>> Signed-off-by: Levend Sayar <levendsayar@gmail.com>
>
> Thank you for the update, mainly looks good to me, there area a few
> minor questions/comments below.
>
> <...>
>> +static const struct gve_xstats_name_offset tx_xstats_name_offset[] = {
>> + { "packets", offsetof(struct gve_tx_stats, packets) },
>> + { "bytes", offsetof(struct gve_tx_stats, bytes) },
>> + { "errors", offsetof(struct gve_tx_stats, errors) },
>> +};
>> +
>
> 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:
>
> #define RX_QUEUE_STATS_OFFSET(x) offsetof(struct gve_rx_stats, X)
> #define TX_QUEUE_STATS_OFFSET(x) offsetof(struct gve_tx_stats, X)
>
LS: I take dpdk code as reference and mimicked the usage on rte_ethdev.c here.
Your proposal surely applicable.
>> +static const struct gve_xstats_name_offset rx_xstats_name_offset[] = {
>> + { "packets", offsetof(struct gve_rx_stats, packets) },
>> + { "bytes", offsetof(struct gve_rx_stats, bytes) },
>> + { "errors", offsetof(struct gve_rx_stats, errors) },
>> +};
>> +
>
> Is 'no_mbufs' omitted intentionally?
>
> <...>
LS: no_mbufs are accumulated as rx_mbuf_allocation_errors on basic stats. But yes, it can be queue based also.
>>
>> +static int
>> +gve_xstats_count(struct rte_eth_dev *dev)
>> +{
>> + uint16_t i, count = 0;
>> +
>> + for (i = 0; i < dev->data->nb_tx_queues; i++) {
>> + if (dev->data->tx_queues[i])
>
> Normally 'dev->data->tx_queues[i]' shouldn't be NULL, but I can see
> driver checks this in a few locations.
>
> Is there a case that 'dev->data->tx_queues[i]' can be NULL where "0 <= i
> < dev->data->nb_tx_queues" ?
LS: You’re 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.
>
>> + count += RTE_DIM(tx_xstats_name_offset);
>> + }
>> +
>> + for (i = 0; i < dev->data->nb_rx_queues; i++) {
>> + if (dev->data->rx_queues[i])
>> + count += 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 = 0;
>> +
>> + if (!xstats)
>> + return (size == 0) ? gve_xstats_count(dev) : -EINVAL;
>
> Error case (xstats == NULL && size != 0) should be handled by ethdev, so
> driver can only check "xstats == NULL" case.
>
> btw, although we have mixed usage and not very strict on it, coding
> convention requires testing against NULL [1] instead of '!'.
>
> [1]
> https://doc.dpdk.org/guides/contributing/coding_style.html#null-pointers
LS: You’re right. rte_eth_xstats_get checks for (xstats == NULL && size != 0).
Let me correct that part and use NULL.
>
>> +
>> + for (i = 0; i < dev->data->nb_tx_queues; i++) {
>> + const struct gve_tx_queue *txq = dev->data->tx_queues[i];
>> + if (!txq)
>> + continue;
>> +
>
> similar to above comment, I expect 'txq' not to be NULL, isn't this
> correct for the driver?
>
>> + if (count >= size)
>> + break;
>> +
>
> Instead of above check in a loop, it is possible to check once at the
> beginning of the function like
>
> count = gve_xstats_count(dev)
> if (size < count)
> return count;
>
> 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’s say NIC has 20 xstats. Your application allocates xstats memory enough to hold 30
and request 30 xtstats. NIC will return 20 xstats. That’s ok. But if application allocates memory
For 10 xstats and request 10, it’s taken as error although Nic can fill first 10 xstats.
So size must be higher or equal to number of xstats. Right?
>
>> + uint16_t j = 0;
>> + const char *stats = (const char *)&txq->stats;
>
> 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 =0 ...
That’s a quite surprise for me when I got an error from checkpatches.sh.
>
>> + for (j = 0; j < RTE_DIM(tx_xstats_name_offset); j++, count++) {
>> + xstats[count].id = count;
>> + xstats[count].value = *(const uint64_t *)
>> + (stats + tx_xstats_name_offset[j].offset);
>> + }
>> + }
>> +
>> + for (i = 0; i < dev->data->nb_rx_queues; i++) {
>> + const struct gve_rx_queue *rxq = dev->data->rx_queues[i];
>> + if (!rxq)
>> + continue;
>> +
>> + if (count >= size)
>> + break;
>> +
>> + uint16_t j = 0;
>> + const char *stats = (const char *)&rxq->stats;
>> + for (j = 0; j < RTE_DIM(rx_xstats_name_offset); j++, count++) {
>> + xstats[count].id = count;
>> + xstats[count].value = *(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 = 0;
>> +
>> + if (!xstats_names)
>> + return (size == 0) ? gve_xstats_count(dev) : -EINVAL;
>> +
>> + for (i = 0; i < dev->data->nb_tx_queues; i++) {
>> + if (!dev->data->tx_queues[i])
>> + continue;
>> +
>> + if (count >= size)
>> + break;
>> +
>> + uint16_t j = 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 = 0; i < dev->data->nb_rx_queues; i++) {
>> + if (!dev->data->rx_queues[i])
>> + continue;
>> +
>> + if (count >= size)
>> + break;
>> +
>> + uint16_t j = 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;
>> +}
>> +
>
> comments on 'gve_xstats_get()' valid here too.
>
next prev parent reply other threads:[~2023-02-21 11:12 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-02-19 0:30 [PATCH] net/gve: fix Rx no mbufs stats counter update Levend Sayar
2023-02-19 17:35 ` Stephen Hemminger
2023-02-19 20:43 ` Levend Sayar
2023-02-19 22:59 ` Stephen Hemminger
2023-02-20 15:19 ` [PATCH v2] " Levend Sayar
2023-02-20 21:11 ` [PATCH v3 1/2] " Levend Sayar
2023-02-20 21:11 ` [PATCH v3 2/2] net/gve: add Rx/Tx queue stats as extended stats Levend Sayar
2023-02-20 22:57 ` Ferruh Yigit
2023-02-21 11:11 ` Levend Sayar [this message]
2023-02-21 14:13 ` [PATCH v4] " Levend Sayar
2023-02-21 14:18 ` [PATCH v5] " Levend Sayar
2023-02-21 15:58 ` Ferruh Yigit
2023-02-21 16:44 ` Levend Sayar
2023-02-23 2:49 ` Guo, Junfeng
2023-02-23 6:28 ` Levend Sayar
2023-02-23 11:09 ` Ferruh Yigit
2023-02-23 12:30 ` Levend Sayar
2023-02-20 22:57 ` [PATCH v3 1/2] net/gve: fix Rx no mbufs stats counter update Ferruh Yigit
2023-02-21 10:07 ` Levend Sayar
2023-02-21 10:30 ` Ferruh Yigit
2023-02-23 4:34 ` Guo, Junfeng
2023-02-23 6:29 ` Levend Sayar
2023-02-23 11:10 ` Ferruh Yigit
2023-02-23 12:29 ` Levend Sayar
2023-02-21 15:58 ` Ferruh Yigit
2023-02-21 16:42 ` Levend Sayar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=23F32959-1B37-4453-B69C-ACD33769C8D2@gmail.com \
--to=levendsayar@gmail.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@amd.com \
--cc=junfeng.guo@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).