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 38E2DA0C50; Sat, 24 Jul 2021 14:06:42 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id A8B9E40DDA; Sat, 24 Jul 2021 14:06:41 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id A139F4067A; Sat, 24 Jul 2021 14:06:40 +0200 (CEST) Received: from [192.168.100.116] (unknown [37.139.99.76]) (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 8CD357F50A; Sat, 24 Jul 2021 15:06:39 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 8CD357F50A DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1627128399; bh=geMUMq35xRVe+ip91bjwkNI693Oy5OLnPlUNDeeFMzA=; h=From:Subject:To:Cc:References:Date:In-Reply-To; b=VnmG1d5B8vG7+QFwqPygSSe/mNw+dlWsldbuk1WkiRRf/9ijgh3ZjUQfOLKgH0rz1 J/t21h3InP6Vu6qdS9f5yCw8EKuTtnk7GNuz8aiQIKjg/tTjWNhDbO8788WIl+Bxz9 8YeefyWW3nFS2ZWYLPlzzZ4t/WE5EONOp7SlwWBs= From: Andrew Rybchenko To: Ferruh Yigit , dev@dpdk.org Cc: Ivan Ilchenko , stable@dpdk.org, Andy Moreton , Thomas Monjalon , Kuba Kozak References: <20210604144225.287678-1-andrew.rybchenko@oktetlabs.ru> <20210604144225.287678-4-andrew.rybchenko@oktetlabs.ru> <0a2b1c55-bcf0-de80-0baa-eb0abb97f37d@intel.com> Message-ID: <8aa2279f-ecec-447e-d80d-a360ab486ac4@oktetlabs.ru> Date: Sat, 24 Jul 2021 15:06:37 +0300 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.12.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [PATCH 03/11] ethdev: fix docs of functions 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 7/23/21 5:19 PM, Ferruh Yigit wrote: > On 7/22/2021 10:12 AM, Andrew Rybchenko wrote: >> On 7/20/21 7:25 PM, Ferruh Yigit wrote: >>> On 6/4/2021 3:42 PM, Andrew Rybchenko wrote: >>>> From: Ivan Ilchenko >>>> >>>> Document valid combinations of input arguments in accordance with >>>> current implementation in ethdev. >>>> >>>> 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/rte_ethdev.h | 23 ++++++++++++++--------- >>>>   1 file changed, 14 insertions(+), 9 deletions(-) >>>> >>>> diff --git a/lib/ethdev/rte_ethdev.h b/lib/ethdev/rte_ethdev.h >>>> index faf3bd901d..1f63118544 100644 >>>> --- a/lib/ethdev/rte_ethdev.h >>>> +++ b/lib/ethdev/rte_ethdev.h >>>> @@ -2873,12 +2873,15 @@ int rte_eth_xstats_get(uint16_t port_id, struct >>>> rte_eth_xstat *xstats, >>>>    *   The port identifier of the Ethernet device. >>>>    * @param xstats_names >>>>    *   An rte_eth_xstat_name array of at least *size* elements to >>>> - *   be filled. If set to NULL, the function returns the required number >>>> - *   of elements. >>>> + *   be filled. Must not be NULL if @p ids are specified (not NULL). >>> >>> Removed part is also valid. If both 'ids' & 'xstats_names' are NULL, API returns >>> number of all elements. >> >> Yes, but it is an excessive information. The trigger to return number >> of all elements is 'ids == NULL'. Here we are talking about >> 'xstats_names' parameter. If the parameter is NULL, but ids is not >> null, it does not trigger number of all elements return. It is an >> invalid input parameters. That's what a new description says. >> > > ids xstats_names > a) NULL NULL valid, return _number_ of all elements > b) NULL !NULL valid, return all elements (if size big enough) > c) !NULL NULL invalid > > Above a) is a valid option, the previous comment tries to describe it via "If > set to NULL, the function returns the required number of elements." and that > part is removed. > Since it is valid option I think we should keep it documented, location or > wording is up to you. OK, I see. IMHO, it looks better if it is mentioned in ids parameter description. See v4. >>> >>> Addition part looks good. >>> >>>>    * @param ids >>>> - *   IDs array given by app to retrieve specific statistics >>>> + *   IDs array given by app to retrieve specific statistics. May be NULL >>>> + *   to retrieve all available statistics. >>> >>> ack >>> >>>>    * @param size >>>> - *   The size of the xstats_names array (number of elements). >>>> + *   If @p ids is not NULL, number of elements in the array with requested IDs >>>> + *   and number of elements in @p xstats_names to put names in. If @p ids is >>>> + *   NULL, number of elements in @p xstats_names to put all available >>>> statistics >>>> + *   names in. >>> >>> ack >>> >>>>    * @return >>>>    *   - A positive value lower or equal to size: success. The return value >>>>    *     is the number of entries filled in the stats table. >>>> @@ -2886,7 +2889,7 @@ int rte_eth_xstats_get(uint16_t port_id, struct >>>> rte_eth_xstat *xstats, >>>>    *     is too small. The return value corresponds to the size that should >>>>    *     be given to succeed. The entries in the table are not valid and >>>>    *     shall not be used by the caller. >>>> - *   - A negative value on error (invalid port id). >>>> + *   - A negative value on error. >>> >>> ack >>> >>> >>> The 'eth_dev_get_xstats_count()' API is flexible but it makes API unnecessarily >>> complex, not for this patch but for future perhaps we can update the API and it >>> can return error if either 'ids' or 'xstats_names' is NULL. Remove support to >>> get all elements or getting number of elements support, these already supported >>> by non _id version of API. >> >> I'm not sure that it is a right direction. The support allows >> application to use less number of functions and depend on less >> number of function prototypes. >> > > True, but makes API more complex, and creates multiple way to do same thing (get > number of all entries), trade off. Yes. >>> And as a note for future, if we ever consider updating these _by_id APIs, we can >>> consider making the parameter order same for both, currently it is: >>> "rte_eth_xstats_get_names_by_id(port_id, values, size, ids)" >>> "      rte_eth_xstats_get_by_id(port_id, ids, values, size)" >> >> +1, current difference is terribly bad >> >>>>    */ >>>>   int >>>>   rte_eth_xstats_get_names_by_id(uint16_t port_id, >>>> @@ -2900,13 +2903,15 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id, >>>>    *   The port identifier of the Ethernet device. >>>>    * @param ids >>>>    *   A pointer to an ids array passed by application. This tells which >>>> - *   statistics values function should retrieve. This parameter >>>> - *   can be set to NULL if size is 0. In this case function will retrieve >>>> + *   statistics values function should retrieve. May be NULL to retrieve >>>>    *   all available statistics. >>> >>> Update is good. But what do you think to make it exact same in the both APIs >>> ('rte_eth_xstats_get_names_by_id()' & 'rte_eth_xstats_get_by_id()')? Since it is >>> used for same purpose and exact same way in both APIs, no need to have slightly >>> different description. >> >> I agree. I'll fix in v2. >> >>>>    * @param values >>>>    *   A pointer to a table to be filled with device statistics values. >>>> + *   Must not be NULL if ids are specified (not NULL). >>> >>> Same comment on making description similar in both APIs. >> >> OK >> >>> Also both 'ids' & 'values' being NULL returns number of all elements should be >>> addressed. >> >> I think it is excessibe. It is sufficient to say so for ids==NULL which >> is a trigger to get all elements. >> > > But these two are different things, > one fills values with stats and return number of filled elements > other doesn't fill any value, but only return number of all elements I get it. See v4. Hopefully I'll address it in ids parameter description. >>>>    * @param size >>>> - *   The size of the ids array (number of elements). >>>> + *   If @p ids is not NULL, number of elements in the array with requested IDs >>>> + *   and number of elements in values to put statistics in. If @p ids is NULL, >>>> + *   number of elements in values to put all available statistics in. >>> >>> ack >>> >>>>    * @return >>>>    *   - A positive value lower or equal to size: success. The return value >>>>    *     is the number of entries filled in the stats table. >>>> @@ -2914,7 +2919,7 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id, >>>>    *     is too small. The return value corresponds to the size that should >>>>    *     be given to succeed. The entries in the table are not valid and >>>>    *     shall not be used by the caller. >>>> - *   - A negative value on error (invalid port id). >>>> + *   - A negative value on error. >>> >>> ack >>> >>>>    */ >>>>   int rte_eth_xstats_get_by_id(uint16_t port_id, const uint64_t *ids, >>>>                    uint64_t *values, unsigned int size); >>>> >>