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

Why? Query refers to a counter using action pointer. There is always one
counter in one action. If you need more counters, just use more actions.

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