On 12/28/23 15:54, Salem Sol wrote: > > >> -----Original Message----- >> From: Ilya Maximets <[email protected]> >> Sent: Friday, 17 November 2023 0:04 >> To: Salem Sol <[email protected]>; [email protected] >> Cc: Eli Britstein <[email protected]>; [email protected]; Simon Horman >> <[email protected]> >> Subject: Re: [ovs-dev] [PATCH v3 1/1] netdev-offload-dpdk: Fix GRE without a >> key match. >> >> External email: Use caution opening links or attachments >> >> >> On 10/31/23 16:14, Salem Sol via dev wrote: >>> In case there is no match on GRE key, avoid adding the key match item. >>> >>> Fixes: 7617d0583c73 ("netdev-offload-dpdk: Add support for matching on >>> gre fields.") >>> Signed-off-by: Salem Sol <[email protected]> >>> --- >>> lib/netdev-offload-dpdk.c | 15 +++++++++------ >>> 1 file changed, 9 insertions(+), 6 deletions(-) >>> >>> diff --git a/lib/netdev-offload-dpdk.c b/lib/netdev-offload-dpdk.c >>> index 992627fa2..b2bb9013e 100644 >>> --- a/lib/netdev-offload-dpdk.c >>> +++ b/lib/netdev-offload-dpdk.c >>> @@ -1336,16 +1336,19 @@ parse_gre_match(struct flow_patterns >> *patterns, >>> greh_spec->k = !!(match->flow.tunnel.flags & FLOW_TNL_F_KEY); >>> greh_mask->k = 1; >>> >>> - key_spec = xzalloc(sizeof *key_spec); >>> - key_mask = xzalloc(sizeof *key_mask); >>> + if (greh_spec->k) { >>> + key_spec = xzalloc(sizeof *key_spec); >>> + key_mask = xzalloc(sizeof *key_mask); >>> >>> - *key_spec = htonl(ntohll(match->flow.tunnel.tun_id)); >>> - *key_mask = htonl(ntohll(match->wc.masks.tunnel.tun_id)); >>> + *key_spec = htonl(ntohll(match->flow.tunnel.tun_id)); >>> + *key_mask = htonl(ntohll(match->wc.masks.tunnel.tun_id)); >>> + >>> + add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_GRE_KEY, >> key_spec, >>> + key_mask, NULL); >>> + } >>> >>> consumed_masks->tunnel.tun_id = 0; >> >> We do not always consume tun_id now. Above line should likely be moved >> under the if condition. >> > Thanks for the comment, from my testing I noticed that ovs still adds a mask > on the tunnel.tun_id even if the gre-key is not present that's why I kept it > outside the if condition.
Hmm, I suppose that is because the if condition here is also not correct. it should be if (greh_mask->k) instead of if (greh_spec->k). And greh_mask->k = 1; should take the actual value from the mask instead of always setting the value to 1. Is there a reason for hardcoding the mask value here? >>> consumed_masks->tunnel.flags &= ~FLOW_TNL_F_KEY; >>> - add_flow_pattern(patterns, RTE_FLOW_ITEM_TYPE_GRE_KEY, >> key_spec, >>> - key_mask, NULL); >>> } >>> >>> consumed_masks->tunnel.flags &= ~FLOW_TNL_F_DONT_FRAGMENT; > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
