[ovs-dev] [PATCH V3 12/12] netdev-offload-dpdk: Fix Ethernet matching for type only

Eli Britstein elibr at mellanox.com
Tue Jun 30 04:43:13 UTC 2020


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


More information about the dev mailing list