Re: [dpdk-dev] [RFC PATCH 2/2] ethdev: add capability to keep indirect actions on restart

[Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>]

On 10/12/21 1:26 PM, Ori Kam wrote:
> Hi
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>> Sent: Tuesday, October 12, 2021 12:15 PM
>> Subject: Re: [dpdk-dev] [RFC PATCH 2/2] ethdev: add capability to keep indirect actions on restart
>>
>> On 10/11/21 6:53 PM, Ori Kam wrote:
>>> Hi Andrew and Ajit,
>>>
>>>> -----Original Message-----
>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>> Sent: Monday, October 11, 2021 4:58 PM
>>>> Subject: Re: [dpdk-dev] [RFC PATCH 2/2] ethdev: add capability to
>>>> keep indirect actions on restart
>>>>
>>>> On 10/7/21 11:16 AM, Dmitry Kozlyuk wrote:
>>>>>> -----Original Message-----
>>>>>> From: Ajit Khaparde <ajit.khaparde@broadcom.com>
>>>>>> Sent: 6 октября 2021 г. 20:13
>>>>>> To: Dmitry Kozlyuk <dkozlyuk@nvidia.com>
>>>>>> Cc: dpdk-dev <dev@dpdk.org>; Matan Azrad <matan@nvidia.com>; Ori
>>>>>> Kam <orika@nvidia.com>; NBU-Contact-Thomas Monjalon
>>>>>> <thomas@monjalon.net>; Ferruh Yigit <ferruh.yigit@intel.com>;
>>>>>> Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>>> Subject: Re: [dpdk-dev] [RFC PATCH 2/2] ethdev: add capability to
>>>>>> keep indirect actions on restart
>>>>>>
>>>>>> On Wed, Sep 1, 2021 at 1:55 AM Dmitry Kozlyuk <dkozlyuk@nvidia.com> wrote:
>>>>>>>
>>>>>>> rte_flow_action_handle_create() did not mention what happens with
>>>>>>> an indirect action when a device is stopped, possibly
>>>>>>> reconfigured, and started again. It is natural for some indirect
>>>>>>> actions to be persistent, like counters and meters; keeping others
>>>>>>> just saves application time and complexity. However, not all PMDs can support it.
>>>>>>> It is proposed to add a device capability to indicate if indirect
>>>>>>> actions are kept across the above sequence or implicitly destroyed.
>>>>>>>
>>>>>>> It may happen that in the future a PMD acquires support for a type
>>>>>>> of indirect actions that it cannot keep across a restart. It is
>>>>>>> undesirable to stop advertising the capability so that
>>>>>>> applications that don't use actions of the problematic type can still take advantage of it.
>>>>>>> This is why PMDs are allowed to keep only a subset of indirect
>>>>>>> actions provided that the vendor mandatorily documents it.
>>>>>> Sorry - I am seeing this late.
>>>>>> This could become confusing.
>>>>>> May be it is better for the PMDs to specify which actions are persistent.
>>>>>> How about adding a bit for the possible actions of interest.
>>>>>> And then PMDs can set bits for actions which can be persistent
>>>>>> across stop, start and reconfigurations?
>>>>>
>>>>> This approach was considered, but there is a risk of quickly running
>>>>> out of capability bits. Each action
>>>> would consume one bit plus as many bits as there are special
>>>> conditions for it in all the PMDs, because conditions are likely to
>>>> be PMD-specific. And the application will anyway need to consider
>>>> specific conditions to know which bit to test, so the meaning of the bits will be PMD-specific. On the
>> other hand, PMDs are not expected to exercise this loophole unless absolutely needed.
>>>>>
>>> Right those bits should be considered as master bits and are not per actions.
>>> If there is specific case for a PMD it should solve it by documation or other means.
>>
>> Documentation does not solve the problem since it can't be automated. So, it just help to solve case-by-
>> case.
> 
> I agree that documentation can't be automated, I think this is just like other edge cases that can't be checked
> for example you can reconfigure the device after start except the queue number or queue size (just an example)
> The metrix of actions/items/pmds I don't think we will ever be able to have an easy way to check capabilities.
> 
> Maybe we can say that if PMD reports that it supports keeping the actions, and it can't support just one of the actions
> it can fail or issue a special error code when calling stop. To let the application know that something was incorrect.
> In this case application can create a sample of the action it requires and then call the stop. If it fails it can try again until
> he gets no error, and only then start. What do you think?

It all sounds complicated. Do we really need it?

> Another way is to assume that if the action was created before port start it will be kept after port stop.

It does not sound like a solution. May be I simply don't know
target usecase.

> And this bit is just for letting the application know if it is worth to check.
>  
>>
>>>
>>>>
>>>> May be we should separate at least transfer and non-transfer rules?
>>>> Transfer rules are less configuration dependent.
>>>
>>> May be I'm missing something but jut like stated above those are
>>> master bits I don't see much use case where the PMD can store transfer
>>> rules but not other rules. I assume  that if the application uses the transfer mode most of the flows will be
>> in the transfer domain.
>>
>> Most likely different HW blocks are responsible for transfer and non-transfer rules. So, I can easily imagine
>> that one could be preserved across restart, but another can't.
>>
> 
> I don't know, but in our case this is the same block.
> since a lot of the action are the same between the eswitch and the ethdev I would expect that the limitation will be the same.
> how is it in your case?

Actions or rules? QUEUE and RSS are non-transfer actions.
PORT_ID etc are transfer actions which do not make sense in
non-transfer case. DROP, COUNT and MARK make sense in both
cases. Packet edits make sense in both cases as well.

> 
>> Anyway, I'm just trying to understand. Not a blocker.
>>
>> Also have you considered to make it controllable by the application. I.e. PMD advertises a capability and it
>> is responsibility of the application to use it or not.
>> May be it is excessive. In theory application can check the flag and do flush before or just after stop if it
>> does not want to preserve rules.
>>
> 
> I'm not sure I understand this comment, The application is always free to use or not use a capability this is 
> just to let the application know that if it doesn't want to destroy the action before stop he doesn't have to
> and the action will be saved.

Hm, who said that application must explicitly destroy
rules/actions before stop?