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 <el...@mellanox.com> >>> Reviewed-by: Roni Bar Yanai <ron...@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) { >>> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev