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

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

[Andrew Rybchenko]

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

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

[Ori Kam]

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

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

[Thomas Monjalon]

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.

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

[Xueming(Steven) Li]

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

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

[Ori Kam]

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

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

[Andrew Rybchenko]

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.

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

[Ferruh Yigit]

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>

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

[Slava Ovsiienko]

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

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

[Andrew Rybchenko]

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

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

[Slava Ovsiienko]

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

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

[Andrew Rybchenko]

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

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

[Xueming(Steven) Li]

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

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

[Xueming(Steven) Li]

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

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

[Andrew Rybchenko]

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>
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
Cc: Ori Kam <orika@nvidia.com>
Cc: Xueming(Steven) Li <xuemingl@mellanox.com>
Cc: Slava Ovsiienko <viacheslavo@nvidia.com>

Right now it is suggested do not touch ID and keep it as is.
Anyway we have plenty of time before 21.11 when it can be removed.
However it is important to highlight the right direction for shared
counters now.

 doc/guides/prog_guide/rte_flow.rst   | 17 ++++++++++-------
 doc/guides/rel_notes/deprecation.rst |  4 ++++
 lib/librte_ethdev/rte_flow.h         |  6 +++++-
 3 files changed, 19 insertions(+), 8 deletions(-)

diff --git a/doc/guides/prog_guide/rte_flow.rst b/doc/guides/prog_guide/rte_flow.rst
index ea203e0..64b533b 100644
--- a/doc/guides/prog_guide/rte_flow.rst
+++ b/doc/guides/prog_guide/rte_flow.rst
@@ -1724,17 +1724,20 @@ that counter.
 For ports within the same switch domain then the counter id namespace extends
 to all ports within that switch domain.
 
+The shared flag is DEPRECATED and ``SHARED`` ``COUNT`` action should be used
+to make shared counters.
+
 .. _table_rte_flow_action_count:
 
 .. table:: COUNT
 
-   +------------+---------------------+
-   | Field      | Value               |
-   +============+=====================+
-   | ``shared`` | shared counter flag |
-   +------------+---------------------+
-   | ``id``     | counter id          |
-   +------------+---------------------+
+   +------------+---------------------------------+
+   | Field      | Value                           |
+   +============+=================================+
+   | ``shared`` | DEPRECATED, shared counter flag |
+   +------------+---------------------------------+
+   | ``id``     | counter id                      |
+   +------------+---------------------------------+
 
 Query structure to retrieve and reset flow rule counters:
 
diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index c2d3457..7f3c2e7 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -122,6 +122,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 a8eac4d..2bb93d2 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. */
 };
-- 
1.8.3.1

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

[Thomas Monjalon]

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

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

[Andrew Rybchenko]

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.

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

[Ferruh Yigit]

On 11/3/2020 2:26 PM, Andrew Rybchenko wrote:
> 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>
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
> Cc: Ori Kam <orika@nvidia.com>
> Cc: Xueming(Steven) Li <xuemingl@mellanox.com>
> Cc: Slava Ovsiienko <viacheslavo@nvidia.com>
> 

Ping for this deprecation notice.

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

[Ferruh Yigit]

On 11/12/2020 1:21 PM, Ferruh Yigit wrote:
> On 11/3/2020 2:26 PM, Andrew Rybchenko wrote:
>> 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>
>> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
>> ---
>> Cc: Ori Kam <orika@nvidia.com>
>> Cc: Xueming(Steven) Li <xuemingl@mellanox.com>
>> Cc: Slava Ovsiienko <viacheslavo@nvidia.com>
>>
> 
> Ping for this deprecation notice.
> 

For this one too added a few more people, removal target is 21.11, but it also 
marks the fields as deprecated, so it should be merged in this release.

Can you please review this deprecation notice?

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

[Ajit Khaparde]

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>

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

[Ori Kam]

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.


 

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

[Ferruh Yigit]

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.

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

[Ori Kam]

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

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

[Bruce Richardson]

On Thu, Nov 12, 2020 at 01:21:20PM +0000, Ferruh Yigit wrote:
> On 11/3/2020 2:26 PM, Andrew Rybchenko wrote:
> > 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>
> > Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> > ---
> > Cc: Ori Kam <orika@nvidia.com>
> > Cc: Xueming(Steven) Li <xuemingl@mellanox.com>
> > Cc: Slava Ovsiienko <viacheslavo@nvidia.com>
> > 
> 
> Ping for this deprecation notice.
> 
I believe this patch v2 is largely the same a v1 just with some added docs,
right? In which case the acks on v1 from Ajit and Ori should be kept, no?

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

[Bruce Richardson]

On Thu, Nov 12, 2020 at 01:21:20PM +0000, Ferruh Yigit wrote:
> On 11/3/2020 2:26 PM, Andrew Rybchenko wrote:
> > 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>
> > Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> > ---
> > Cc: Ori Kam <orika@nvidia.com>
> > Cc: Xueming(Steven) Li <xuemingl@mellanox.com>
> > Cc: Slava Ovsiienko <viacheslavo@nvidia.com>
> > 
> 
> Ping for this deprecation notice.
> 
Reading through the patch history, seems the consensus is that this is
reasonable.

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

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

[Ajit Khaparde]

On Fri, Nov 27, 2020 at 8:08 AM Bruce Richardson <bruce.richardson@intel.com>
wrote:

> On Thu, Nov 12, 2020 at 01:21:20PM +0000, Ferruh Yigit wrote:
> > On 11/3/2020 2:26 PM, Andrew Rybchenko wrote:
> > > 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>
> > > Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> > > ---
> > > Cc: Ori Kam <orika@nvidia.com>
> > > Cc: Xueming(Steven) Li <xuemingl@mellanox.com>
> > > Cc: Slava Ovsiienko <viacheslavo@nvidia.com>
> > >
> >
> > Ping for this deprecation notice.
> >
> Reading through the patch history, seems the consensus is that this is
> reasonable.
>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
>
Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>

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

[Thomas Monjalon]

> > > > 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>
> > > > Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> > > > ---
> > > > Cc: Ori Kam <orika@nvidia.com>
> > > > Cc: Xueming(Steven) Li <xuemingl@mellanox.com>
> > > > Cc: Slava Ovsiienko <viacheslavo@nvidia.com>
> > > >
> > >
> > > Ping for this deprecation notice.
> > >
> > Reading through the patch history, seems the consensus is that this is
> > reasonable.
> >
> > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> >
> Acked-by: Ajit Khaparde <ajit.khaparde@broadcom.com>

Applied, thanks