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.
> > 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 > > > >> > >>> > >>>> + > >>>> + 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; > >>>> +} > >>> > >>> <...> > >>> > >> > >> > >
