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 4AA49A0C4E; Thu, 22 Jul 2021 11:12:08 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id BE6D24014D; Thu, 22 Jul 2021 11:12:07 +0200 (CEST) Received: from shelob.oktetlabs.ru (shelob.oktetlabs.ru [91.220.146.113]) by mails.dpdk.org (Postfix) with ESMTP id 2C50D40040; Thu, 22 Jul 2021 11:12:06 +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 7651D7F529; Thu, 22 Jul 2021 12:12:05 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 shelob.oktetlabs.ru 7651D7F529 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=oktetlabs.ru; s=default; t=1626945125; bh=/8fNN8NqGjWNGxO+IJY/dr8kJskvhlheeeqRFcyDtFQ=; h=Subject:To:Cc:References:From:Date:In-Reply-To; b=DkaRAuqB9vrJOcMF6PPxPIBxzhOxHckoyK+Ear4ozRMBE6bJW/0YcKls46dsW4nbF GomWck4V+PYMPfAHWj1TLnkgO/PXm2us+bogSGSwO5pUtKRdb/blAoYtBbIW4bbWk/ 6/cBN0yjrzYU9aAyyPw5U0Lild8LDA5Dr3nrtCg4= 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> From: Andrew Rybchenko Message-ID: Date: Thu, 22 Jul 2021 12:12:02 +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: <0a2b1c55-bcf0-de80-0baa-eb0abb97f37d@intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit 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/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. > > 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. > 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. >> * @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); >>