On 6/29/2020 9:10 PM, Ilya Maximets wrote:
While calling from the userspace datapath, we always have a match on dl_type.
This means that it's not possible to hit the 'else' condition.
I'm worried about the usecase described there. It's hard to track changes
in i40e_flow.c. Can someone test this with XL710?
Yes, that's true. I think we should have it for consistency with the rest of
the code, and also just in case.
Regarding the XL710 comment - I think it should not have been merged into OVS
in the first place. If it is an issue with XL710, the relevant PMD should
handle it, and not OVS. I just kept it in case I miss something here. Do you
think it's OK to remove it?
Thinking about correctness, if dl_type match requested and we're
matching any L2 packets instead, this does not sound good. In
this case we might perform incorrect set of actions for a packet
that should be handled differently. Also, we might end up having
several flows with the same match and different actions in case the
only dufference was in dl_type, which is not a good thing too.
This commit addresses exactly this issue, and applies specific ETH
matches if applicable, including dl_type.
So, from that point of view it's better to remove the 'else' branch
entirely. Good reasoning should be described in a commit message.
I don't see how that implies removing the else entirely. If there is
from some reason (though won't happen at least currently) no matches on
any eth (src/dst/type), it does make sense to match on generic "ETH".
For completeness I'd prefer to keep the else branch, but maybe drop only
the comment about XL710. What do you think?
For the 'if' condition: You could do 'masks.dl_type' check first,
i.e. before checking eth addresses, this way we will save a few cpu
cycles in most cases.
OK.
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
