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 10:26:38 +0100 Message-ID: <58c5beca-4b70-83d0-5528-469de521005e@intel.com> 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 To: Shreyansh Jain Return-path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by dpdk.org (Postfix) with ESMTP id 2F575F94 for ; Thu, 28 Sep 2017 11:26:40 +0200 (CEST) In-Reply-To: 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/28/2017 3:52 AM, Shreyansh Jain wrote: >> -----Original Message----- >> From: Shreyansh Jain >> Sent: Thursday, September 28, 2017 7:59 AM >> To: 'Ferruh Yigit' >> Cc: dev@dpdk.org; Hemant Agrawal >> Subject: RE: [PATCH v4 41/41] net/dpaa: support for extended statistics >> >> Hi Ferruh, >> >>> -----Original Message----- >>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com] >>> Sent: Thursday, September 28, 2017 5:07 AM >>> To: Shreyansh Jain >>> Cc: dev@dpdk.org; Hemant Agrawal >>> Subject: Re: [PATCH v4 41/41] net/dpaa: support for extended statistics >>> >>> 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. >> >> Makes sense. I missed this and kept looking for implementations. >> I will at least fix dpaa code. > > On a second though: there might be another issue. > Application calls rte_eth_xstats_get_names and finds that 'N' xstats exist. > Thereafter, in a call to rte_eth_xstats_get, xstats==NULL but n=N, so the API would return: > > if (n < count + xcount || xstats == NULL) > return count + xcount; > > 'count' is size of generic stats. If drivers->xstats_get were to return xcount='N', the application would think that it has got a positive response. > See the doxygen page [3] - it states: > > -- > Returns: > * A positive value lower or equal to size: success. > The return value is the number of entries filled in the > stats table > -- > > There might be a case where the generic stats are exactly equal to xstats size and application would attempt to access the array. > > I am not even sure why the xstats_get is returning (count + xcount) when the API definition doesn't say that generic+xstat is returned. > Am I missing something? Even for rte_eth_xstats_get_names(), returned N is generic + xstat. dev_ops->xstats_get() manages xstat only, rte_eth_xstats_xxx() on top of them manages generic + xstat, this seems how it is designed. for rte_eth_xstats_get(), I guess there is an assumption that when app provides xstats == NULL, n also should be 0. Perhaps this should be implemented into API. > > [3] http://dpdk.org/doc/api/rte__ethdev_8h.html#adad5c65f659487db1fefba7d7d902973 > >> >>> >>> 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) >> >> Probably this info should be in Doxygen APIs [2]. >> >> [2] >> http://dpdk.org/doc/api/rte__ethdev_8h.html#adad5c65f659487db1fefba7d7d902973 >> >