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 AD56CA0C43; Thu, 30 Sep 2021 18:01:54 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 54384410E5; Thu, 30 Sep 2021 18:01:54 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id E981C40DDA; Thu, 30 Sep 2021 18:01:52 +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 (2048 bits) server-digest SHA256) (No client certificate requested) by shelob.oktetlabs.ru (Postfix) with ESMTPSA id 60EF07F508; Thu, 30 Sep 2021 19:01:52 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 60EF07F508 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1633017712; bh=Wt105vvDsEAi+oPq4hMO4Vv6D6HDNslmOJghgKoqiz4=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=MLp2GcKLcICYFhg52d5EWaK6m+Ms8Y0y6fUj5VVmKfBLg0C2waA53UmBtDggoiKlO F981s662fS1j9+X7j8WG1+ZggT+9dzwsBhIzoJq1BAhY41Qo6STsBmIA9f0Q2ylmmC AYG00+0cL8PYyWLejt81GshK7rjKsRJ18XnGrJx8= To: Ferruh Yigit , Thomas Monjalon Cc: dev@dpdk.org, Olivier Matz , Ivan Ilchenko , stable@dpdk.org, Andy Moreton , Kuba Kozak References: <20210604144225.287678-1-andrew.rybchenko@oktetlabs.ru> <20210928120533.834334-1-andrew.rybchenko@oktetlabs.ru> <20210928120533.834334-2-andrew.rybchenko@oktetlabs.ru> <5e9f39eb-d859-0c4a-8eb7-1c9f494364a0@intel.com> <5d9c9565-b3cf-8e29-309f-1aa3a2e91491@oktetlabs.ru> From: Andrew Rybchenko Organization: OKTET Labs Message-ID: Date: Thu, 30 Sep 2021 19:01:52 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [dpdk-stable] [PATCH v5 2/2] ethdev: fix docs of drivers callbacks getting xstats by IDs 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 Sender: "dev" On 9/30/21 6:30 PM, Ferruh Yigit wrote: > On 9/30/2021 3:01 PM, Andrew Rybchenko wrote: >> On 9/30/21 3:08 PM, Ferruh Yigit wrote: >>> On 9/29/2021 12:54 PM, Andrew Rybchenko wrote: >>>> On 9/29/21 11:44 AM, Ferruh Yigit wrote: >>>>> On 9/28/2021 5:53 PM, Andrew Rybchenko wrote: >>>>>> On 9/28/21 7:50 PM, Ferruh Yigit wrote: >>>>>>> On 9/28/2021 1:05 PM, Andrew Rybchenko wrote: >>>>>>>> From: Ivan Ilchenko >>>>>>>> >>>>>>>> Update xstats by IDs callbacks documentation in accordance with >>>>>>>> ethdev usage of these callbacks. Document valid combinations of >>>>>>>> input arguments to make driver implementation simpler. >>>>>>>> >>>>>>>> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID") >>>>>>>> Cc: stable@dpdk.org >>>>>>>> >>>>>>>> Signed-off-by: Ivan Ilchenko >>>>>>>> Signed-off-by: Andrew Rybchenko >>>>>>>> Reviewed-by: Andy Moreton >>>>>>>> --- >>>>>>>> lib/ethdev/ethdev_driver.h | 42 ++++++++++++++++++++++++++++++++++++-- >>>>>>>> 1 file changed, 40 insertions(+), 2 deletions(-) >>>>>>>> >>>>>>>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h >>>>>>>> index 40e474aa7e..c89eefcc42 100644 >>>>>>>> --- a/lib/ethdev/ethdev_driver.h >>>>>>>> +++ b/lib/ethdev/ethdev_driver.h >>>>>>>> @@ -187,11 +187,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev, >>>>>>>> struct rte_eth_xstat *stats, unsigned int n); >>>>>>>> /**< @internal Get extended stats of an Ethernet device. */ >>>>>>>> >>>>>>>> +/** >>>>>>>> + * @internal >>>>>>>> + * Get extended stats of an Ethernet device. >>>>>>>> + * >>>>>>>> + * @param dev >>>>>>>> + * ethdev handle of port. >>>>>>>> + * @param ids >>>>>>>> + * IDs array to retrieve specific statistics. Must not be NULL. >>>>>>>> + * @param values >>>>>>>> + * A pointer to a table to be filled with device statistics values. >>>>>>>> + * Must not be NULL. >>>>>>>> + * @param n >>>>>>>> + * Element count in @p ids and @p values. >>>>>>>> + * >>>>>>>> + * @return >>>>>>>> + * - A number of filled in stats. >>>>>>>> + * - A negative value on error. >>>>>>>> + */ >>>>>>>> typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev, >>>>>>>> const uint64_t *ids, >>>>>>>> uint64_t *values, >>>>>>>> unsigned int n); >>>>>>>> -/**< @internal Get extended stats of an Ethernet device. */ >>>>>>>> >>>>>>>> /** >>>>>>>> * @internal >>>>>>>> @@ -218,10 +235,31 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev, >>>>>>>> struct rte_eth_xstat_name *xstats_names, unsigned int size); >>>>>>>> /**< @internal Get names of extended stats of an Ethernet device. */ >>>>>>>> >>>>>>>> +/** >>>>>>>> + * @internal >>>>>>>> + * Get names of extended stats of an Ethernet device. >>>>>>>> + * For name count, set @p xstats_names and @p ids to NULL. >>>>>>> >>>>>>> Why limiting this behavior to 'xstats_get_names_by_id'? >>>>>>> >>>>>>> Internally 'xstats_get_names_by_id' is used to get the count, but I think this >>>>>>> is not intentionally selected, just one of the xstats_*_by_id dev_ops used. >>>>>>> >>>>>>> I think it is confusing to have this support for one of the '_by_id' dev_ops but >>>>>>> not for other. Why not require both to support returning 'count'? >>>>>> >>>>>> Simply because it is dead code. There is no point to require >>>>>> from driver to have dead code. >>>>>> >>>>> >>>>> Let me step back a little, both ethdev APIs can be used to return xstats count >>>>> by providing 'values/names' & 'ids' pointers as NULL and 'size' as 0: >>>>> 'rte_eth_xstats_get_names_by_id()' >>>>> 'rte_eth_xstats_get_by_id()' >>>>> >>>>> But internally both APIs use 'xstats_get_names_by_id' dev_ops to get the count, >>>>> as said above I believe this selection is done unintentionally. >>>>> >>>>> >>>>> I am for below two options: >>>>> >>>>> a) Internally use 'xstats_get_names' || 'xstats_get' dev_ops to get the xstats >>>>> count, and doesn't support getting xstats count for both '_by_id' dev_ops, this >>>>> simplifies driver code. As far as I remember I suggested this before, still I >>>>> prefer this one. >>>>> >>>>> b) If we will support getting xstats count from '_by_id' dev_ops, I think both >>>>> should support it, to not make it more complex to figure out which one support >>>>> what. As sample both 'xstats_get_names' and 'xstats_get' supports getting xstats >>>>> count, not just one. >>>>> >>>> >>>> In (b) do you suggest to change ethdev to use xstats_get_by_id >>>> driver op if users asks for a number of xstats using >>>> rte_eth_xstats_get_by_id()? It will complicate ethdev and >>>> will complicate drivers. Just for the symmetry? >>>> >>> >>> Not just for symmetry, but simplify the logic. Both dev_ops are very similar and >> >> I'm sorry, but could you point of which logic you'd >> like to simply. Less requirements on driver ops >> means less code required inside. >> >>> they are having different behavior with same parameters is confusing. >> >> Ah, logic from PMD maintainer point of view. Does it >> really worse to require extra code inside because of it? >> >>> If you want to simplify the drivers why not go with option (a)? Option (a) also >>> doesn't change external API, and has only minor change in the ethdev layer. >> >> Is two extra patches in v6 (discussed below) a step >> towards (a)? >> > > I am not sure of it, for (a) ethdev layer will handle getting all stats or > getting count using 'xstats_get_names' || 'xstats_get', and '*_by_id()' dev_ops > will become simpler since they won't accept NULL 'ids' & 'values'. Got it. Will fix it in v7. I'll remove requirement in a dedicated patch, but may be it should be squashed with this one on apply. > Also when dev_ops merged, it forces PMDs to implement the getting by ids, but > now it is covered by ethdev layer for the PMDs that doesn't implement '_by_id()'. Thanks, good catch. I'll handle it v7 as well.