| Message ID | 20231031151417.124022-1-salems@nvidia.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v3,1/1] netdev-offload-dpdk: Fix GRE without a key match. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/intel-ovs-compilation | success | test: success |
On Tue, Oct 31, 2023 at 05:14:17PM +0200, 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 <salems@nvidia.com> Acked-by: Simon Horman <horms@ovn.org>
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 <salems@nvidia.com> > --- > 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. > 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;
> -----Original Message----- > From: Ilya Maximets <i.maximets@ovn.org> > Sent: Friday, 17 November 2023 0:04 > To: Salem Sol <salems@nvidia.com>; dev@openvswitch.org > Cc: Eli Britstein <elibr@nvidia.com>; i.maximets@ovn.org; Simon Horman > <horms@ovn.org> > 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 <salems@nvidia.com> > > --- > > 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. > > 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;
On 12/28/23 15:54, Salem Sol wrote: > > >> -----Original Message----- >> From: Ilya Maximets <i.maximets@ovn.org> >> Sent: Friday, 17 November 2023 0:04 >> To: Salem Sol <salems@nvidia.com>; dev@openvswitch.org >> Cc: Eli Britstein <elibr@nvidia.com>; i.maximets@ovn.org; Simon Horman >> <horms@ovn.org> >> 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 <salems@nvidia.com> >>> --- >>> 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; >
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; 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;
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 <salems@nvidia.com> --- lib/netdev-offload-dpdk.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-)