Re: [dpdk-dev] [PATCH 1/2] ethdev: add flow action type update as an offload

[Thomas Monjalon <thomas@monjalon.net>]

17/02/2021 14:45, Ferruh Yigit:
> On 7/3/2020 3:34 PM, Ferruh Yigit wrote:
> > On 11/19/2019 11:09 AM, Thomas Monjalon wrote:
> >> 19/11/2019 11:59, Andrew Rybchenko:
> >>> On 11/19/19 12:50 PM, Thomas Monjalon wrote:
> >>>> 19/11/2019 10:24, Andrew Rybchenko:
> >>>>> On 11/8/19 4:30 PM, Thomas Monjalon wrote:
> >>>>>> 08/11/2019 14:27, Andrew Rybchenko:
> >>>>>>> On 11/8/19 4:17 PM, Thomas Monjalon wrote:
> >>>>>>>> 08/11/2019 13:00, Andrew Rybchenko:
> >>>>>>>>> On 11/8/19 2:03 PM, Thomas Monjalon wrote:
> >>>>>>>>>> 08/11/2019 11:42, Andrew Rybchenko:
> >>>>>>>>>>> On 11/8/19 1:28 PM, Thomas Monjalon wrote:
> >>>>>>>>>>>> 08/11/2019 09:35, Andrew Rybchenko:
> >>>>>>>>>>>>> The problem:
> >>>>>>>>>>>>> ~~~~~~~~~~~~
> >>>>>>>>>>>>> PMD wants to know before port start if application wants to
> >>>>>>>>>>>>> to use flow MARK/FLAG in the future. It is required because:
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> 1. HW may be configured in a different way to reserve resources
> >>>>>>>>>>>>>     for MARK/FLAG delivery
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> 2. Datapath implementation choice may depend on it (e.g. vPMD
> >>>>>>>>>>>>>     is faster, but does not support MARK)
> >>>>>>>>>>>> Thank you for the clear problem statement.
> >>>>>>>>>>>> I agree with it. This is a real design issue.
> >>>>>>>>>>>>
> >>>>>>>>>>>>
> >>>>>>>>>>>>> Discussed solutions:
> >>>>>>>>>>>>> ~~~~~~~~~~~~~~~~~~~~
> >>>>>>>>>>> May be it is not 100% clear since below are alternatives.
> >>>>>>>>>>>
> >>>>>>>>>>>>> A. Explicit Rx offload suggested by the patch.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> B. Implicit by validation of a flow rule with MARK/FLAG actions used.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> C. Use dynamic field/flag (i.e. application registers dynamic field
> >>>>>>>>>>>>>     and/or flag and PMD uses lookup to solve the problem) plus part
> >>>>>>>>>>>>>     of (B) to discover if a feature is supported.
> >>>>>>>>>>>> The dynamic field should be registered via a new API function
> >>>>>>>>>>>> named '<feature>_init'.
> >>>>>>>>>>>> It means the application must explicit request the feature.
> >>>>>>>>>>>> I agree this is the way to go.
> >>>>>>>>>>> If I understand your statement correctly, but (C) is not ideal since it
> >>>>>>>>>>> looks global. If registered dynamic field of mbuf and is flag that
> >>>>>>>>>>> the feature should be enabled, it is a flag to all ports/devices.
> >>>>>>>>>>>
> >>>>>>>>>>>>> All solutions require changes in applications which use these
> >>>>>>>>>>>>> features. There is a deprecation notice in place which advertises
> >>>>>>>>>>>>> DEV_RX_OFFLOAD_FLOW_MARK addition, but may be it is OK to substitute
> >>>>>>>>>>>>> it with solution (B) or (C). Solution (C) requires changes since
> >>>>>>>>>>>>> it should be combined with (B) in order to understand if
> >>>>>>>>>>>>> the feature is supported.
> >>>>>>>>>>>> I don't understand.
> >>>>>>>>>>>> Application request and PMD support are two different things.
> >>>>>>>>>>>> PMD support must be via rte_flow validation on a case by case anyway.
> >>>>>>>>>>> I mean that application wants to understand if the feature is
> >>>>>>>>>>> supported. Then, it wants to enable it. In the case of (B),
> >>>>>>>>>>> if I understand the solution correctly, there is no explicit
> >>>>>>>>>>> way to enable, PMD just detects it because of discovery is done
> >>>>>>>>>>> (that's what I mean by "implicit" and it is a drawback from my
> >>>>>>>>>>> point of view, but still could be considered). (C) solves the
> >>>>>>>>>>> problem of (B).
> >>>>>>>>>>>
> >>>>>>>>>>>>> Advantages and drawbacks of solutions:
> >>>>>>>>>>>>> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>>>>>>>>>>>> 1. The main drawback of (A) is a "duplication" since we already
> >>>>>>>>>>>>>     have a way to request flow MARK using rte_flow API.
> >>>>>>>>>>>>>     I don't fully agree that it is a duplication, but I agree
> >>>>>>>>>>>>>     that it sounds like duplication and complicates a bit flow
> >>>>>>>>>>>>>     MARK usage by applications. (B) complicates it as well.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> 2. One more drawback of the solution (A) is the necessity of
> >>>>>>>>>>>>>     similar solution for META and it eats one more offload bit.
> >>>>>>>>>>>>>     Yes, that's true and I think it is not a problem.
> >>>>>>>>>>>>>     It would make it easier for applications to find out if
> >>>>>>>>>>>>>     either MARK or META is supported.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> 3. The main advantage of the solution (A) is simplicity.
> >>>>>>>>>>>>>     It is simple for application to understand if it supported.
> >>>>>>>>>>>>>     It is simple in PMD to understand that it is required.
> >>>>>>>>>>>>>     It is simple to disable it - just reconfigure.
> >>>>>>>>>>>>>     Also it is easier to document it - just mention that
> >>>>>>>>>>>>>     the offload should be supported and enabled.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> 4. The main advantage of the solution (B) is no "duplication".
> >>>>>>>>>>>>>     I agree that it is valid argument. Solving the problem
> >>>>>>>>>>>>>     without extra entities is always nice, but unfortunately
> >>>>>>>>>>>>>     it is too complex in this case.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> 5. The main drawback of the solution (B) is the complexity.
> >>>>>>>>>>>>>     It is necessary to choose a flow rule which should be used
> >>>>>>>>>>>>>     as a criteria. It could be hardware dependent.
> >>>>>>>>>>>>>     Complex logic is require in PMD if it wants to address the
> >>>>>>>>>>>>>     problem and control MARK delivery based on validated flow
> >>>>>>>>>>>>>     rules. It adds dependency between start/stop processing and
> >>>>>>>>>>>>>     flow rules validation code.
> >>>>>>>>>>>>>     It is pretty complicated to document it.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> 6. Useless enabling of the offload in the case of solution (A)
> >>>>>>>>>>>>>     if really used flow rules do not support MARK looks like
> >>>>>>>>>>>>>     drawback as well, but easily mitigated by a combination
> >>>>>>>>>>>>>     with solution (B) and only required if the application wants
> >>>>>>>>>>>>>     to dive in the level of optimization and complexity and
> >>>>>>>>>>>>>     makes sense if application knows required flow rules in
> >>>>>>>>>>>>>     advance. So, it is not a problem in this case.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> 7. Solution (C) has drawbacks of the solution (B) for
> >>>>>>>>>>>>>     applications to understand if these features are supported,
> >>>>>>>>>>>>>     but no drawbacks in PMD, since explicit criteria is used to
> >>>>>>>>>>>>>     enable/disable (dynamic field/flag lookup).
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> 8. Solution (C) is nice since it avoids "duplication".
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> 9. The main drawback of the solution (C) is asymmetry.
> >>>>>>>>>>>>>     As it was discussed in the case of RX_TIMESTAMP
> >>>>>>>>>>>>>     (if I remember it correctly):
> >>>>>>>>>>>>>      - PMD advertises RX_TIMESTAMP offload capability
> >>>>>>>>>>>>>      - application enables the offload
> >>>>>>>>>>>>>      - PMD registers dynamic field for timestamp
> >>>>>>>>>>>>>     Solution (C):
> >>>>>>>>>>>>>       - PMD advertises nothing
> >>>>>>>>>>>>>       - application uses solution (B) to understand if
> >>>>>>>>>>>>>         these features are supported
> >>>>>>>>>>>>>       - application registers dynamic field/flag
> >>>>>>>>>>>>>       - PMD does lookup and solve the problem
> >>>>>>>>>>>>>     The asymmetry could be partially mitigated if RX_TIMESTAMP
> >>>>>>>>>>>>>     solution is changed to require an application to register
> >>>>>>>>>>>>>     dynamic fields and PMD to do lookup if the offload is
> >>>>>>>>>>>>>     enabled. So, the only difference will be in no offload
> >>>>>>>>>>>>>     in the case of flow MARK/FLAG and usage of complex logic
> >>>>>>>>>>>>>     to understand if it is supported or no.
> >>>>>>>>>>>>>     May be it would be really good since it will allow to
> >>>>>>>>>>>>>     have dynamic fields registered before mempool population.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> 10. Common drawback of solutions (B) and (C) is no granularity.
> >>>>>>>>>>>>>      Solution (A) may be per queue while (B) and (C) cannot be
> >>>>>>>>>>>>>      per queue. Moreover (C) looks global - for all devices.
> >>>>>>>>>>>>>      It could be really painful.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> (C) is nice, but I still vote for simplicity and
> >>>>>>>>>>>>> granularity of (A).
> >>>>>>>>>>>> I vote for clear separation of application needs and PMD support,
> >>>>>>>>>>>> by using the method C (dynamic fields).
> >>>>>>>>>>>> I agree timestamp must use the same path.
> >>>>>>>>>>>> I agree it's complicate because we don't know in advance whether
> >>>>>>>>>>>> a flow rule will be accepted, but that's the reality, config is complex.
> >>>>>>>>>>> Do you think that global nature of the (C) is acceptable?
> >>>>>>>>>> That's a good question.
> >>>>>>>>>> Maybe the feature request should be per port.
> >>>>>>>>>> In this case, we are back to solution A with a flag per port?
> >>>>>>>>> Offloads are natively per-queue as well, so (A) keeps the choice
> >>>>>>>>> between per-port vs per-queue to PMDs as usual.
> >>>>>>>>>
> >>>>>>>>>> Note that A and C will not guarantee that the offload will be possible.
> >>>>>>>>> Yes, definitely.
> >>>>>>>>>
> >>>>>>>>>> We need B (flow rule validation) anyway.
> >>>>>>>>> Strictly speaking (B) (checking flow rules before device
> >>>>>>>>> startup) is required if an application can predict flow
> >>>>>>>>> rules and wants to ensure that MARK offload will be usable.
> >>>>>>>>> Otherwise, it may be skipped.
> >>>>>>>> No no, I mean flow rule validation MUST be used anyway
> >>>>>>>> during the runtime before applying a rule.
> >>>>>>>> I agree it is hard to predict. I speak only about real rules.
> >>>>>>> OK, I see. Of course, flow rule validation is required at runtime.
> >>>>>>> I was rather concentrated on the stated problem solutions.
> >>>>>>>
> >>>>>>>>>> It seems A, B, C are not alternatives but all required
> >>>>>>>>>> as pieces of a puzzle...
> >>>>>>>>> Unfortunately true in the most complex case.
> >>>>>>>>> Right now it will be A with B if required as explained above.
> >>>>>>>>> C will come a bit later when the field migrates to dynamic.
> >>>>>>>>>
> >>>>>>>>> May be it is even better if application registers dynamic
> >>>>>>>>> fields before an attempt to enable offload to be sure that
> >>>>>>>>> it will not fail because of impossibility to register
> >>>>>>>>> dynamic field (lack of space). I'm not sure, but it is not
> >>>>>>>>> not that important.
> >>>>>>>> Yes of course, lack of mbuf space is another reason for
> >>>>>>>> disabling the feature.
> >>>>>>>>
> >>>>>>>>> If we finally go way A, should we add offloads for META back?
> >>>>>>>>> I guess separate Rx and Tx are required.
> >>>>>>>> I would prefer to add it as dynamic flags.
> >>>>>>>> Why rushing on a very temporary solution while it is not a new issue?
> >>>>>>> Basically it means that we go just (B)+(C) in the case of META.
> >>>>>>> I have no strong opinion but thought that it could be better to
> >>>>>>> align the solution. Of course, we can wait with it. As I understand
> >>>>>>> META is an experimental feature.
> >>>>>> Yes it is experimental and I think it is too late to align now.
> >>>>>>
> >>>>>> Anyway, we will probably to discuss again these offloads TAG/MARK/META,
> >>>>>> as requested by several people.
> >>>>>>
> >>>>> The series implements (A) to help to solve the problem described above.
> >>>>> What is the fate of the series in v19.11 in accordance with the
> >>>>> discussion?
> >>>> I am against adding anything related to a feature union'ed in mbuf.
> >>>> The feature must move to dynamic field first.
> >>>>
> >>>> In addition, such capability is very weak.
> >>>> I am not sure it is a good idea to have some weak capabilities,
> >>>> meaning a feature could be available but not in all cases.
> >>>> I think we should discuss more generally how we want to handle
> >>>> the rte_flow capabilities conveniently and reliably.
> >>>
> >>> It is really unexpected outcome from the above discussion.
> >>
> >> I'm sorry, I thought I was clear in my request to switch to dynamic first.
> >>
> >>
> >>> It is just possibility to deliver and handle marks on datapath and
> >>> request to have it. It says almost nothing about rte_flow rules
> >>> supported etc. I'll be happy to take part in the discussion.
> >>>
> >>>> So regarding 19.11, as this feature is not new, it can wait 20.02.
> >>>
> >>> OK, it is not critical for me, so I don't mind, however, I've seen
> >>> patches which try to use it [1] except net/octeontx2 in the second
> >>> patch of the series.
> >>>
> >>> [1] https://patches.dpdk.org/patch/62415/
> >>
> > 
> > Sorry, I have to resurrect this old (long) discussion because the patches are
> > still active in the patchwork [1] and the deprecation notice is still there [2].
> > 
> > Andrew has a good summary in the thread [3], after a year nothing seems changed.
> > 
> > 
> > Pavan, Thomas, Andrew, Ori,
> > 
> > What is our plan with this series, lets try to have a conclusion.
> > 
> > 
> > 
> > 
> > [1]
> > https://patches.dpdk.org/user/todo/dpdk/?series=7076
> > 
> > [2]
> > http://lxr.dpdk.org/dpdk/v20.05/source/doc/guides/rel_notes/deprecation.rst#L88
> > 
> > [3]
> > http://inbox.dpdk.org/dev/f170105b-9c60-1b04-cb18-52e0951ddcdb@solarflare.com/
> > 
> > 
> 
> I re-read the thread, will try to have a little movement while we are in the new 
> release cycle, if there is no update I am planning to reject the patches.
> 
> There seems two problems:
> 
> P1) Application will keep trying to program NIC for MARK action for each flow, 
> since application doesn't know if next one will succeed or not.
> If only there would be a way to find out that NIC/PMD doesn't support the MARK 
> action at all, this could save application to keep trying.
> 
> P2) PMD can make better internal choices if it gets more hint from application 
> about MARK action may be used or not.
> Application at least may say it won't use the MARK flow action at all.
> 
> 
> This patch uses offload flags infrastructure to solve above two problems, 
> solution (A) in Andrew's summary.
> 
> Although it may solve the issues, there are questions/concerns around using this 
> additional flag to control flow API, I also agree it may be confusing in the 
> design level although practically using flags can be simple.
> And this is not generic solution, what happen with META action question is 
> already hanging on in the thread, more flags? How many more can we add?
> 
> And also there is option an to use dynamic mbuf flags to detect the capability, 
> solution (C) in Andrew's summary, again it may solve the problem but it looks 
> again a workaround to solve same flow API design restriction, and this one is 
> not as simple as (A).
> 
> Overall the discussion seems going on circles without an agreed on decision.
> 
> 
> 
> What about trying to solve this with flow API return values,
> 
> If a flow rule is not supported at all by the NIC/PMD, it may return 
> '-ENO_WAY_JOSE', and application knows it can't be used at all, this may solve 
> the (P1) above.

I like it, but who is Jose?
We can also have a function to test if an action is supported or not at all.

> And if a flow rule can be supported for the given pattern, but it is not 
> supported right now because current configuration or resourcing restrictions 
> doesn't allow creating rule, a special error type can be returned with a 
> descriptive error log for application to response:
> -ECONFLICT, "Can't enable rule A when rule B is enabled"
> -EDATAPATH, "Can't enable this rule when vector datapath is used"
> -ERESOURCE, "Can't enable more than 3 rules"
> This may solve the (P2) partially.
> 
> I am not sure about second part, but at least first part shouldn't be too hard 
> to implement, and it is a generic solution, what do you think?

+1