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

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 2/17/2021 2:10 PM, Thomas Monjalon wrote:
> 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
> 

This is an old patch, and there was no update/response during the release 
time-frame.
I am rejecting in patchwork, please send a new version if there is still demand 
for it.