[dpdk-dev] [PATCH] ethdev: deprecate shared counters using action attribute

[Xueming(Steven) Li]

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko at oktetlabs.ru>
> Sent: Tuesday, November 3, 2020 1:56 AM
> To: Slava Ovsiienko <viacheslavo at nvidia.com>; Ferruh Yigit
> <ferruh.yigit at intel.com>; Xueming(Steven) Li <xuemingl at nvidia.com>;
> Andrew Rybchenko <arybchenko at solarflare.com>; dev at dpdk.org;
> declan.doherty at intel.com
> Cc: Andrey Vesnovaty <andreyv at nvidia.com>; NBU-Contact-Thomas
> Monjalon <thomas at monjalon.net>; Ray Kinsella <mdr at ashroe.eu>; Neil
> Horman <nhorman at tuxdriver.com>; Ori Kam <orika at nvidia.com>; Wei Hu
> (Xavier) <xavier.huwei at huawei.com>; Min Hu (Connor)
> <humin29 at huawei.com>; Yisen Zhuang <yisen.zhuang at huawei.com>; Lijun
> Ou <oulijun at huawei.com>; Matan Azrad <matan at nvidia.com>; Shahaf
> Shuler <shahafs at nvidia.com>; Jasvinder Singh <jasvinder.singh at intel.com>;
> Cristian Dumitrescu <cristian.dumitrescu at intel.com>; Ajit Khaparde
> <ajit.khaparde at broadcom.com>; Somnath Kotur
> <somnath.kotur at broadcom.com>; Qiming Yang <qiming.yang at intel.com>;
> Qi Zhang <qi.z.zhang at intel.com>
> Subject: Re: [dpdk-dev] [PATCH] ethdev: deprecate shared counters using
> action attribute
> 
> On 11/2/20 8:48 PM, Slava Ovsiienko wrote:
> >> -----Original Message-----
> >> From: Andrew Rybchenko <andrew.rybchenko at oktetlabs.ru>
> >> Sent: Monday, November 2, 2020 18:40
> >> To: Slava Ovsiienko <viacheslavo at nvidia.com>; Ferruh Yigit
> >> <ferruh.yigit at intel.com>; Xueming(Steven) Li <xuemingl at nvidia.com>;
> >> Andrew Rybchenko <arybchenko at solarflare.com>; dev at dpdk.org;
> >> declan.doherty at intel.com
> >> Cc: Andrey Vesnovaty <andreyv at nvidia.com>; NBU-Contact-Thomas
> >> Monjalon <thomas at monjalon.net>; Ray Kinsella <mdr at ashroe.eu>; Neil
> >> Horman <nhorman at tuxdriver.com>; Ori Kam <orika at nvidia.com>; Wei
> Hu
> >> (Xavier) <xavier.huwei at huawei.com>; Min Hu (Connor)
> >> <humin29 at huawei.com>; Yisen Zhuang <yisen.zhuang at huawei.com>;
> Lijun
> >> Ou <oulijun at huawei.com>; Matan Azrad <matan at nvidia.com>; Shahaf
> >> Shuler <shahafs at nvidia.com>; Jasvinder Singh
> >> <jasvinder.singh at intel.com>; Cristian Dumitrescu
> >> <cristian.dumitrescu at intel.com>; Ajit Khaparde
> >> <ajit.khaparde at broadcom.com>; Somnath Kotur
> >> <somnath.kotur at broadcom.com>; Qiming Yang
> <qiming.yang at intel.com>; Qi
> >> Zhang <qi.z.zhang at intel.com>
> >> Subject: Re: [dpdk-dev] [PATCH] ethdev: deprecate shared counters
> >> using action attribute
> >>
> >> On 11/2/20 7:12 PM, Slava Ovsiienko wrote:
> >>>> -----Original Message-----
> >>>> From: Ferruh Yigit <ferruh.yigit at intel.com>
> >>>> Sent: Monday, November 2, 2020 18:01
> >>>> To: Andrew Rybchenko <andrew.rybchenko at oktetlabs.ru>;
> >> Xueming(Steven)
> >>>> Li <xuemingl at nvidia.com>; Andrew Rybchenko
> >>>> <arybchenko at solarflare.com>; dev at dpdk.org;
> declan.doherty at intel.com
> >>>> Cc: Andrey Vesnovaty <andreyv at nvidia.com>; NBU-Contact-Thomas
> >>>> Monjalon <thomas at monjalon.net>; Ray Kinsella <mdr at ashroe.eu>;
> Neil
> >>>> Horman <nhorman at tuxdriver.com>; Ori Kam <orika at nvidia.com>;
> Wei Hu
> >>>> (Xavier) <xavier.huwei at huawei.com>; Min Hu (Connor)
> >>>> <humin29 at huawei.com>; Yisen Zhuang <yisen.zhuang at huawei.com>;
> >> Lijun
> >>>> Ou <oulijun at huawei.com>; Matan Azrad <matan at nvidia.com>;
> Shahaf
> >>>> Shuler <shahafs at nvidia.com>; Slava Ovsiienko
> >>>> <viacheslavo at nvidia.com>; Jasvinder Singh
> >>>> <jasvinder.singh at intel.com>; Cristian Dumitrescu
> >>>> <cristian.dumitrescu at intel.com>; Ajit Khaparde
> >>>> <ajit.khaparde at broadcom.com>; Somnath Kotur
> >>>> <somnath.kotur at broadcom.com>; Qiming Yang
> <qiming.yang at intel.com>;
> >> Qi
> >>>> Zhang <qi.z.zhang at intel.com>
> >>>> Subject: Re: [dpdk-dev] [PATCH] ethdev: deprecate shared counters
> >>>> using action attribute
> >>>>
> >>>> On 11/1/2020 10:45 AM, Andrew Rybchenko wrote:
> >>>>> On 10/30/20 7:12 PM, Xueming(Steven) Li wrote:
> >>>>>> Hi Andrew,
> >>>>>>
> >>>>>>> -----Original Message-----
> >>>>>>> From: dev <dev-bounces at dpdk.org> On Behalf Of Andrew
> Rybchenko
> >>>>>>> Sent: Thursday, October 29, 2020 4:53 PM
> >>>>>>> To: dev at dpdk.org
> >>>>>>> Cc: Andrey Vesnovaty <andreyv at nvidia.com>; NBU-Contact-
> Thomas
> >>>>>>> Monjalon <thomas at monjalon.net>; Ferruh Yigit
> >>>>>>> <ferruh.yigit at intel.com>; Ray Kinsella <mdr at ashroe.eu>; Neil
> >>>>>>> Horman <nhorman at tuxdriver.com>; Ori Kam <orika at nvidia.com>;
> >> Andrew
> >>>>>>> Rybchenko <andrew.rybchenko at oktetlabs.ru>
> >>>>>>> Subject: [dpdk-dev] [PATCH] ethdev: deprecate shared counters
> >>>>>>> using action attribute
> >>>>>>>
> >>>>>>> A new generic shared actions API may be used to create shared
> counter.
> >>>>>>> There is no point to keep duplicate COUNT action specific
> >>>>>>> capability to create shared counters.
> >>>>>>>
> >>>>>>> Signed-off-by: Andrew Rybchenko <arybchenko at solarflare.com>
> >>>>>>> ---
> >>>>>>> In fact, it looks like the next logical step is to remove struct
> >>>>>>> rte_flow_action_count completely since counter ID makes sense
> >>>>>>> for shared counters only. I think it will just make it easiser
> >>>>>>> to use COUNT
> >>>> action.
> >>>>>>> Comments are welcome.
> >>>>>>>
> >>>>>>>     doc/guides/rel_notes/deprecation.rst | 4 ++++
> >>>>>>>     lib/librte_ethdev/rte_flow.h         | 6 +++++-
> >>>>>>>     2 files changed, 9 insertions(+), 1 deletion(-)
> >>>>>>>
> >>>>>>> diff --git a/doc/guides/rel_notes/deprecation.rst
> >>>>>>> b/doc/guides/rel_notes/deprecation.rst
> >>>>>>> index 2e082499b8..4f3bac1a6d 100644
> >>>>>>> --- a/doc/guides/rel_notes/deprecation.rst
> >>>>>>> +++ b/doc/guides/rel_notes/deprecation.rst
> >>>>>>> @@ -138,6 +138,10 @@ Deprecation Notices
> >>>>>>>       will be limited to maximum 256 queues.
> >>>>>>>       Also compile time flag ``RTE_ETHDEV_QUEUE_STAT_CNTRS``
> >>>>>>> will be removed.
> >>>>>>>
> >>>>>>> +* ethdev: Attribute ``shared`` of the ``struct
> >>>>>>> +rte_flow_action_count``
> >>>>>>> +  is deprecated and will be removed in DPDK 21.11. Shared
> >>>>>>> +counters should
> >>>>>>> +  be managed using shared actions API
> >>>>>>> +(``rte_flow_shared_action_create``
> >>>>>>> etc).
> >>>>>>> +
> >>>>>>>     * cryptodev: support for using IV with all sizes is added,
> >>>>>>> J0 still can
> >>>>>>>       be used but only when IV length in following structs
> >>>>>>> ``rte_crypto_auth_xform``,
> >>>>>>>       ``rte_crypto_aead_xform`` is set to zero. When IV length
> >>>>>>> is greater or equal diff --git a/lib/librte_ethdev/rte_flow.h
> >>>>>>> b/lib/librte_ethdev/rte_flow.h index a8eac4deb8..2bb93d237a
> >>>>>>> 100644
> >>>>>>> --- a/lib/librte_ethdev/rte_flow.h
> >>>>>>> +++ b/lib/librte_ethdev/rte_flow.h
> >>>>>>> @@ -2287,6 +2287,9 @@ struct rte_flow_query_age {
> >>>>>>>      * Counters can be retrieved and reset through
> >>>>>>> ``rte_flow_query()``, see
> >>>>>>>      * ``struct rte_flow_query_count``.
> >>>>>>>      *
> >>>>>>> + * @deprecated Shared attribute is deprecated, use generic
> >>>>>>> + * RTE_FLOW_ACTION_TYPE_SHARED action.
> >>>>>>> + *
> >>>>>>>      * The shared flag indicates whether the counter is unique
> >>>>>>> to the flow rule the
> >>>>>>>      * action is specified with, or whether it is a shared counter.
> >>>>>>>      *
> >>>>>>> @@ -2299,7 +2302,8 @@ struct rte_flow_query_age {
> >>>>>>>      * to all ports within that switch domain.
> >>>>>>>      */
> >>>>>>>     struct rte_flow_action_count {
> >>>>>>> -    uint32_t shared:1; /**< Share counter ID with other flow rules.
> >>>>>>> */
> >>>>>>> +    /** @deprecated Share counter ID with other flow rules. */
> >>>>>>> +    uint32_t shared:1;
> >>>>>>>         uint32_t reserved:31; /**< Reserved, must be zero. */
> >>>>>>>         uint32_t id; /**< Counter ID. */
> >>>>>> Do you think id could be removed as well? neither non-shared flow
> >>>>>> counter query, nor shared action query.
> >>>>>
> >>>>> I'm not 100% sure, but yes, as I write above just after my Signed-off-by.
> >>>>>
> >>>>
> >>>> cc'ed Declan + maintainers of PMDs for the 'id' field, but as far
> >>>> as I can see it is used out of the 'shared' context, so I am for
> >>>> going on with existing patch for now.
> >>>>
> >>>> Reviewed-by: Ferruh Yigit <ferruh.yigit at intel.com>
> >>>
> >>> It depends whether we are going to support multiple counters for the
> >>> same
> >> flow.
> >>
> >> Why? Query refers to a counter using action pointer. There is always
> >> one counter in one action. If you need more counters, just use more
> actions.
> >>
> > Honestly, I wonder if someone wants to use multiple counters in the same
> flow.
> > That might happen if we add some unique attributes to the counter
> > action (say, count some complex events/traffic params).
> >
> 
> It might make sense for shared counters, but it they are done using shared
> actions, we don't need ID as well.
> 

Flow 5 tuple flows, besides the per flow counter, some customer wants 
shared counter per stream type, per VM and per customer(owns group of VMs).
Shared counter action should work.


> > Action pointer in the rte_flow_query() is just a pointer to some
> > counter action describing the counter. If we drop id field (and only
> > action type COUNTER remains in action description) - there would be no
> > way to distinguish two (regular, not shared) counters in the flow. Which
> one should be returned on query?
> >
> 
> I thought that 'action' must be exactly the same pointer to actions array
> element specified on flow creation. It looks like I'm wrong.
> If so, yes, id is still required for non-shared many COUNT actions in the same
> flow. I doubt that we need it as well.
> Thanks for clarification.
> 

By using shared counter action, only one per flow counter allowed, no need
to support multiple per flow counters IMHO.

In current design, the user specified counter ID field of shared counter implies
additional mapping with PMD internal counter - another reason to deprecate
counter ID.

> >>> If there is the only counter per flow we could get rid of the "id"
> >>> field either. If it is still needed, PMDs should generate counter id
> >>> internally
> >> and id should not be exposed outside.
> >>>
> >>> With best regards, Slava
> >>>
> >>> PS. What about meters? The next good candidate to shared actions.
> >>>
> >>>
> >