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;
next prev parent 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