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

[Dmitry Kozlyuk <dkozlyuk@oss.nvidia.com>]

Please let's continue in the thread for the latest version of the patches:

http://inbox.dpdk.org/dev/CH0PR12MB5091792A77CBD1528DB7A005B9B79@CH0PR12MB5091.namprd12.prod.outlook.com/

Please see my comments there.

> -----Original Message-----
> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
> Sent: 12 октября 2021 г. 13:41
> To: Ori Kam <orika@nvidia.com>; Dmitry Kozlyuk <dkozlyuk@nvidia.com>; Ajit
> Khaparde <ajit.khaparde@broadcom.com>
> Cc: dpdk-dev <dev@dpdk.org>; Matan Azrad <matan@nvidia.com>; NBU-
> Contact-Thomas Monjalon <thomas@monjalon.net>; Ferruh Yigit
> <ferruh.yigit@intel.com>
> Subject: Re: [dpdk-dev] [RFC PATCH 2/2] ethdev: add capability to keep indirect
> actions on restart
> 
> External email: Use caution opening links or attachments
> 
> 
> 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?