Re: [dpdk-dev] [RFC PATCH] ethdev: add support for testpmd-compliant flow rule dumping

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

On 6/3/2021 10:27 AM, Ori Kam wrote:
> Hi Andrew,
> 
>> -----Original Message-----
>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>
>> Hi Ori,
>>
>> Cc Eli and Ilya since I think OvS could be interested in the feature.
>>
>> On 6/3/21 11:25 AM, Ori Kam wrote:
>>> Hi Andrew,
>>>
>>>> -----Original Message-----
>>>> From: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>
>>>>
>>>> Hi Ori,
>>>>
>>>> On 6/2/21 4:32 PM, Ori Kam wrote:
>>>>> Hi Ivan,
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Ivan Malov <Ivan.Malov@oktetlabs.ru>
>>>>>>
>>>>>> Hi Ori,
>>>>>>
>>>>>> Your review efforts are much appreciated. I understand your concern
>>>>>> about the partial item/action coverage, but there are some points
>>>>>> to be considered when addressing it:
>>>>>> - It's anyway hardly possible to use the printed flow directly in
>>>>>> testpmd if it contains "opaque", or "PMD-specific", items/actions
>>>>>> in terms of the tunnel offload model. These items/actions have to
>>>>>> be omitted when printing the flow, and their absence in the
>>>>>> resulting string means that copy/pasting the flow to testpmd isn't
>>>>>> helpful in this particular case.
>>>>> I fully agree with you that some of the rules can't be printed. That is why.
>>>>> I'm not sure having partial solution is the way to go.
>>>>
>>>> Sorry, I disagree that possibility to cover 99% and impossibility to
>>>> cover just 1% is the reason to discard.
>>>>
>>>
>>> I agree with you that 99% is better than 0 😊 but is this patch 99%?
>>> maybe we can agree even if it is 70% it is good for this patch.
>>
>> Hold on. Here we're talking about a theoretical possibility to cover 99%.
>> Coverage in this patch is discussed below in terms of "the most basic
>> commands".
> 
> I  know that that is why I said 70%.
> 
>> .
>>
>>>>> If OVS for example cares about
>>>>> some of the item/action, maybe this log should be on their part.
>>>>
>>>> OvS does and as far as I can see already has bugs there.
>>>> Of course, nobody says that it is testpmd-complient format, but it
>>>> definitely looks so.
>>>>
>>>> Anyway, it sounds strange do duplicate the functionality in many-many
>>>> DPDK apps. Of course, it removes the headache from DPDK maintainers,
>>>> but it is hardly friendly to DPDK community in general.
>>>>
>>>
>>> Fully agree with you that if some feature is used by number of
>>> applications, then it is better or at least nicer to have it in DPDK,
>>> but my question is that, the current patch is for the OVS use case
>>> from my understanding and not considering any other application. So, in this
>> case do we want it in DPDK?
>>
>> The primary goal, in fact, is our testing harness :) OvS is just an open source
>> example. We could easily add it to our code but decided that it would be useful
>> in DPDK since seen such messages in OvS logs.
>>
> Private application are also good examples from my point of view. 
> 
>>>>>> - There's action ENCAP which also can't be fully represented by the
>>>>>> tool in question, simply because it has no parameters. In tespmd,
>>>>>> one first has to issue "set vxlan" command to configure the encap.
>>>>>> header, whilst "vxlan" token in the flow rule string just refers to
>>>>>> the previously set encap. parameters. The suggested flow print
>>>>>> helper can't reliably print these two components ("set vxlan" and
>>>>>> the flow rule itself) as they belong to different testpmd command strings.
>>>>>>
>>>>> Again, I agree with you but like my above answer, do we want a
>>>>> partial solution in DPDK?
>>>>
>>>> My answer is YES.
>>>>
>>>
>>> I can live with such decision but like I said above it depends on the
>>> use case and how partial it is.
>>
>> See above.
>>
>>>>>> As you might see, completeness of the solution wouldn't necessarily
>>>>>> be reachable, even if full item/action coverage was provided.
>>>>>>
>>>>>> As for the item/action coverage itself, it's rather controversial.
>>>>>> On the one hand, yes, we should probably try to cover more items
>>>>>> and actions in the suggested patch, to the extent allowed by our
>>>>>> current priorities. But on the other hand, the existing coverage
>>>>>> might not be that poor: it's fairly elaborate and at least allows
>>>>>> to print the most common flow rules.
>>>>>>
>>>>> That is my main issue you are going to push something that is good
>>>>> for you and maybe some other cases, but it can't be used by all
>>>>> application, even with the most basic commands like encap.
>>>>
>>>> Isn't it fair: if one wants to use something, be ready to help and
>>>> extend it. No pain, no gain :) Jokes aside - we're ready to support
>>>> "the most basic commands", just list it, but do not say everything is
>>>> basic. "everything" will delay the feature and simply unfair to demand
>> (IMHO).
>>>>
>>>> IMHO, the feature would make flow API more friendly and easier to
>>>> debug, report bugs etc.
>>>>
>>> I fully agree that if someone wants functionality, he should work for it.
>>> But as a developer of rte_flow and maintainer I need to ask who will
>>> add the new features/missing features? Should we enforce that each
>>> developer when coding a new item/action will add it to the print?
>>> Or just users that care about such log will add it?
>>
>> It is a good and valid questions. First of all we can help with (or just completely
>> take it) to maintain the file.
> 
> The issue is not the maintaining of the file is the extra work required for each new feature.
> and what do we do with features that are hard to print for example encap data?
> 
>> Second basically any option from above is OK for me.
>> My personal preference would be to require implementation for new RTE flow
>> features. In fact, testpmd may start to use it to list created rules etc.
>> We'll try to make it easier to add new items and actions support.
>>
> I also think that the best is that all new features will be added to this print
> but the requirement is that adding new this code will not have high overhead.
> If we can find and easy way to do it, I think this will be perfect.
> 
>>> To summarize.
>>> I think the following question must be answer before deciding:
>>> 1. how many apps are going to use this feature?
>>
>> I'll keep OvS maintainers to answer if OvS would like to use it. We'll definitely
>> use it (either from DPDK or from internal code base if it is not accepted), but I
>> guess not open source projects may be not taken into account.
>>
> Agree lets see the application programmers view point on this.
> 
>>> 2. it the coverage sufficient?
>>
>> I hope yes, since it tries to warn about not supported features. I.e. it does not lie
>> simply skipping not supported bits.
>>
> 
> But then you can't copy paste it to testpmd and test, which I think is a very good
> why to debug issues that costumers may find.
> 
>>> 3. who is responsible to update it? (each developer/the interested
>>> party member?)
>>
>> See above.
>>
>> I hope to see more answers here.
>>
> +1
> 
> Best,
> Ori
> 

This is an old patch still active in patchwork [1], lets try to conclude it.

Patch is to help debugging flow API, by printing flow API rule in
testpmd syntax so that issue can be reproduced/tested using testpmd.

This support comes with some amount of code and I agree with Ori that it
is a question who will add relevant code for future flow rules, and if
it worth the overhead it brings.

Also I have additional concern that how correct to associate this kind
of support to testpmd syntax which can change or go away easily, and add
code to ethdev library that has (logical) dependency to test application.

Considering patch is sitting without update for a while and concerns
above, I am rejecting this patch.



But Aman has a good suggestion in other thread, why not record flow
rule, and later parse it with a separate tool. This tool can generate
output in testpmd syntax as well as any other format desired.
With this ethdev code becomes very small and fixed size, and parsing
code (tool) decoupled from ethdev library.

If this is interesting to you, please start another thread to discuss or
send another RFC for it.


Thanks,
ferruh


[1]
https://patches.dpdk.org/project/dpdk/patch/20210527082504.3495-1-ivan.malov@oktetlabs.ru/