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

[Andrew Rybchenko <arybchenko@solarflare.com>]

On 11/6/19 10:42 AM, Ori Kam wrote:
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>> Sent: Wednesday, November 6, 2019 8:41 AM
>> To: Ori Kam <orika@mellanox.com>; Thomas Monjalon
>> <thomas@monjalon.net>
>> Cc: dev@dpdk.org; pbhagavatula@marvell.com; ferruh.yigit@intel.com;
>> jerinj@marvell.com; John McNamara <john.mcnamara@intel.com>; Marko
>> Kovacevic <marko.kovacevic@intel.com>; Adrien Mazarguil
>> <adrien.mazarguil@6wind.com>; david.marchand@redhat.com;
>> ktraynor@redhat.com
>> Subject: Re: [dpdk-dev] [PATCH 1/2] ethdev: add flow action type update as an
>> offload
>>
>> On 11/5/19 7:37 PM, Ori Kam wrote:
>>>
>>>> -----Original Message-----
>>>> From: Andrew Rybchenko <arybchenko@solarflare.com>
>>>> Sent: Tuesday, November 5, 2019 1:31 PM
>>>> To: Ori Kam <orika@mellanox.com>; Thomas Monjalon
>>>> <thomas@monjalon.net>
>>>> Cc: dev@dpdk.org; pbhagavatula@marvell.com; ferruh.yigit@intel.com;
>>>> jerinj@marvell.com; John McNamara <john.mcnamara@intel.com>; Marko
>>>> Kovacevic <marko.kovacevic@intel.com>; Adrien Mazarguil
>>>> <adrien.mazarguil@6wind.com>; david.marchand@redhat.com;
>>>> ktraynor@redhat.com
>>>> Subject: Re: [dpdk-dev] [PATCH 1/2] ethdev: add flow action type update as an
>>>> offload
>>>>
>>>
>>> [Snip]
>>>
>>>>>
>>>>> Yes but like I said in Mellanox PMD for example we supported the mark only on non-transfer flows until this release.
>>>>> so when the user set mark on transfer flow it was invalid. (in transfer flow if we have a miss we send the packet back to the Rx
>>>>> port so the application can understand on which table the miss happened)
>>>>> In this version we added the support for mark also in transfer (E-Switch) flows.
>>>>> So my question before this release what should the PMD report? What should the PMD report after this release?
>>>>>
>>>>> Your idea was our first thought when adding the Tx meta, in that case the meta was always set in application
>>>>> so we thought that this offload will enable us better function selection, but as you know we removed this capability
>>>>> since it is not correct any more.
>>>>>
>>>>>> The above also highlights problems of the meta vs mark design. They are very
>>>>>> similar and there is no any good definition of the difference and rules
>>>>>> which one should be used/supported in which conditions.
>>>>>>
>>>>>
>>>>> Mark and Meta are exactly the same, the meta is just another value that the application can use.
>>>>> This is why both should act the same.
>>>>>
>>>>> And maybe this is the wining argument, the rte_flow validation approach was used and accepted for the meta.
>>>>> So lets try it also with the mark. (please also remember that we didn't have this mark until now to somehow the
>>>>> PMD worked 😊)
>>>>>
>>>>> Like I said before, I understand your approach, and each one of them has its own advantages and draw backs.
>>>>> Lets start using the rte_flow approach and see how it goes, I promise you that if I see that it doesn't scale or cause more
>>>>> issues I will be first one to submit changes.
>>>>
>>>> I tend to say OK, let's try. However, it must be documented
>>>> in MARK action that if an application wants to use it, a rule
>>>> with the action must be validated before device start.
>>>
>>> I agree to add this to the rte_flow mark action documation.
>>>
>>>> PMD may use the attempt as an indication from the application
>>>> that it would like to use flow mark even if the validation
>>>> fails.
>>>
>>> No if the PMD uses this validation as hint it should return success and
>>> use the correct PMD.
>>
>> It would make it too strictly dependent on pattern/actions/state.
> 
> I'm not sure I understand your comment.
> Why would it make it to strict? I guess that mark action is doesn't care which items are in 
> the flow, so just setting eth item sound good enough.

The problem is if single pattern+actions is used to check MARK
support and let PMD know that application is going to use it and
the flow rule is not supported, but another combination is supported,
that's it. The feature is false classified as not supported.

> Also I guess the mark doesn't conflict with many actions maybe the meta.
> So the application doesn't have a lot of choices.
> In any case and this is the most important one. In any case the application must validate that the 
> Nic support the mark in any case, according to RTE_FLOW definition. What is the meaning
> of saying we support offload (using your segguetion) and then each flow you try to insert
> will fail. So in any case the application must validate the flows it is going to use.

Yes, but the decision here is per rule, not global.

>>>> Ori, please, suggest formalized pattern and actions
>>>> specification to use if application wants to utilize
>>>> validation result as a criteria to enable/disable flow
>>>> marks usage.
>>>
>>> I can’t do that, it depends on the application, the most basic is just "pattern eth actions mark / queue" .
>>> In some cases where you need it for E-Switch if should be something like"transfer  items port / eth / actions mark"
>>
>> If so, what application author should do if even maintainers cannot
>> formalize it. It sounds like the approach does not work.
> 
> This should be very easy for the application to know. How can I a mantianer know 
> what flows and what is important to the application, I need to make sure that the application
> can understand if what it wants is supported.

Yes and it is the problem of complex logic to make global
decision at start of day. Flow API allows to avoid global
decisions and verdict is returned for each specific rule,
but here we need global decision which will affect further
logic.

>>>> What should be done if patterns to use and set
>>>> of actions together with MARK are not known in advance.
>>>
>>> I think that the application knows which kind of traffic it expects and which actions it needs.
>>
>> I'm afraid it is not always true.
> 
> Can you please give me such example, how can you develop an application without knowing what it should do?
> This means the application is not optimized and impossible to test.

First of all testpmd. I know that it is a test application etc,
but I think it is still valid example.
Second OVS, if I understand it correctly. As far as I know it
allows to add a rule with various patterns and, in theory, it
could result in the pattern to be used with MARK action.
Basically any application which allows to add custom flow rules.

I think we are running in circles here. I'll try to summarize
the result of the discussion to make it easier for other
developers to follow. Please, correct me if I'm wrong below.

Sorry for a very-very long mail below, but I'd really like
to finalize the discussion.

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)

Discussed solutions:
~~~~~~~~~~~~~~~~~~~~
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.

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.

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

Andrew.