Re: [dpdk-dev] [PATCH v2] ethdev: fix xstats retrieve by id API

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 10/18/2017 4:10 AM, Ivan Malov wrote:
> On 10/12/2017 2:31 PM, Lee Daly wrote:
>> From: Lee <lee.daly@intel.com>
>>
>> Fix xstats functions, rte_eth_xstats_get_names_by_id()
>> and rte_eth_xstats_get_by_id(), in current implementation
>> ethdev level reads all xstat values and filters out
>> the ones requested by the application. This behavior doesn't
>> benefit from PMD ops and doesn't provide the benefit the
>> API was created in the first place for. APIs are also unnecessarily
>> complicated. Both APIs have different returns for the same params.
>>
>> In this fix, instead of reading all the stats and finding the
>> requested value, drivers can provide ops to get selected xstats.
>> API no longer crashes with certain params,
>>
>> rte_eth_get_by_id returned seg fault with
>> "ids = NULL && values != NULL && n<max"
>> rte_eth_get_names_by_id returned seg fault with
>> "ids = NULL && values != NULL && n=0"
>> These now return max number of stats available, matching the other API.
>>
>> rte_eth_get_by_id returned seg fault with
>> "ids != NULL && values = NULL && n<max"
>> This now returns -22,(EINVAL).
>>
>> Standardized variable/parameter names between the 2 APIs.
>>
>> Overall code complexity reduced.
>>
>> Fixes: 79c913a42f0e ("ethdev: retrieve xstats by ID")
>> Cc: kubax.kozak@intel.com
>>
>> Signed-off-by: Lee Daly <lee.daly@intel.com>
>> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> I have a serious concern regarding the patch. There is a common scenario
> of rte_eth_xstats_get_names_by_id() usage, and the patch breaks it.
> Typically, the function is called with 'ids = NULL' and 'xstats_names = NULL'
> in order to get the number of figures. Then the function is called one more
> time with appropriate storage for 'xstats_names'. According to the patch,
> on the former step get_xstats_count() is called to count the figures available.
> The resulting number counts for PMD statistics + RTE_NB_STATS + some amount of
> per-queue figures. However, on the latter step the following may take place:
> 
>> +	if (dev->dev_ops->xstats_get_names_by_id != NULL)
>> +		return (*dev->dev_ops->xstats_get_names_by_id)(
>> +				dev, xstats_names, ids, size);
> 
> This obviously means that in the case when 'xstats_get_names_by_id' is present,
> it will be called directly, and RTE statistics will not be filled in the storage
> before the PMD figures. Accordingly, the value returned by the function in this
> case will count for PMD figures only (i.e. will be less than the value obtained
> on the first step). This is an obvious malfunction, and it would be desirable
> to provide a fix for that. I hope for your understanding.

Hi Ivan,

You are right, this is broken.

But the problem is, ethdev API works with ids which are "basic + xstat" and PMD dev_ops
works with ids which are "xstat". This mismatch ends up xstats_get_names_by_id() dev_ops
being called to get all values, and filtering done in ethdev API, as done in previous
implementation.

If we use xstats_get_names_by_id() to get all values, why we have it at all, there is
already a dev_ops to get all values.

I believe we have two options,

1- Remove this by _by_id() dev_ops, but keep ethdev APIs, and APIs does the filtering of
the ids. This may be missing if any PMD has an optimal way of getting subset of ids, this
is the main reason of the devops.

2- Convert ids for PMD usage, what do you think about following fix:
(same thing for rte_eth_xstats_get_by_id() also of course)

  diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
  index 0b1e92894..1145539ee 100644
  --- a/lib/librte_ether/rte_ethdev.c
  +++ b/lib/librte_ether/rte_ethdev.c
  @@ -1631,6 +1631,7 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
   {
          struct rte_eth_xstat_name *xstats_names_copy;
          unsigned int expected_entries;
  +       unsigned int all_ids_from_pmd = 1;
          struct rte_eth_dev *dev;
          unsigned int i;

  @@ -1649,9 +1650,28 @@ rte_eth_xstats_get_names_by_id(uint16_t port_id,
          if (ids && !xstats_names)
                  return -EINVAL;

  -       if (dev->dev_ops->xstats_get_names_by_id != NULL)
  -               return (*dev->dev_ops->xstats_get_names_by_id)(
  -                               dev, xstats_names, ids, size);
  +       if (ids && dev->dev_ops->xstats_get_names_by_id != NULL && size > 0) {
  +               uint64_t ids_copy[size];
  +               unsigned int pmd_count;
  +               unsigned int count;
  +
  +               pmd_count = (*dev->dev_ops->xstats_get_names_by_id)(dev, NULL,
  +                                       NULL, 0);
  +               count = expected_entries - pmd_count;
  +
  +               for (i = 0; i < size; i++) {
  +                       if (ids[i] < count) {
  +                               all_ids_from_pmd = 0;
  +                               break;
  +                       }
  +
  +                       ids_copy[i] = ids[i] - count;
  +               }
  +
  +               if (all_ids_from_pmd)
  +                       return (*dev->dev_ops->xstats_get_names_by_id)(dev,
  +                                       xstats_names, ids_copy, size);
  +       }

          /* Retrieve all stats */
          if (!ids) {