[dpdk-dev] [PATCH v4 41/41] net/dpaa: support for extended statistics

[Shreyansh Jain]

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