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' <ferruh.yi...@intel.com> >> Cc: dev@dpdk.org; Hemant Agrawal <hemant.agra...@nxp.com> >> Subject: RE: [PATCH v4 41/41] net/dpaa: support for extended statistics >> >> Hi Ferruh, >> >>> -----Original Message----- >>> From: Ferruh Yigit [mailto:ferruh.yi...@intel.com] >>> Sent: Thursday, September 28, 2017 5:07 AM >>> To: Shreyansh Jain <shreyansh.j...@nxp.com> >>> Cc: dev@dpdk.org; Hemant Agrawal <hemant.agra...@nxp.com> >>> 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 <hemant.agra...@nxp.com> >>>>>>> >>>>>>> Signed-off-by: Hemant Agrawal <hemant.agra...@nxp.com> >>>>>> >>>>>> <...> >>>>>> >>>>>>> +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 >> >
