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

Ferruh Yigit ferruh.yigit at intel.com
Thu Sep 28 11:26:38 CEST 2017


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

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



More information about the dev mailing list