From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ferruh Yigit Subject: Re: [PATCH v4 41/41] net/dpaa: support for extended statistics Date: Thu, 28 Sep 2017 00:37:13 +0100 Message-ID: References: <20170823141213.25476-1-shreyansh.jain@nxp.com> <20170909112132.13936-1-shreyansh.jain@nxp.com> <20170909112132.13936-42-shreyansh.jain@nxp.com> <3332a9da-5e28-260a-68fa-ab665f907403@intel.com> <58ffe1c0-308a-f730-a6d1-9bcf8ddb3d57@nxp.com> <92166552-9ab5-db9a-d7f5-8fb2f4a8a3e7@nxp.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Cc: dev@dpdk.org, hemant.agrawal@nxp.com To: Shreyansh Jain Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id 1B83A2BA9 for ; Thu, 28 Sep 2017 01:37:18 +0200 (CEST) In-Reply-To: <92166552-9ab5-db9a-d7f5-8fb2f4a8a3e7@nxp.com> Content-Language: en-US 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/27/2017 9:26 AM, Shreyansh Jain wrote: > On Thursday 21 September 2017 06:56 PM, Shreyansh Jain wrote: >> On Monday 18 September 2017 08:27 PM, Ferruh Yigit wrote: >>> On 9/9/2017 12:21 PM, Shreyansh Jain wrote: >>>> From: Hemant Agrawal >>>> >>>> Signed-off-by: Hemant Agrawal >>> >>> <...> >>> >>>> +static int >>>> +dpaa_dev_xstats_get(struct rte_eth_dev *dev, struct rte_eth_xstat >>>> *xstats, >>>> +            unsigned int n) >>>> +{ >>>> +    struct dpaa_if *dpaa_intf = dev->data->dev_private; >>>> +    unsigned int i = 0, num = RTE_DIM(dpaa_xstats_strings); >>>> +    uint64_t values[sizeof(struct dpaa_if_stats) / 8]; >>>> + >>>> +    if (xstats == NULL) >>>> +        return 0; >>> >>> This is a little not clear from API definition, but I guess when xstats >>> is NULL, it should return num of available stats, "num" for this case. I >>> guess there are PMDs implements both, can you please double check? >> >> Ok. I will check again. > > I checked a number of other ethev implementations. Some like i40e/e1000 > also return 0 when xstats is NULL. Others, like bnx2x and qede don't > handle this situation. > All return "num" when passed argument is larger than number of elements > in the table. > > Though, I think the logic that get_xstats should return its size (num) > when passed with NULL, looks good to me. > How does one standardize such semantics for existing APIs? Thanks for checking, I guess first we should clarify the API and the expected behavior [1] and later update required PMDs. So for now I think PMD is OK as it is. [1] I double checked the rte_eth_xstats_get(). It does: If xstats == NULL xcount = dev_ops->xstats_get(dev, NULL, 0); return count + xcount; Intention looks like to returning number of available stats, otherwise returning "count + 0" will be useless. So it looks like expectation from eth_xstats_get_t for that case is returning xstats size, but this not clear and not documented in API comment. > > (I can add this info to the API document that you created - but only > once we know if others will agree to change) > >> >>> >>>> + >>>> +    if (n < num) >>>> +        return num; >>>> + >>>> +    fman_if_stats_get_all(dpaa_intf->fif, values, >>>> +                  sizeof(struct dpaa_if_stats) / 8); >>>> + >>>> +    for (i = 0; i < num; i++) { >>>> +        xstats[i].id = i; >>>> +        xstats[i].value = values[dpaa_xstats_strings[i].offset / 8]; >>>> +    } >>>> +    return i; >>>> +} >>> >>> <...> >>> >> >> >