| Message ID | 20200621111924.12397-13-elibr@mellanox.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | netdev datapath offload: Support IPv6 and VXLAN encap | expand |
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@mellanox.com> > Reviewed-by: Roni Bar Yanai <roniba@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? > 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) { >
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@mellanox.com> >> Reviewed-by: Roni Bar Yanai <roniba@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? > >> 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) { >>
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@mellanox.com> >>> Reviewed-by: Roni Bar Yanai <roniba@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) { >>>
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. >
On 6/30/20 6:43 AM, Eli Britstein wrote: > > 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? AFAIU, rte_flow allows to not specify RTE_FLOW_ITEM_TYPE_ETH. So, I'm not sure why we need to add a match on RTE_FLOW_ITEM_TYPE_ETH if not requested by upper layers. You could remove the current comment, but you will need to add another one to justify adding ETH item while it's not requested. Will HW drop all the packets if we don't have ETH pattern item? That might be a good justification (still HW/PMD specific, though). >> 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.
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) { 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); } - consumed_masks->dl_type = 0; /* VLAN */ if (match->wc.masks.vlans[0].tci && match->flow.vlans[0].tci) {
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@mellanox.com> Reviewed-by: Roni Bar Yanai <roniba@mellanox.com> --- lib/netdev-offload-dpdk.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)