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

["Van Haaren, Harry" <harry.van.haaren@intel.com>]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> Sent: Wednesday, October 18, 2017 10:49 PM
> To: Ivan Malov <Ivan.Malov@oktetlabs.ru>; Daly, Lee <lee.daly@intel.com>;
> thomas@monjalon.net
> Cc: dev@dpdk.org; Kozak, KubaX <kubax.kozak@intel.com>
> Subject: Re: [dpdk-dev] [PATCH v2] ethdev: fix xstats retrieve by id API
> 
> 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.


Lets not remove the _by_id() opts, because that removes the possibility for PMDs to optimize the get_by_id() case.


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


A quick test of this patch allows proc-info to work again to dump xstats of a primary, eg testpmd. Run testpmd first and then proc-info:

testpmd: ./testpmd -- -i
proc-info: ./dpdk-procinfo  -- --xstats

Before patch:
Cannot get xstat names
Cannot get xstat names

After patch:
<correct xstats output>

I suggest we use Ferruh's fix, and apply ASAP unless @Ivan has a better solution/suggestion?



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