On 10/16/2020 3:02 AM, Ilya Maximets wrote:
External email: Use caution opening links or attachments
On 10/12/20 5:25 PM, Sriharsha Basavapatna via dev wrote:
On Mon, Oct 12, 2020 at 4:48 PM Eli Britstein <[email protected]> wrote:
On 10/12/2020 10:20 AM, Sriharsha Basavapatna wrote:
On Sun, Sep 6, 2020 at 5:51 PM Eli Britstein <[email protected]> wrote:
ping
On 7/30/2020 1:58 PM, Eli Britstein wrote:
From: Lei Wang <[email protected]>
Struct match has the tunnel values/masks in
match->flow.tunnel/match->wc.masks.tunnel.
Load actions such as load:0xa566c10->NXM_NX_TUN_IPV4_DST[],
load:0xbba->NXM_NX_TUN_ID[] are utilizing the tunnel masks fields,
but those should not be used for matching.
Offloading fails if masks is not clear. Clear it if no tunnel used.
Signed-off-by: Lei Wang <[email protected]>
Reviewed-by: Eli Britstein <[email protected]>
Reviewed-by: Gaetan Rivet <[email protected]>
Signed-off-by: Eli Britstein <[email protected]>
Acked-by: Sriharsha Basavapatna <[email protected]>
See my comment below.
---
lib/netdev-offload-dpdk.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
index de6101e4d..0d23e4879 100644
--- a/lib/netdev-offload-dpdk.c
+++ b/lib/netdev-offload-dpdk.c
@@ -682,6 +682,10 @@ parse_flow_match(struct flow_patterns *patterns,
consumed_masks = &match->wc.masks;
+ if (!flow_tnl_dst_is_set(&match->flow.tunnel)) {
+ memset(&match->wc.masks.tunnel, 0, sizeof match->wc.masks.tunnel);
Small nit: For consistency, I think, we should access via 'consumed_masks'
pointer, not directly. What do you think?
I could change that on commit.
OK.
Another point is that we could actually consider this as a bug fix, but
I'm not sure which commit to use for a 'Fixes' tag. Suggestions?
Technically, the issue exists on all branches down to 2.10, but 2.13 and
lower will require a bit different fix due t major rework of this code.
I see the original "validate" code also has it, so maybe e8a2b5bf92bb
("netdev-dpdk: implement flow offload with rte flow")
Do you want me to send v2 with this fixes tag and usage of
consumed_masks (above), or you can do it when applying?
Thanks, Eli
+ }
+
This fix looks good to me. Did you take a look to see if this can be
fixed in the code that generates these invalid masks in the first
place ?
I wouldn't say it's "invalid masks". OVS takes those masks into
consideration with such usage of flow_tnl_dst_is_set, that is done in
several places in the code. For example odp-util.c, lib/netdev-offload-tc.c.
It is invalid because the datapath rule is specifying tunnel masks
when we are not really matching on them. In the context of encap
actions (which is what this patch is fixing), tunnel masks shouldn't
be set.
It seems like tunnel metadata and a couple of other fields are special
cases that allowed to have masks set without having keys. Datapaths
handles these cases. This was done for some reason, but it hard to
track down why exactly. Following commit made higher layers to always
provide tunnel masks even if tunnel keys are not specified:
01fcdfc6b322 ("odp-util: Always serialize tunnel mask attributes.")
It looks like it was intended to simplify processing, but I'm not sure.
Anyway, datapath should not look at these masks if tnl_dst is not
specified in keys as this is a sign of a correct tunnel metadata. So,
the fix looks correct.
Thanks,
-Harsha
memset(&consumed_masks->in_port, 0, sizeof consumed_masks->in_port);
/* recirc id must be zero. */
if (match->wc.masks.recirc_id & match->flow.recirc_id) {
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
