ethdev: deprecate shared counters using action attribute

Message ID 1603961577-13264-1-git-send-email-arybchenko@solarflare.com (mailing list archive)
State Superseded, archived
Delegated to: Ferruh Yigit
Headers
Series ethdev: deprecate shared counters using action attribute |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/iol-intel-Functional success Functional Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/travis-robot success Travis build: passed
ci/iol-mellanox-Performance success Performance Testing PASS

Commit Message

Andrew Rybchenko Oct. 29, 2020, 8:52 a.m. UTC
  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@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(-)
  

Comments

Ori Kam Oct. 29, 2020, 2:39 p.m. UTC | #1
Hi Andrew,

> -----Original Message-----
> From: Andrew Rybchenko <arybchenko@solarflare.com>
> Sent: Thursday, October 29, 2020 10:53 AM
> Subject: [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@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. */
>  };
> --
> 2.17.1

As much as I agree with your patch, I don't think we should push it since
not all PMD made the move to support count action, so the application still needs
to use this API.

I think this patch should be done but in next LTS release.

Best,
Ori
  
Thomas Monjalon Oct. 29, 2020, 4:11 p.m. UTC | #2
29/10/2020 15:39, Ori Kam:
> Hi Andrew,
> 
[...]
> > 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@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.
[...]
> > +* 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).
> > +
[...]
> > + * @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. */
> >  };
> > --
> > 2.17.1
> 
> As much as I agree with your patch, I don't think we should push it since
> not all PMD made the move to support count action, so the application still needs
> to use this API.
> 
> I think this patch should be done but in next LTS release.

The patch is not removing the field,
it is just warning it will be removed in next year.
  
Xueming Li Oct. 30, 2020, 4:12 p.m. UTC | #3
Hi Andrew,

> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Andrew Rybchenko
> Sent: Thursday, October 29, 2020 4:53 PM
> To: dev@dpdk.org
> Cc: Andrey Vesnovaty <andreyv@nvidia.com>; NBU-Contact-Thomas
> Monjalon <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>;
> Ray Kinsella <mdr@ashroe.eu>; Neil Horman <nhorman@tuxdriver.com>;
> Ori Kam <orika@nvidia.com>; Andrew Rybchenko
> <andrew.rybchenko@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@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.

>  };
> --
> 2.17.1
  
Ori Kam Nov. 1, 2020, 7:49 a.m. UTC | #4
> -----Original Message-----
> From: Thomas Monjalon <thomas@monjalon.net>
> Sent: Thursday, October 29, 2020 6:12 PM
> To: Ori Kam <orika@nvidia.com>
> Subject: Re: [PATCH] ethdev: deprecate shared counters using action attribute
> 
> 29/10/2020 15:39, Ori Kam:
> > Hi Andrew,
> >
> [...]
> > > 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@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.
> [...]
> > > +* 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).
> > > +
> [...]
> > > + * @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. */
> > >  };
> > > --
> > > 2.17.1
> >
> > As much as I agree with your patch, I don't think we should push it since
> > not all PMD made the move to support count action, so the application still
> needs
> > to use this API.
> >
> > I think this patch should be done but in next LTS release.
> 
> The patch is not removing the field,
> it is just warning it will be removed in next year.
> 

Yes I know, but I don't think it is correct to issue such a warning without support.
The application still must use this API, the warning should be added as soon as
at least one PMD support shared counters with the new API.

Best,
Ori
  
Andrew Rybchenko Nov. 1, 2020, 10:45 a.m. UTC | #5
On 10/30/20 7:12 PM, Xueming(Steven) Li wrote:
> Hi Andrew,
> 
>> -----Original Message-----
>> From: dev <dev-bounces@dpdk.org> On Behalf Of Andrew Rybchenko
>> Sent: Thursday, October 29, 2020 4:53 PM
>> To: dev@dpdk.org
>> Cc: Andrey Vesnovaty <andreyv@nvidia.com>; NBU-Contact-Thomas
>> Monjalon <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>;
>> Ray Kinsella <mdr@ashroe.eu>; Neil Horman <nhorman@tuxdriver.com>;
>> Ori Kam <orika@nvidia.com>; Andrew Rybchenko
>> <andrew.rybchenko@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@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.
  
Ferruh Yigit Nov. 2, 2020, 4:01 p.m. UTC | #6
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@dpdk.org> On Behalf Of Andrew Rybchenko
>>> Sent: Thursday, October 29, 2020 4:53 PM
>>> To: dev@dpdk.org
>>> Cc: Andrey Vesnovaty <andreyv@nvidia.com>; NBU-Contact-Thomas
>>> Monjalon <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>;
>>> Ray Kinsella <mdr@ashroe.eu>; Neil Horman <nhorman@tuxdriver.com>;
>>> Ori Kam <orika@nvidia.com>; Andrew Rybchenko
>>> <andrew.rybchenko@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@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@intel.com>
  
Slava Ovsiienko Nov. 2, 2020, 4:12 p.m. UTC | #7
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Monday, November 2, 2020 18:01
> To: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Xueming(Steven)
> Li <xuemingl@nvidia.com>; Andrew Rybchenko <arybchenko@solarflare.com>;
> dev@dpdk.org; declan.doherty@intel.com
> Cc: Andrey Vesnovaty <andreyv@nvidia.com>; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>; Ray Kinsella <mdr@ashroe.eu>; Neil Horman
> <nhorman@tuxdriver.com>; Ori Kam <orika@nvidia.com>; Wei Hu (Xavier)
> <xavier.huwei@huawei.com>; Min Hu (Connor) <humin29@huawei.com>;
> Yisen Zhuang <yisen.zhuang@huawei.com>; Lijun Ou <oulijun@huawei.com>;
> Matan Azrad <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>;
> Slava Ovsiienko <viacheslavo@nvidia.com>; Jasvinder Singh
> <jasvinder.singh@intel.com>; Cristian Dumitrescu
> <cristian.dumitrescu@intel.com>; Ajit Khaparde
> <ajit.khaparde@broadcom.com>; Somnath Kotur
> <somnath.kotur@broadcom.com>; Qiming Yang <qiming.yang@intel.com>; Qi
> Zhang <qi.z.zhang@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@dpdk.org> On Behalf Of Andrew Rybchenko
> >>> Sent: Thursday, October 29, 2020 4:53 PM
> >>> To: dev@dpdk.org
> >>> Cc: Andrey Vesnovaty <andreyv@nvidia.com>; NBU-Contact-Thomas
> >>> Monjalon <thomas@monjalon.net>; Ferruh Yigit
> >>> <ferruh.yigit@intel.com>; Ray Kinsella <mdr@ashroe.eu>; Neil Horman
> >>> <nhorman@tuxdriver.com>; Ori Kam <orika@nvidia.com>; Andrew
> >>> Rybchenko <andrew.rybchenko@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@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@intel.com>

It depends whether we are going to support multiple counters for the same flow.
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.
  
Andrew Rybchenko Nov. 2, 2020, 4:40 p.m. UTC | #8
On 11/2/20 7:12 PM, Slava Ovsiienko wrote:
>> -----Original Message-----
>> From: Ferruh Yigit <ferruh.yigit@intel.com>
>> Sent: Monday, November 2, 2020 18:01
>> To: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>; Xueming(Steven)
>> Li <xuemingl@nvidia.com>; Andrew Rybchenko <arybchenko@solarflare.com>;
>> dev@dpdk.org; declan.doherty@intel.com
>> Cc: Andrey Vesnovaty <andreyv@nvidia.com>; NBU-Contact-Thomas Monjalon
>> <thomas@monjalon.net>; Ray Kinsella <mdr@ashroe.eu>; Neil Horman
>> <nhorman@tuxdriver.com>; Ori Kam <orika@nvidia.com>; Wei Hu (Xavier)
>> <xavier.huwei@huawei.com>; Min Hu (Connor) <humin29@huawei.com>;
>> Yisen Zhuang <yisen.zhuang@huawei.com>; Lijun Ou <oulijun@huawei.com>;
>> Matan Azrad <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>;
>> Slava Ovsiienko <viacheslavo@nvidia.com>; Jasvinder Singh
>> <jasvinder.singh@intel.com>; Cristian Dumitrescu
>> <cristian.dumitrescu@intel.com>; Ajit Khaparde
>> <ajit.khaparde@broadcom.com>; Somnath Kotur
>> <somnath.kotur@broadcom.com>; Qiming Yang <qiming.yang@intel.com>; Qi
>> Zhang <qi.z.zhang@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@dpdk.org> On Behalf Of Andrew Rybchenko
>>>>> Sent: Thursday, October 29, 2020 4:53 PM
>>>>> To: dev@dpdk.org
>>>>> Cc: Andrey Vesnovaty <andreyv@nvidia.com>; NBU-Contact-Thomas
>>>>> Monjalon <thomas@monjalon.net>; Ferruh Yigit
>>>>> <ferruh.yigit@intel.com>; Ray Kinsella <mdr@ashroe.eu>; Neil Horman
>>>>> <nhorman@tuxdriver.com>; Ori Kam <orika@nvidia.com>; Andrew
>>>>> Rybchenko <andrew.rybchenko@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@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@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.

> 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.
> 
>
  
Slava Ovsiienko Nov. 2, 2020, 5:48 p.m. UTC | #9
> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Monday, November 2, 2020 18:40
> To: Slava Ovsiienko <viacheslavo@nvidia.com>; Ferruh Yigit
> <ferruh.yigit@intel.com>; Xueming(Steven) Li <xuemingl@nvidia.com>;
> Andrew Rybchenko <arybchenko@solarflare.com>; dev@dpdk.org;
> declan.doherty@intel.com
> Cc: Andrey Vesnovaty <andreyv@nvidia.com>; NBU-Contact-Thomas Monjalon
> <thomas@monjalon.net>; Ray Kinsella <mdr@ashroe.eu>; Neil Horman
> <nhorman@tuxdriver.com>; Ori Kam <orika@nvidia.com>; Wei Hu (Xavier)
> <xavier.huwei@huawei.com>; Min Hu (Connor) <humin29@huawei.com>;
> Yisen Zhuang <yisen.zhuang@huawei.com>; Lijun Ou <oulijun@huawei.com>;
> Matan Azrad <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>;
> Jasvinder Singh <jasvinder.singh@intel.com>; Cristian Dumitrescu
> <cristian.dumitrescu@intel.com>; Ajit Khaparde
> <ajit.khaparde@broadcom.com>; Somnath Kotur
> <somnath.kotur@broadcom.com>; Qiming Yang <qiming.yang@intel.com>; Qi
> Zhang <qi.z.zhang@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@intel.com>
> >> Sent: Monday, November 2, 2020 18:01
> >> To: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>;
> Xueming(Steven)
> >> Li <xuemingl@nvidia.com>; Andrew Rybchenko
> >> <arybchenko@solarflare.com>; dev@dpdk.org; declan.doherty@intel.com
> >> Cc: Andrey Vesnovaty <andreyv@nvidia.com>; NBU-Contact-Thomas
> >> Monjalon <thomas@monjalon.net>; Ray Kinsella <mdr@ashroe.eu>; Neil
> >> Horman <nhorman@tuxdriver.com>; Ori Kam <orika@nvidia.com>; Wei Hu
> >> (Xavier) <xavier.huwei@huawei.com>; Min Hu (Connor)
> >> <humin29@huawei.com>; Yisen Zhuang <yisen.zhuang@huawei.com>;
> Lijun
> >> Ou <oulijun@huawei.com>; Matan Azrad <matan@nvidia.com>; Shahaf
> >> Shuler <shahafs@nvidia.com>; Slava Ovsiienko
> >> <viacheslavo@nvidia.com>; Jasvinder Singh
> >> <jasvinder.singh@intel.com>; Cristian Dumitrescu
> >> <cristian.dumitrescu@intel.com>; Ajit Khaparde
> >> <ajit.khaparde@broadcom.com>; Somnath Kotur
> >> <somnath.kotur@broadcom.com>; Qiming Yang <qiming.yang@intel.com>;
> Qi
> >> Zhang <qi.z.zhang@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@dpdk.org> On Behalf Of Andrew Rybchenko
> >>>>> Sent: Thursday, October 29, 2020 4:53 PM
> >>>>> To: dev@dpdk.org
> >>>>> Cc: Andrey Vesnovaty <andreyv@nvidia.com>; NBU-Contact-Thomas
> >>>>> Monjalon <thomas@monjalon.net>; Ferruh Yigit
> >>>>> <ferruh.yigit@intel.com>; Ray Kinsella <mdr@ashroe.eu>; Neil
> >>>>> Horman <nhorman@tuxdriver.com>; Ori Kam <orika@nvidia.com>;
> Andrew
> >>>>> Rybchenko <andrew.rybchenko@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@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@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).

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?  

> > 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.
> >
> >
  
Andrew Rybchenko Nov. 2, 2020, 5:55 p.m. UTC | #10
On 11/2/20 8:48 PM, Slava Ovsiienko wrote:
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Monday, November 2, 2020 18:40
>> To: Slava Ovsiienko <viacheslavo@nvidia.com>; Ferruh Yigit
>> <ferruh.yigit@intel.com>; Xueming(Steven) Li <xuemingl@nvidia.com>;
>> Andrew Rybchenko <arybchenko@solarflare.com>; dev@dpdk.org;
>> declan.doherty@intel.com
>> Cc: Andrey Vesnovaty <andreyv@nvidia.com>; NBU-Contact-Thomas Monjalon
>> <thomas@monjalon.net>; Ray Kinsella <mdr@ashroe.eu>; Neil Horman
>> <nhorman@tuxdriver.com>; Ori Kam <orika@nvidia.com>; Wei Hu (Xavier)
>> <xavier.huwei@huawei.com>; Min Hu (Connor) <humin29@huawei.com>;
>> Yisen Zhuang <yisen.zhuang@huawei.com>; Lijun Ou <oulijun@huawei.com>;
>> Matan Azrad <matan@nvidia.com>; Shahaf Shuler <shahafs@nvidia.com>;
>> Jasvinder Singh <jasvinder.singh@intel.com>; Cristian Dumitrescu
>> <cristian.dumitrescu@intel.com>; Ajit Khaparde
>> <ajit.khaparde@broadcom.com>; Somnath Kotur
>> <somnath.kotur@broadcom.com>; Qiming Yang <qiming.yang@intel.com>; Qi
>> Zhang <qi.z.zhang@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@intel.com>
>>>> Sent: Monday, November 2, 2020 18:01
>>>> To: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>;
>> Xueming(Steven)
>>>> Li <xuemingl@nvidia.com>; Andrew Rybchenko
>>>> <arybchenko@solarflare.com>; dev@dpdk.org; declan.doherty@intel.com
>>>> Cc: Andrey Vesnovaty <andreyv@nvidia.com>; NBU-Contact-Thomas
>>>> Monjalon <thomas@monjalon.net>; Ray Kinsella <mdr@ashroe.eu>; Neil
>>>> Horman <nhorman@tuxdriver.com>; Ori Kam <orika@nvidia.com>; Wei Hu
>>>> (Xavier) <xavier.huwei@huawei.com>; Min Hu (Connor)
>>>> <humin29@huawei.com>; Yisen Zhuang <yisen.zhuang@huawei.com>;
>> Lijun
>>>> Ou <oulijun@huawei.com>; Matan Azrad <matan@nvidia.com>; Shahaf
>>>> Shuler <shahafs@nvidia.com>; Slava Ovsiienko
>>>> <viacheslavo@nvidia.com>; Jasvinder Singh
>>>> <jasvinder.singh@intel.com>; Cristian Dumitrescu
>>>> <cristian.dumitrescu@intel.com>; Ajit Khaparde
>>>> <ajit.khaparde@broadcom.com>; Somnath Kotur
>>>> <somnath.kotur@broadcom.com>; Qiming Yang <qiming.yang@intel.com>;
>> Qi
>>>> Zhang <qi.z.zhang@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@dpdk.org> On Behalf Of Andrew Rybchenko
>>>>>>> Sent: Thursday, October 29, 2020 4:53 PM
>>>>>>> To: dev@dpdk.org
>>>>>>> Cc: Andrey Vesnovaty <andreyv@nvidia.com>; NBU-Contact-Thomas
>>>>>>> Monjalon <thomas@monjalon.net>; Ferruh Yigit
>>>>>>> <ferruh.yigit@intel.com>; Ray Kinsella <mdr@ashroe.eu>; Neil
>>>>>>> Horman <nhorman@tuxdriver.com>; Ori Kam <orika@nvidia.com>;
>> Andrew
>>>>>>> Rybchenko <andrew.rybchenko@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@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@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.

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

>>> 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.
>>>
>>>
>
  
Xueming Li Nov. 3, 2020, 1:30 p.m. UTC | #11
> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Tuesday, November 3, 2020 1:56 AM
> To: Slava Ovsiienko <viacheslavo@nvidia.com>; Ferruh Yigit
> <ferruh.yigit@intel.com>; Xueming(Steven) Li <xuemingl@nvidia.com>;
> Andrew Rybchenko <arybchenko@solarflare.com>; dev@dpdk.org;
> declan.doherty@intel.com
> Cc: Andrey Vesnovaty <andreyv@nvidia.com>; NBU-Contact-Thomas
> Monjalon <thomas@monjalon.net>; Ray Kinsella <mdr@ashroe.eu>; Neil
> Horman <nhorman@tuxdriver.com>; Ori Kam <orika@nvidia.com>; Wei Hu
> (Xavier) <xavier.huwei@huawei.com>; Min Hu (Connor)
> <humin29@huawei.com>; Yisen Zhuang <yisen.zhuang@huawei.com>; Lijun
> Ou <oulijun@huawei.com>; Matan Azrad <matan@nvidia.com>; Shahaf
> Shuler <shahafs@nvidia.com>; Jasvinder Singh <jasvinder.singh@intel.com>;
> Cristian Dumitrescu <cristian.dumitrescu@intel.com>; Ajit Khaparde
> <ajit.khaparde@broadcom.com>; Somnath Kotur
> <somnath.kotur@broadcom.com>; Qiming Yang <qiming.yang@intel.com>;
> Qi Zhang <qi.z.zhang@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@oktetlabs.ru>
> >> Sent: Monday, November 2, 2020 18:40
> >> To: Slava Ovsiienko <viacheslavo@nvidia.com>; Ferruh Yigit
> >> <ferruh.yigit@intel.com>; Xueming(Steven) Li <xuemingl@nvidia.com>;
> >> Andrew Rybchenko <arybchenko@solarflare.com>; dev@dpdk.org;
> >> declan.doherty@intel.com
> >> Cc: Andrey Vesnovaty <andreyv@nvidia.com>; NBU-Contact-Thomas
> >> Monjalon <thomas@monjalon.net>; Ray Kinsella <mdr@ashroe.eu>; Neil
> >> Horman <nhorman@tuxdriver.com>; Ori Kam <orika@nvidia.com>; Wei
> Hu
> >> (Xavier) <xavier.huwei@huawei.com>; Min Hu (Connor)
> >> <humin29@huawei.com>; Yisen Zhuang <yisen.zhuang@huawei.com>;
> Lijun
> >> Ou <oulijun@huawei.com>; Matan Azrad <matan@nvidia.com>; Shahaf
> >> Shuler <shahafs@nvidia.com>; Jasvinder Singh
> >> <jasvinder.singh@intel.com>; Cristian Dumitrescu
> >> <cristian.dumitrescu@intel.com>; Ajit Khaparde
> >> <ajit.khaparde@broadcom.com>; Somnath Kotur
> >> <somnath.kotur@broadcom.com>; Qiming Yang
> <qiming.yang@intel.com>; Qi
> >> Zhang <qi.z.zhang@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@intel.com>
> >>>> Sent: Monday, November 2, 2020 18:01
> >>>> To: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>;
> >> Xueming(Steven)
> >>>> Li <xuemingl@nvidia.com>; Andrew Rybchenko
> >>>> <arybchenko@solarflare.com>; dev@dpdk.org;
> declan.doherty@intel.com
> >>>> Cc: Andrey Vesnovaty <andreyv@nvidia.com>; NBU-Contact-Thomas
> >>>> Monjalon <thomas@monjalon.net>; Ray Kinsella <mdr@ashroe.eu>;
> Neil
> >>>> Horman <nhorman@tuxdriver.com>; Ori Kam <orika@nvidia.com>;
> Wei Hu
> >>>> (Xavier) <xavier.huwei@huawei.com>; Min Hu (Connor)
> >>>> <humin29@huawei.com>; Yisen Zhuang <yisen.zhuang@huawei.com>;
> >> Lijun
> >>>> Ou <oulijun@huawei.com>; Matan Azrad <matan@nvidia.com>;
> Shahaf
> >>>> Shuler <shahafs@nvidia.com>; Slava Ovsiienko
> >>>> <viacheslavo@nvidia.com>; Jasvinder Singh
> >>>> <jasvinder.singh@intel.com>; Cristian Dumitrescu
> >>>> <cristian.dumitrescu@intel.com>; Ajit Khaparde
> >>>> <ajit.khaparde@broadcom.com>; Somnath Kotur
> >>>> <somnath.kotur@broadcom.com>; Qiming Yang
> <qiming.yang@intel.com>;
> >> Qi
> >>>> Zhang <qi.z.zhang@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@dpdk.org> On Behalf Of Andrew
> Rybchenko
> >>>>>>> Sent: Thursday, October 29, 2020 4:53 PM
> >>>>>>> To: dev@dpdk.org
> >>>>>>> Cc: Andrey Vesnovaty <andreyv@nvidia.com>; NBU-Contact-
> Thomas
> >>>>>>> Monjalon <thomas@monjalon.net>; Ferruh Yigit
> >>>>>>> <ferruh.yigit@intel.com>; Ray Kinsella <mdr@ashroe.eu>; Neil
> >>>>>>> Horman <nhorman@tuxdriver.com>; Ori Kam <orika@nvidia.com>;
> >> Andrew
> >>>>>>> Rybchenko <andrew.rybchenko@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@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@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.
> >>>
> >>>
> >
  
Xueming Li Nov. 3, 2020, 1:34 p.m. UTC | #12
> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: Tuesday, November 3, 2020 1:56 AM
> To: Slava Ovsiienko <viacheslavo@nvidia.com>; Ferruh Yigit
> <ferruh.yigit@intel.com>; Xueming(Steven) Li <xuemingl@nvidia.com>;
> Andrew Rybchenko <arybchenko@solarflare.com>; dev@dpdk.org;
> declan.doherty@intel.com
> Cc: Andrey Vesnovaty <andreyv@nvidia.com>; NBU-Contact-Thomas
> Monjalon <thomas@monjalon.net>; Ray Kinsella <mdr@ashroe.eu>; Neil
> Horman <nhorman@tuxdriver.com>; Ori Kam <orika@nvidia.com>; Wei Hu
> (Xavier) <xavier.huwei@huawei.com>; Min Hu (Connor)
> <humin29@huawei.com>; Yisen Zhuang <yisen.zhuang@huawei.com>; Lijun
> Ou <oulijun@huawei.com>; Matan Azrad <matan@nvidia.com>; Shahaf
> Shuler <shahafs@nvidia.com>; Jasvinder Singh <jasvinder.singh@intel.com>;
> Cristian Dumitrescu <cristian.dumitrescu@intel.com>; Ajit Khaparde
> <ajit.khaparde@broadcom.com>; Somnath Kotur
> <somnath.kotur@broadcom.com>; Qiming Yang <qiming.yang@intel.com>;
> Qi Zhang <qi.z.zhang@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@oktetlabs.ru>
> >> Sent: Monday, November 2, 2020 18:40
> >> To: Slava Ovsiienko <viacheslavo@nvidia.com>; Ferruh Yigit
> >> <ferruh.yigit@intel.com>; Xueming(Steven) Li <xuemingl@nvidia.com>;
> >> Andrew Rybchenko <arybchenko@solarflare.com>; dev@dpdk.org;
> >> declan.doherty@intel.com
> >> Cc: Andrey Vesnovaty <andreyv@nvidia.com>; NBU-Contact-Thomas
> >> Monjalon <thomas@monjalon.net>; Ray Kinsella <mdr@ashroe.eu>; Neil
> >> Horman <nhorman@tuxdriver.com>; Ori Kam <orika@nvidia.com>; Wei
> Hu
> >> (Xavier) <xavier.huwei@huawei.com>; Min Hu (Connor)
> >> <humin29@huawei.com>; Yisen Zhuang <yisen.zhuang@huawei.com>;
> Lijun
> >> Ou <oulijun@huawei.com>; Matan Azrad <matan@nvidia.com>; Shahaf
> >> Shuler <shahafs@nvidia.com>; Jasvinder Singh
> >> <jasvinder.singh@intel.com>; Cristian Dumitrescu
> >> <cristian.dumitrescu@intel.com>; Ajit Khaparde
> >> <ajit.khaparde@broadcom.com>; Somnath Kotur
> >> <somnath.kotur@broadcom.com>; Qiming Yang
> <qiming.yang@intel.com>; Qi
> >> Zhang <qi.z.zhang@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@intel.com>
> >>>> Sent: Monday, November 2, 2020 18:01
> >>>> To: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>;
> >> Xueming(Steven)
> >>>> Li <xuemingl@nvidia.com>; Andrew Rybchenko
> >>>> <arybchenko@solarflare.com>; dev@dpdk.org;
> declan.doherty@intel.com
> >>>> Cc: Andrey Vesnovaty <andreyv@nvidia.com>; NBU-Contact-Thomas
> >>>> Monjalon <thomas@monjalon.net>; Ray Kinsella <mdr@ashroe.eu>;
> Neil
> >>>> Horman <nhorman@tuxdriver.com>; Ori Kam <orika@nvidia.com>;
> Wei Hu
> >>>> (Xavier) <xavier.huwei@huawei.com>; Min Hu (Connor)
> >>>> <humin29@huawei.com>; Yisen Zhuang <yisen.zhuang@huawei.com>;
> >> Lijun
> >>>> Ou <oulijun@huawei.com>; Matan Azrad <matan@nvidia.com>;
> Shahaf
> >>>> Shuler <shahafs@nvidia.com>; Slava Ovsiienko
> >>>> <viacheslavo@nvidia.com>; Jasvinder Singh
> >>>> <jasvinder.singh@intel.com>; Cristian Dumitrescu
> >>>> <cristian.dumitrescu@intel.com>; Ajit Khaparde
> >>>> <ajit.khaparde@broadcom.com>; Somnath Kotur
> >>>> <somnath.kotur@broadcom.com>; Qiming Yang
> <qiming.yang@intel.com>;
> >> Qi
> >>>> Zhang <qi.z.zhang@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@dpdk.org> On Behalf Of Andrew
> Rybchenko
> >>>>>>> Sent: Thursday, October 29, 2020 4:53 PM
> >>>>>>> To: dev@dpdk.org
> >>>>>>> Cc: Andrey Vesnovaty <andreyv@nvidia.com>; NBU-Contact-
> Thomas
> >>>>>>> Monjalon <thomas@monjalon.net>; Ferruh Yigit
> >>>>>>> <ferruh.yigit@intel.com>; Ray Kinsella <mdr@ashroe.eu>; Neil
> >>>>>>> Horman <nhorman@tuxdriver.com>; Ori Kam <orika@nvidia.com>;
> >> Andrew
> >>>>>>> Rybchenko <andrew.rybchenko@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@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@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.
> >>>
> >>>
> >
  
Thomas Monjalon Nov. 3, 2020, 5:21 p.m. UTC | #13
+Cc techboard

There is an interesting case here that we should decide
how to manage in general. Please see below.

01/11/2020 08:49, Ori Kam:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 29/10/2020 15:39, Ori Kam:
> > > >  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. */
> > > >  };
> > >
> > > As much as I agree with your patch, I don't think we should push it since
> > > not all PMD made the move to support count action, so the application still
> > needs
> > > to use this API.
> > >
> > > I think this patch should be done but in next LTS release.
> > 
> > The patch is not removing the field,
> > it is just warning it will be removed in next year.
> 
> Yes I know, but I don't think it is correct to issue such a warning without support.
> The application still must use this API, the warning should be added as soon as
> at least one PMD support shared counters with the new API.

It should be replaced with shared actions API,
but you claim it is not supported yet. Right?
What are the criterias to define the replacement as supported?

What to do in such case?
Can we warn about a deprecation without having a proper replacement?
What is the pre-condition to warn about a deprecation?

About the complete removal, it has already been decided by the techboard
that we cannot remove an API until its replacement is stable.
In other words, the new experimental API must be promoted
in the stable ABI, before removing the deprecated API.
  
Andrew Rybchenko Nov. 3, 2020, 5:26 p.m. UTC | #14
On 11/3/20 8:21 PM, Thomas Monjalon wrote:
> +Cc techboard
> 
> There is an interesting case here that we should decide
> how to manage in general. Please see below.
> 
> 01/11/2020 08:49, Ori Kam:
>> From: Thomas Monjalon <thomas@monjalon.net>
>>> 29/10/2020 15:39, Ori Kam:
>>>>>  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. */
>>>>>  };
>>>>
>>>> As much as I agree with your patch, I don't think we should push it since
>>>> not all PMD made the move to support count action, so the application still
>>> needs
>>>> to use this API.
>>>>
>>>> I think this patch should be done but in next LTS release.
>>>
>>> The patch is not removing the field,
>>> it is just warning it will be removed in next year.
>>
>> Yes I know, but I don't think it is correct to issue such a warning without support.
>> The application still must use this API, the warning should be added as soon as
>> at least one PMD support shared counters with the new API.
> 
> It should be replaced with shared actions API,
> but you claim it is not supported yet. Right?
> What are the criterias to define the replacement as supported?
> 
> What to do in such case?
> Can we warn about a deprecation without having a proper replacement?
> What is the pre-condition to warn about a deprecation?
> 
> About the complete removal, it has already been decided by the techboard
> that we cannot remove an API until its replacement is stable.
> In other words, the new experimental API must be promoted
> in the stable ABI, before removing the deprecated API.
> 

May be the right way here is to remove deprecation mark up,
but add a deprecation notice that it will be deprecated in
21.02 and PMDs are encouraged to switch to shared actions.

Anyway questions above about the criteria are still valid
even in this case and should be answered.
  
Ajit Khaparde Nov. 24, 2020, 6:09 p.m. UTC | #15
On Mon, Nov 2, 2020 at 8:01 AM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> 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@dpdk.org> On Behalf Of Andrew Rybchenko
> >>> Sent: Thursday, October 29, 2020 4:53 PM
> >>> To: dev@dpdk.org
> >>> Cc: Andrey Vesnovaty <andreyv@nvidia.com>; NBU-Contact-Thomas
> >>> Monjalon <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>;
> >>> Ray Kinsella <mdr@ashroe.eu>; Neil Horman <nhorman@tuxdriver.com>;
> >>> Ori Kam <orika@nvidia.com>; Andrew Rybchenko
> >>> <andrew.rybchenko@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@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@intel.com>

Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
  
Ori Kam Nov. 26, 2020, 9:08 a.m. UTC | #16
Hi All

> -----Original Message-----
> From: Ajit Khaparde <ajit.khaparde@broadcom.com>
> Sent: Tuesday, November 24, 2020 8:10 PM
> Subject: Re: [dpdk-dev] [PATCH] ethdev: deprecate shared counters using action
> attribute
> 
> On Mon, Nov 2, 2020 at 8:01 AM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> >
> > 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@dpdk.org> On Behalf Of Andrew Rybchenko
> > >>> Sent: Thursday, October 29, 2020 4:53 PM
> > >>> To: dev@dpdk.org
> > >>> Cc: Andrey Vesnovaty <andreyv@nvidia.com>; NBU-Contact-Thomas
> > >>> Monjalon <thomas@monjalon.net>; Ferruh Yigit
> <ferruh.yigit@intel.com>;
> > >>> Ray Kinsella <mdr@ashroe.eu>; Neil Horman
> <nhorman@tuxdriver.com>;
> > >>> Ori Kam <orika@nvidia.com>; Andrew Rybchenko
> > >>> <andrew.rybchenko@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@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@intel.com>
> 
> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>

One small point, I know it was mention in some thread, can we also deprecate the id?
Maybe the entire struct? since there is not need for configuration any more.
  
Ferruh Yigit Nov. 26, 2020, 12:23 p.m. UTC | #17
On 11/26/2020 9:08 AM, Ori Kam wrote:
> Hi All
> 
>> -----Original Message-----
>> From: Ajit Khaparde <ajit.khaparde@broadcom.com>
>> Sent: Tuesday, November 24, 2020 8:10 PM
>> Subject: Re: [dpdk-dev] [PATCH] ethdev: deprecate shared counters using action
>> attribute
>>
>> On Mon, Nov 2, 2020 at 8:01 AM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>>
>>> 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@dpdk.org> On Behalf Of Andrew Rybchenko
>>>>>> Sent: Thursday, October 29, 2020 4:53 PM
>>>>>> To: dev@dpdk.org
>>>>>> Cc: Andrey Vesnovaty <andreyv@nvidia.com>; NBU-Contact-Thomas
>>>>>> Monjalon <thomas@monjalon.net>; Ferruh Yigit
>> <ferruh.yigit@intel.com>;
>>>>>> Ray Kinsella <mdr@ashroe.eu>; Neil Horman
>> <nhorman@tuxdriver.com>;
>>>>>> Ori Kam <orika@nvidia.com>; Andrew Rybchenko
>>>>>> <andrew.rybchenko@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@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@intel.com>
>>
>> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>
> 
> One small point, I know it was mention in some thread, can we also deprecate the id?
> Maybe the entire struct? since there is not need for configuration any more.
> 

There is alternative usage for 'shared' fields, that is the reason of the 
deprecation. But there are some usage of the 'id' without alternative, we can 
discuss if that usage is right or wrong later, but for now I think better to 
start with just this patch.

And if 'id' also decided to be deprecated later, yes we can remove entire struct 
in that case.
  
Ori Kam Nov. 26, 2020, 4:49 p.m. UTC | #18
> -----Original Message-----
> From: Ferruh Yigit <ferruh.yigit@intel.com>
> Sent: Thursday, November 26, 2020 2:23 PM
> Subject: Re: [dpdk-dev] [PATCH] ethdev: deprecate shared counters using action
> attribute
> > One small point, I know it was mention in some thread, can we also
> deprecate the id?
> > Maybe the entire struct? since there is not need for configuration any more.
> >
> 
> There is alternative usage for 'shared' fields, that is the reason of the
> deprecation. But there are some usage of the 'id' without alternative, we can
> discuss if that usage is right or wrong later, but for now I think better to
> start with just this patch.
> 
> And if 'id' also decided to be deprecated later, yes we can remove entire struct
> in that case.

Acked-by: Ori Kam <orika@nvidia.com>

Thanks,
Ori
  

Patch

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. */
 };