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 74730A0032; Thu, 12 May 2022 11:52:05 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 83D3C42827; Thu, 12 May 2022 11:52:01 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id DE11240E64 for ; Thu, 12 May 2022 11:51:59 +0200 (CEST) Received: from [192.168.38.17] (aros.oktetlabs.ru [192.168.38.17]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 5EA4A82; Thu, 12 May 2022 12:51:59 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 5EA4A82 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1652349119; bh=mA/dCIPoiuITv9b+aT6zYQvLniBVSYP7XwGF6qcBHyA=; h=Date:Subject:To:Cc:References:From:In-Reply-To:From; b=oGNjMk0aBlGrTvACfzp6TBR919Q2H4NFNwLBoCRvm8I5t5D7okvLSYkxLYfh41rTk viis6NBmZliJAzQpUDt8sldAON9N8HEG9DVjyd+jEPmd245UuVWy7KyLMy9fbLIGAW GKz2jbKIsBaCriF5g85B9MvNxDQLIVWg3wE60f+4= Message-ID: Date: Thu, 12 May 2022 12:51:58 +0300 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.8.0 Subject: Re: [PATCH v3 03/10] net/hns3: adjust retval when xstats is null of get xstats Content-Language: en-US To: Chengwen Feng , 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 References: <20220416010747.40714-1-fengchengwen@huawei.com> <20220505080233.12737-1-fengchengwen@huawei.com> <20220505080233.12737-4-fengchengwen@huawei.com> From: Andrew Rybchenko Organization: OKTET Labs In-Reply-To: <20220505080233.12737-4-fengchengwen@huawei.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 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 > --- > 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;