> -----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?
[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
>
