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

Ilya Maximets i.maximets at ovn.org
Mon Jun 29 18:10:51 UTC 2020


On 6/29/20 2:21 PM, Eli Britstein wrote:
> 
> On 6/29/2020 3:38 AM, Ilya Maximets wrote:
>> On 6/21/20 1:19 PM, Eli Britstein wrote:
>>> For OVS rule of the form "eth type is 0x1234 / end", rule is offloaded
>>> in the form of "eth / end", which is incorrect. Fix it.
>>>
>>> Fixes: e8a2b5bf92bb ("netdev-dpdk: implement flow offload with rte flow")
>>> Signed-off-by: Eli Britstein <elibr at mellanox.com>
>>> Reviewed-by: Roni Bar Yanai <roniba at mellanox.com>
>>> ---
>>>   lib/netdev-offload-dpdk.c | 5 +++--
>>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
>>> index e8b8d6464..e68b6549c 100644
>>> --- a/lib/netdev-offload-dpdk.c
>>> +++ b/lib/netdev-offload-dpdk.c
>>> @@ -860,7 +860,8 @@ parse_flow_match(struct flow_patterns *patterns,
>>>         /* Eth */
>>>       if (!eth_addr_is_zero(match->wc.masks.dl_src) ||
>>> -        !eth_addr_is_zero(match->wc.masks.dl_dst)) {
>>> +        !eth_addr_is_zero(match->wc.masks.dl_dst) ||
>>> +        match->wc.masks.dl_type) {
>> 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.

So, from that point of view it's better to remove the 'else' branch
entirely.  Good reasoning should be described in a commit message.

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.

> 
>>
>>>           struct rte_flow_item_eth *spec, *mask;
>>>             spec = xzalloc(sizeof *spec);
>>> @@ -876,6 +877,7 @@ parse_flow_match(struct flow_patterns *patterns,
>>>             memset(&consumed_masks->dl_dst, 0, sizeof consumed_masks->dl_dst);
>>>           memset(&consumed_masks->dl_src, 0, sizeof consumed_masks->dl_src);
>>> +        consumed_masks->dl_type = 0;
>>>             add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, spec, mask);
>>>       } else {
>>> @@ -888,7 +890,6 @@ parse_flow_match(struct flow_patterns *patterns,
>>>            */
>>>           add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_ETH, NULL, NULL);
>> Actually looking at i40e_flow.c, it seems like this pattern will be rejected.
>> But I'm not sure.
>>
>>>       }
>>> -    consumed_masks->dl_type = 0;
>>>         /* VLAN */
>>>       if (match->wc.masks.vlans[0].tci && match->flow.vlans[0].tci) {
>>>



More information about the dev mailing list