Hi Andrew,

-----Original Message-----
From: Andrew Rybchenko <redacted>

Hi Ori,

On 6/2/21 4:32 PM, Ori Kam wrote:

quoted

Hi Ivan,

quoted

-----Original Message-----
From: Ivan Malov <redacted>

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.

quoted

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?

quoted

quoted

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

quoted

quoted

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?


To summarize.
I think the following question must be answer before deciding:
1. how many apps are going to use this feature?
2. it the coverage sufficient?
3. who is responsible to update it? (each developer/the interested party member?)


Best,
Ori
 

quoted

quoted

Yes, macros and some other cunning ways to cover more flow specifics
might come in handy, but, at the same time, can be rather error prone.
Sometimes it's more robust to just write the code out in full.

I'm always in favor of easy of extra complex but too hard is also not good.

Thanks,
Ori

quoted

Thank you.

On 30/05/2021 10:27, Ori Kam wrote:

quoted

Hi Ivan,

First nice idea and thanks for the picking up the ball.

Before a detail review,
The main thing I'm concerned about is that this print will be
partially

supported,

quoted

I know that you covered this issue by printing unknown for
unsupported

item/actions,

quoted

but this will mean that it is enough that one item/action is not
supported

and already the

quoted

flow can't be used in testpmd.
To get full support it means that the developer needs to add such
print

with each new

quoted

item/action. I agree it is possible, but it has high overhead for each

feature.

quoted

quoted

quoted

Maybe we should somehow create a macros for the prints or other
easier

to support ways.

quoted

For example, just printing the ipv4 has 7 function calls inside of
it each one

with error checking,

quoted

and I'm not counting the dedicated functions.



Best,
Ori

quoted

-----Original Message-----
From: Ivan Malov <redacted>
Sent: Thursday, May 27, 2021 11:25 AM
To: dev@dpdk.org
Cc: NBU-Contact-Thomas Monjalon <redacted>; Ferruh

Yigit

quoted

quoted

[off-list ref]; Andrew Rybchenko
[off-list ref]; Ori Kam [off-list ref]; Ray
Kinsella [off-list ref]; Neil Horman [off-list ref]
Subject: [RFC PATCH] ethdev: add support for testpmd-compliant flow

rule

quoted

quoted

dumping

DPDK applications (for example, OvS) or tests which use RTE flow
API

need to

quoted

quoted

log created or rejected flow rules to help to recognise what goes
right or wrong. From this standpoint, testpmd-compliant format is
nice for the purpose because it allows to copy-paste the flow rules
and debug using testpmd.

Recognisable pattern items:
VOID, VF, PF, PHY_PORT, PORT_ID, ETH, VLAN, IPV4, IPV6, UDP, TCP,

VXLAN,

quoted

quoted

NVGRE, GENEVE, MARK, PPPOES, PPPOED.

Recognisable actions:
VOID, JUMP, MARK, FLAG, QUEUE, DROP, COUNT, RSS, PF, VF,

PHY_PORT,

quoted

quoted

PORT_ID, OF_POP_VLAN, OF_PUSH_VLAN, OF_SET_VLAN_VID,
OF_SET_VLAN_PCP, VXLAN_ENCAP, VXLAN_DECAP.

Recognisable RSS types (action RSS):
IPV4, FRAG_IPV4, NONFRAG_IPV4_TCP, NONFRAG_IPV4_UDP,
NONFRAG_IPV4_OTHER, IPV6, FRAG_IPV6, NONFRAG_IPV6_TCP,
NONFRAG_IPV6_UDP, NONFRAG_IPV6_OTHER, IPV6_EX,

IPV6_TCP_EX,

quoted

quoted

quoted

quoted

IPV6_UDP_EX, L3_SRC_ONLY, L3_DST_ONLY, L4_SRC_ONLY,

L4_DST_ONLY.

quoted

quoted

Unrecognised parts of the flow specification are represented by
tokens "{unknown}" and "{unknown bits}". Interested parties are
welcome to extend this tool to recognise more items and actions.

Signed-off-by: Ivan Malov <redacted>