DPDK patches and discussions
 help / color / mirror / Atom feed
From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
To: Chengwen Feng <fengchengwen@huawei.com>,
	thomas@monjalon.net, ferruh.yigit@xilinx.com,
	ndabilpuram@marvell.com, kirankumark@marvell.com,
	skori@marvell.com, skoteshwar@marvell.com
Cc: mb@smartsharesystems.com, stephen@networkplumber.org, dev@dpdk.org
Subject: Re: [PATCH v3 03/10] net/hns3: adjust retval when xstats is null of get xstats
Date: Thu, 12 May 2022 12:51:58 +0300
Message-ID: <c3a6d0a1-a44c-fc19-18ab-3d286bfe620c@oktetlabs.ru> (raw)
In-Reply-To: <20220505080233.12737-4-fengchengwen@huawei.com>

On 5/5/22 11:02, Chengwen Feng wrote:
> Many user (e.g. telemetry) invokes rte_eth_xstats_get(port_id, NULL, 0)
> to retrieve the required number of elements, but currently hns3 PMD
> returns zero when xstats is NULL.
> 
> This patch adjusts that the return value was the required number of
> elements when stats is NULL.
> 
> Fixes: 8839c5e202f3 ("net/hns3: support device stats")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
>   drivers/net/hns3/hns3_stats.c | 7 ++-----
>   1 file changed, 2 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/hns3/hns3_stats.c b/drivers/net/hns3/hns3_stats.c
> index 806720faff..4165e22f7f 100644
> --- a/drivers/net/hns3/hns3_stats.c
> +++ b/drivers/net/hns3/hns3_stats.c
> @@ -1020,7 +1020,7 @@ hns3_imissed_stats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
>    * @praram xstats
>    *   A pointer to a table of structure of type *rte_eth_xstat*
>    *   to be filled with device statistics ids and values.
> - *   This parameter can be set to NULL if n is 0.
> + *   If set to NULL, the function returns the required number of elements.
>    * @param n
>    *   The size of the xstats array (number of elements).
>    * @return
> @@ -1041,11 +1041,8 @@ hns3_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat *xstats,
>   	int count;
>   	int ret;
>   
> -	if (xstats == NULL)
> -		return 0;
> -

Above lines are OK. ethdev layer should care about it. See
below.

>   	count = hns3_xstats_calc_num(dev);
> -	if ((int)n < count)
> +	if (xstats == NULL || (int)n < count)

but I dislike the change. xstats may be NULL if and only if n
is 0. If so, the extra check for xstats vs NULL is not required
here.

IMHO ethdev must guarantee it. I.e. rte_eth_xstats_get()
implementation in the first patch should be updated to
return -EINVAL if xstats is NULL, but n is not 0.

The major point for me here is to have just one natural key
parameter to make a decision if number of stats should be
returned or we really need to fill xstats in.
n is the parameter. If space is insufficient to return
xstats, return required space. If n is 0, xstats may be NULL,
since we provide no space for values. If we provide some
space for values (n is greater than 0), xstats must be not
NULL.

One more possible improvement in rte_eth_xstats_get()
implementation is to no provide out of xstats array
pointer if n is smaller or equal to number of basic
stats. Of course, space passed to xstats_get will be
zero anyway and xstats pointer should not be used, but
it is still safer to care about it explicitly. I.e. something like

2984 »···»···xcount = (*dev->dev_ops->xstats_get)(dev,
2985 »···»···»···»···     (n > count) ? xstats + count : NULL,
2986 »···»···»···»···     (n > count) ? n - count : 0);

If we guarantee that xstats is not NULL, if n is positive,
extra check for xstats vs NULL will not be required.

>   		return count;
>   
>   	count = 0;


  reply	other threads:[~2022-05-12  9:52 UTC|newest]

Thread overview: 55+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-04-16  1:07 [PATCH 0/3] bugfix for ethdev telemetry Chengwen Feng
2022-04-16  1:07 ` [PATCH 1/3] ethdev: fix telemetry xstats return null with some PMDs Chengwen Feng
2022-04-16  1:38   ` Stephen Hemminger
2022-04-21  6:49     ` Andrew Rybchenko
2022-04-24  3:44       ` fengchengwen
2022-04-25 10:16         ` Morten Brørup
2022-04-28 13:38           ` fengchengwen
2022-04-28 13:53             ` Morten Brørup
2022-04-16  1:07 ` [PATCH 2/3] ethdev: fix memory leak when telemetry xstats Chengwen Feng
2022-04-21  6:51   ` Andrew Rybchenko
2022-04-21  8:09   ` David Marchand
2022-04-21  9:03     ` Bruce Richardson
2022-04-22  8:14       ` David Marchand
2022-04-16  1:07 ` [PATCH 3/3] net/cnxk: fix telemetry possible null pointer access Chengwen Feng
2022-04-28 13:15 ` [PATCH v2 0/9] bugfix for ethdev telemetry Chengwen Feng
2022-04-28 13:15   ` [PATCH v2 1/9] ethdev: define retval when xstats is null of get xstats Chengwen Feng
2022-05-04 10:32     ` Andrew Rybchenko
2022-04-28 13:15   ` [PATCH v2 2/9] net/hns3: adjust " Chengwen Feng
2022-05-06  7:56     ` Min Hu (Connor)
2022-04-28 13:15   ` [PATCH v2 3/9] net/ipn3ke: " Chengwen Feng
2022-04-28 13:15   ` [PATCH v2 4/9] net/mvpp2: " Chengwen Feng
2022-04-28 13:15   ` [PATCH v2 5/9] net/axgbe: " Chengwen Feng
2022-04-28 13:15   ` [PATCH v2 6/9] ethdev: fix memory leak when telemetry xstats Chengwen Feng
2022-04-28 13:15   ` [PATCH v2 7/9] ethdev: support auto-filled flag when telemetry stats Chengwen Feng
2022-04-28 13:15   ` [PATCH v2 8/9] ethdev: fix possible null pointer access Chengwen Feng
2022-04-28 13:16   ` [PATCH v2 9/9] net/cnxk: fix telemetry " Chengwen Feng
2022-05-05  8:02 ` [PATCH v3 00/10] bugfix for ethdev telemetry Chengwen Feng
2022-05-05  8:02   ` [PATCH v3 01/10] ethdev: define retval when xstats is null of get xstats Chengwen Feng
2022-05-05  8:02   ` [PATCH v3 02/10] ethdev: optimize xstats-get API's implementation Chengwen Feng
2022-05-12 10:31     ` Andrew Rybchenko
2022-05-05  8:02   ` [PATCH v3 03/10] net/hns3: adjust retval when xstats is null of get xstats Chengwen Feng
2022-05-12  9:51     ` Andrew Rybchenko [this message]
2022-05-05  8:02   ` [PATCH v3 04/10] net/ipn3ke: " Chengwen Feng
2022-05-05  8:02   ` [PATCH v3 05/10] net/mvpp2: " Chengwen Feng
2022-05-05  8:02   ` [PATCH v3 06/10] net/axgbe: " Chengwen Feng
2022-05-12 10:00     ` Andrew Rybchenko
2022-05-05  8:02   ` [PATCH v3 07/10] ethdev: fix memory leak when telemetry xstats Chengwen Feng
2022-05-05  8:02   ` [PATCH v3 08/10] ethdev: support auto-filled flag when telemetry stats Chengwen Feng
2022-05-12 10:26     ` Andrew Rybchenko
2022-05-05  8:02   ` [PATCH v3 09/10] ethdev: fix possible null pointer access Chengwen Feng
2022-05-12 10:29     ` Andrew Rybchenko
2022-05-05  8:02   ` [PATCH v3 10/10] net/cnxk: fix telemetry " Chengwen Feng
2022-05-12 10:30     ` Andrew Rybchenko
2022-05-13  2:53 ` [PATCH v4 0/9] bugfix for ethdev telemetry Chengwen Feng
2022-05-13  2:53   ` [PATCH v4 1/9] ethdev: specify return value of xstats-get API Chengwen Feng
2022-05-13  2:53   ` [PATCH v4 2/9] ethdev: optimize xstats-get API's implementation Chengwen Feng
2022-05-13  2:53   ` [PATCH v4 3/9] net/hns3: adjust return value of xstats-get ops Chengwen Feng
2022-05-13  2:53   ` [PATCH v4 4/9] net/ipn3ke: " Chengwen Feng
2022-05-13  2:53   ` [PATCH v4 5/9] net/mvpp2: " Chengwen Feng
2022-05-13  2:53   ` [PATCH v4 6/9] net/axgbe: " Chengwen Feng
2022-05-13  2:53   ` [PATCH v4 7/9] ethdev: fix memory leak when telemetry xstats Chengwen Feng
2022-05-13  2:53   ` [PATCH v4 8/9] ethdev: fix possible null pointer access Chengwen Feng
2022-05-13  2:53   ` [PATCH v4 9/9] net/cnxk: fix telemetry " Chengwen Feng
2022-05-16  8:43   ` [PATCH v4 0/9] bugfix for ethdev telemetry Morten Brørup
2022-05-20 14:50     ` Andrew Rybchenko

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=c3a6d0a1-a44c-fc19-18ab-3d286bfe620c@oktetlabs.ru \
    --to=andrew.rybchenko@oktetlabs.ru \
    --cc=dev@dpdk.org \
    --cc=fengchengwen@huawei.com \
    --cc=ferruh.yigit@xilinx.com \
    --cc=kirankumark@marvell.com \
    --cc=mb@smartsharesystems.com \
    --cc=ndabilpuram@marvell.com \
    --cc=skori@marvell.com \
    --cc=skoteshwar@marvell.com \
    --cc=stephen@networkplumber.org \
    --cc=thomas@monjalon.net \
    /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

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror http://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ http://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git