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

[Shreyansh Jain]

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.

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