patches for DPDK stable branches
 help / color / mirror / Atom feed
From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
To: Ferruh Yigit <ferruh.yigit@intel.com>,
	Thomas Monjalon <thomas@monjalon.net>
Cc: dev@dpdk.org, Olivier Matz <olivier.matz@6wind.com>,
	Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>,
	stable@dpdk.org, Andy Moreton <amoreton@xilinx.com>,
	Kuba Kozak <kubax.kozak@intel.com>
Subject: Re: [dpdk-stable] [PATCH v5 2/2] ethdev: fix docs of drivers callbacks getting xstats by IDs
Date: Wed, 29 Sep 2021 14:54:09 +0300	[thread overview]
Message-ID: <c6bcce29-88bf-c433-5197-5bca8a0462e4@oktetlabs.ru> (raw)
In-Reply-To: <b42ebfb9-23cf-1751-d2af-df29f8f18392@intel.com>

On 9/29/21 11:44 AM, Ferruh Yigit wrote:
> On 9/28/2021 5:53 PM, Andrew Rybchenko wrote:
>> On 9/28/21 7:50 PM, Ferruh Yigit wrote:
>>> On 9/28/2021 1:05 PM, Andrew Rybchenko wrote:
>>>> From: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>>
>>>> Update xstats by IDs callbacks documentation in accordance with
>>>> ethdev usage of these callbacks. Document valid combinations of
>>>> input arguments to make driver implementation simpler.
>>>>
>>>> Fixes: 79c913a42f0 ("ethdev: retrieve xstats by ID")
>>>> Cc: stable@dpdk.org
>>>>
>>>> Signed-off-by: Ivan Ilchenko <ivan.ilchenko@oktetlabs.ru>
>>>> Signed-off-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>> Reviewed-by: Andy Moreton <amoreton@xilinx.com>
>>>> ---
>>>>  lib/ethdev/ethdev_driver.h | 42 ++++++++++++++++++++++++++++++++++++--
>>>>  1 file changed, 40 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/lib/ethdev/ethdev_driver.h b/lib/ethdev/ethdev_driver.h
>>>> index 40e474aa7e..c89eefcc42 100644
>>>> --- a/lib/ethdev/ethdev_driver.h
>>>> +++ b/lib/ethdev/ethdev_driver.h
>>>> @@ -187,11 +187,28 @@ typedef int (*eth_xstats_get_t)(struct rte_eth_dev *dev,
>>>>  	struct rte_eth_xstat *stats, unsigned int n);
>>>>  /**< @internal Get extended stats of an Ethernet device. */
>>>>  
>>>> +/**
>>>> + * @internal
>>>> + * Get extended stats of an Ethernet device.
>>>> + *
>>>> + * @param dev
>>>> + *   ethdev handle of port.
>>>> + * @param ids
>>>> + *   IDs array to retrieve specific statistics. Must not be NULL.
>>>> + * @param values
>>>> + *   A pointer to a table to be filled with device statistics values.
>>>> + *   Must not be NULL.
>>>> + * @param n
>>>> + *   Element count in @p ids and @p values.
>>>> + *
>>>> + * @return
>>>> + *   - A number of filled in stats.
>>>> + *   - A negative value on error.
>>>> + */
>>>>  typedef int (*eth_xstats_get_by_id_t)(struct rte_eth_dev *dev,
>>>>  				      const uint64_t *ids,
>>>>  				      uint64_t *values,
>>>>  				      unsigned int n);
>>>> -/**< @internal Get extended stats of an Ethernet device. */
>>>>  
>>>>  /**
>>>>   * @internal
>>>> @@ -218,10 +235,31 @@ typedef int (*eth_xstats_get_names_t)(struct rte_eth_dev *dev,
>>>>  	struct rte_eth_xstat_name *xstats_names, unsigned int size);
>>>>  /**< @internal Get names of extended stats of an Ethernet device. */
>>>>  
>>>> +/**
>>>> + * @internal
>>>> + * Get names of extended stats of an Ethernet device.
>>>> + * For name count, set @p xstats_names and @p ids to NULL.
>>>
>>> Why limiting this behavior to 'xstats_get_names_by_id'?
>>>
>>> Internally 'xstats_get_names_by_id' is used to get the count, but I think this
>>> is not intentionally selected, just one of the xstats_*_by_id dev_ops used.
>>>
>>> I think it is confusing to have this support for one of the '_by_id' dev_ops but
>>> not for other. Why not require both to support returning 'count'?
>>
>> Simply because it is dead code. There is no point to require
>> from driver to have dead code.
>>
> 
> Let me step back a little, both ethdev APIs can be used to return xstats count
> by providing 'values/names' & 'ids' pointers as NULL and 'size' as 0:
> 'rte_eth_xstats_get_names_by_id()'
> 'rte_eth_xstats_get_by_id()'
> 
> But internally both APIs use 'xstats_get_names_by_id' dev_ops to get the count,
> as said above I believe this selection is done unintentionally.
> 
> 
> I am for below two options:
> 
> a) Internally use 'xstats_get_names' || 'xstats_get' dev_ops to get the xstats
> count, and doesn't support getting xstats count for both '_by_id' dev_ops, this
> simplifies driver code. As far as I remember I suggested this before, still I
> prefer this one.
> 
> b) If we will support getting xstats count from '_by_id' dev_ops, I think both
> should support it, to not make it more complex to figure out which one support
> what. As sample both 'xstats_get_names' and 'xstats_get' supports getting xstats
> count, not just one.
> 

In (b) do you suggest to change ethdev to use xstats_get_by_id
driver op if users asks for a number of xstats using
rte_eth_xstats_get_by_id()? It will complicate ethdev and
will complicate drivers. Just for the symmetry?

The patch does not change external API, does not change etcdev
bahaviour. It just clarify requirements on driver ops to
allow to check less in all drivers and support less cases
in all drivers.

If we make a one more step back, frankly speaking I think we
have too many functions to retrieve statistics. I can
understand from ethdev API point of view taking API stability
into account etc. But why do we have so many complicated
driver callbacks?

First of all I'd like to do one more cleanup:
change eth_xstats_get_names_by_id_t prototype to
have ids before xstats_names.
I.e. eth_xstats_get_by_id_t(dev, ids, values, size)
eth_xstats_get_names_by_id_t(dev, ids, names, size)

Second, merge eth_xstats_get_names_t and eth_xstats_get_names_by_id_t
callbacks to keep
name of the first, but prototype from the second.
The reason is equal functionality if ids==NULL,
shorter name of the first and optional ids (i.e. no
reason to mention optional parameter in name).
Drivers which do not implement by_id_t,
but implement eth_xstats_get_names_t, will simply
return ENOTSUP if ids!=NULL.

The case of values ops is more complicated,
however since:

2834  * There is an assumption that 'xstat_names' and 'xstats' arrays
are matched
2835  * by array index:
2836  *  xstats_names[i].name => xstats[i].value
2837  *
2838  * And the array index is same with id field of 'struct rte_eth_xstat':
2839  *  xstats[i].id == i
2840  *
2841  * This assumption makes key-value pair matching less flexible but
simpler.

we can switch to eth_xstats_get_by_id_t only callback as
well and fill in stats->id equal to index on ethdev layer.
However, it will require extra buffer for
uint64_t *values and copying in the rte_eth_xstats_get()
implementation. So, I doubt in this case.

In fact, it is sad that we still do not move forward
in accordance with Thomas presentation made 2 years ago.

  reply	other threads:[~2021-09-29 11:54 UTC|newest]

Thread overview: 64+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <20210604144225.287678-1-andrew.rybchenko@oktetlabs.ru>
2021-06-04 14:42 ` [dpdk-stable] [PATCH 01/11] net/sfc: fix get xstats by ID callback to use MAC stats lock Andrew Rybchenko
2021-06-04 14:42 ` [dpdk-stable] [PATCH 02/11] net/sfc: fix reading adapter state without locking Andrew Rybchenko
2021-06-04 14:42 ` [dpdk-stable] [PATCH 03/11] ethdev: fix docs of functions getting xstats by IDs Andrew Rybchenko
2021-07-20 16:25   ` Ferruh Yigit
2021-07-22  9:12     ` Andrew Rybchenko
2021-07-23 14:19       ` Ferruh Yigit
2021-07-24 12:06         ` Andrew Rybchenko
2021-06-04 14:42 ` [dpdk-stable] [PATCH 04/11] ethdev: fix docs of drivers callbacks " Andrew Rybchenko
2021-07-20 16:51   ` Ferruh Yigit
2021-07-22  9:33     ` Andrew Rybchenko
2021-07-23 14:31       ` Ferruh Yigit
2021-07-23 18:47         ` Andrew Rybchenko
2021-06-04 14:42 ` [dpdk-stable] [PATCH 05/11] net/sfc: fix xstats by ID callbacks according to ethdev Andrew Rybchenko
2021-06-04 14:42 ` [dpdk-stable] [PATCH 06/11] net/sfc: fix accessing xstats by an unsorted list of IDs Andrew Rybchenko
2021-06-04 14:42 ` [dpdk-stable] [PATCH 07/11] net/sfc: fix MAC stats update to work for stopped device Andrew Rybchenko
     [not found] ` <20210722095433.1898589-1-andrew.rybchenko@oktetlabs.ru>
2021-07-22  9:54   ` [dpdk-stable] [PATCH v2 01/11] net/sfc: fix get xstats by ID callback to use MAC stats lock Andrew Rybchenko
2021-07-22  9:54   ` [dpdk-stable] [PATCH v2 02/11] net/sfc: fix reading adapter state without locking Andrew Rybchenko
2021-07-22  9:54   ` [dpdk-stable] [PATCH v2 03/11] ethdev: fix docs of functions getting xstats by IDs Andrew Rybchenko
2021-07-22  9:54   ` [dpdk-stable] [PATCH v2 04/11] ethdev: fix docs of drivers callbacks " Andrew Rybchenko
2021-07-22  9:54   ` [dpdk-stable] [PATCH v2 05/11] net/sfc: fix xstats by ID callbacks according to ethdev Andrew Rybchenko
2021-07-22  9:54   ` [dpdk-stable] [PATCH v2 06/11] net/sfc: fix accessing xstats by an unsorted list of IDs Andrew Rybchenko
2021-07-22  9:54   ` [dpdk-stable] [PATCH v2 07/11] net/sfc: fix MAC stats update to work for stopped device Andrew Rybchenko
     [not found] ` <20210723131515.2317168-1-andrew.rybchenko@oktetlabs.ru>
2021-07-23 13:15   ` [dpdk-stable] [PATCH v3 01/11] net/sfc: fix get xstats by ID callback to use MAC stats lock Andrew Rybchenko
2021-07-23 13:15   ` [dpdk-stable] [PATCH v3 02/11] net/sfc: fix reading adapter state without locking Andrew Rybchenko
2021-07-23 13:15   ` [dpdk-stable] [PATCH v3 03/11] ethdev: fix docs of functions getting xstats by IDs Andrew Rybchenko
2021-07-23 14:42     ` Ferruh Yigit
2021-07-24 12:07       ` Andrew Rybchenko
2021-07-23 13:15   ` [dpdk-stable] [PATCH v3 04/11] ethdev: fix docs of drivers callbacks " Andrew Rybchenko
2021-07-23 14:46     ` Ferruh Yigit
2021-07-23 13:15   ` [dpdk-stable] [PATCH v3 05/11] net/sfc: fix xstats by ID callbacks according to ethdev Andrew Rybchenko
2021-07-23 13:15   ` [dpdk-stable] [PATCH v3 06/11] net/sfc: fix accessing xstats by an unsorted list of IDs Andrew Rybchenko
2021-07-23 13:15   ` [dpdk-stable] [PATCH v3 07/11] net/sfc: fix MAC stats update to work for stopped device Andrew Rybchenko
2021-07-24 12:33 ` [dpdk-stable] [PATCH v4 1/2] ethdev: fix docs of functions getting xstats by IDs Andrew Rybchenko
2021-07-24 12:33   ` [dpdk-stable] [PATCH v4 2/2] ethdev: fix docs of drivers callbacks " Andrew Rybchenko
2021-07-26 10:13     ` [dpdk-stable] [dpdk-dev] " Olivier Matz
2021-09-28 12:04       ` Andrew Rybchenko
2021-07-26 10:13   ` [dpdk-stable] [dpdk-dev] [PATCH v4 1/2] ethdev: fix docs of functions " Olivier Matz
2021-09-28 12:01     ` Andrew Rybchenko
2021-09-28 12:05 ` [dpdk-stable] [PATCH v5 " Andrew Rybchenko
2021-09-28 12:05   ` [dpdk-stable] [PATCH v5 2/2] ethdev: fix docs of drivers callbacks " Andrew Rybchenko
2021-09-28 16:50     ` Ferruh Yigit
2021-09-28 16:53       ` Andrew Rybchenko
2021-09-29  8:44         ` Ferruh Yigit
2021-09-29 11:54           ` Andrew Rybchenko [this message]
2021-09-30 12:08             ` Ferruh Yigit
2021-09-30 14:01               ` Andrew Rybchenko
2021-09-30 15:30                 ` Ferruh Yigit
2021-09-30 16:01                   ` Andrew Rybchenko
2021-09-28 16:46   ` [dpdk-stable] [PATCH v5 1/2] ethdev: fix docs of functions " Ferruh Yigit
2021-09-30 14:04 ` [dpdk-stable] [PATCH v6 1/4] " Andrew Rybchenko
2021-09-30 14:04   ` [dpdk-stable] [PATCH v6 2/4] ethdev: fix docs of drivers callbacks " Andrew Rybchenko
     [not found] ` <20210930160156.961041-1-andrew.rybchenko@oktetlabs.ru>
2021-09-30 16:01   ` [dpdk-stable] [PATCH v7 1/5] ethdev: fix docs of functions " Andrew Rybchenko
2021-09-30 16:01   ` [dpdk-stable] [PATCH v7 2/4] ethdev: fix docs of drivers callbacks " Andrew Rybchenko
2021-09-30 16:01   ` [dpdk-stable] [PATCH v7 3/5] " Andrew Rybchenko
2021-09-30 16:05 ` [dpdk-stable] [PATCH v8 1/5] ethdev: fix docs of functions " Andrew Rybchenko
2021-09-30 16:05   ` [dpdk-stable] [PATCH v8 3/5] ethdev: fix docs of drivers callbacks " Andrew Rybchenko
2021-09-30 16:33     ` Ferruh Yigit
2021-10-01  9:07       ` Andrew Rybchenko
2021-10-01  9:07 ` [dpdk-stable] [PATCH v9 1/5] ethdev: fix docs of functions " Andrew Rybchenko
2021-10-01  9:07   ` [dpdk-stable] [PATCH v9 3/5] ethdev: fix docs of drivers callbacks " Andrew Rybchenko
2021-10-01  9:42     ` Ferruh Yigit
2021-10-01 10:22       ` Andrew Rybchenko
2021-10-06 10:37         ` Ferruh Yigit
2021-10-06 11:08   ` [dpdk-stable] [PATCH v9 1/5] ethdev: fix docs of functions " Ferruh Yigit

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=c6bcce29-88bf-c433-5197-5bca8a0462e4@oktetlabs.ru \
    --to=andrew.rybchenko@oktetlabs.ru \
    --cc=amoreton@xilinx.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=ivan.ilchenko@oktetlabs.ru \
    --cc=kubax.kozak@intel.com \
    --cc=olivier.matz@6wind.com \
    --cc=stable@dpdk.org \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).