On 10/20/2020 1:12 PM, Sriharsha Basavapatna wrote:
On Tue, Oct 20, 2020 at 2:40 PM Ilya Maximets <[email protected]> wrote:
On 10/20/20 10:51 AM, Sriharsha Basavapatna wrote:
On Mon, Oct 19, 2020 at 7:59 PM Ilya Maximets <[email protected]> wrote:
On 10/18/20 9:10 AM, Eli Britstein wrote:
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. Fix it by checking if tunnel is
present.
Signed-off-by: Eli Britstein <[email protected]>
---
Thanks, Eli.
Harsha, Emma, could you, please, review/test this version?
Best regards, Ilya Maximets.
Reviewed this change. Do we even need this backported to 2.13, since
we don't support clone/tnl-push actions with offload ?
I think we could have metadata partially set even if we're not performing
clone/tnl-push actions. Maybe something like this:
table=0,in_port=1,ip,actions=load:0xbba->NXM_NX_TUN_ID[],goto_table(1)
table=1,ip,nw_src=192.168.0.1,actions=load:0xa566c10->NXM_NX_TUN_IPV4_DST[],<out
to tunnel>
table=1,ip,nw_src=192.168.0.2,actions=drop
In this scenario packet that goes to 'drop' action might have tun_id set
in the metadata and we will not offload it. I didn't test that though.
Does that make sense?
I tested with the above set of rules. It doesn't fail in
validate_flow() even without this fix, since it checks
match_zero_wc.flow.tunnel and not match_zero_wc.masks.tunnel which has
the tun_id (mask) set. So, this fix doesn't make any difference
(validate succeeds with or without it).
(gdb) p /x match_zero_wc.flow.tunnel.tun_id
$4 = 0x0
(gdb) p /x match_zero_wc.wc.masks.tunnel.tun_id
$5 = 0xffffffffffffffff
Harsha - you are right. Thanks. This patch is not needed there as is.
Maybe it was intentional to check the "flow" and not "masks" for tunnel,
so I think we can abandon it for backport <=2.13.
Regarding not supporting clone/tnl-push - it is not related. The issue
is (might have been) with validation of matches. If we checked the masks
we would fail also the partial offload.
Thanks,
-Harsha
Thanks,
-Harsha
lib/netdev-offload-dpdk.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c
index 4538baf5e..c68d539ea 100644
--- a/lib/netdev-offload-dpdk.c
+++ b/lib/netdev-offload-dpdk.c
@@ -1092,7 +1092,8 @@ netdev_offload_dpdk_validate_flow(const struct match
*match)
/* Create a wc-zeroed version of flow. */
match_init(&match_zero_wc, &match->flow, &match->wc);
- if (!is_all_zeros(&match_zero_wc.flow.tunnel,
+ if (flow_tnl_dst_is_set(&match->flow.tunnel) &&
+ !is_all_zeros(&match_zero_wc.flow.tunnel,
sizeof match_zero_wc.flow.tunnel)) {
goto err;
}
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
