diff mbox series

[ovs-dev] netlink: removed incorrect optimization

Message ID 20201120131246.877066-1-cpp.code.lv@gmail.com
State New
Headers show
Series [ovs-dev] netlink: removed incorrect optimization | expand

Commit Message

Cpp Code Nov. 20, 2020, 1:12 p.m. UTC
This optimization caused FLOW_TNL_F_UDPIF flag not to be used in
hash calculation for geneve tunnel when revalidating flows which
resulted in different cache hash values and incorrect behaviour.

Reported-at: https://github.com/vmware-tanzu/antrea/issues/897
Signed-off-by: Toms Atteka <cpp.code.lv@gmail.com>
---
 lib/tun-metadata.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ben Pfaff Nov. 20, 2020, 4:52 p.m. UTC | #1
On Fri, Nov 20, 2020 at 05:12:46AM -0800, Toms Atteka wrote:
> This optimization caused FLOW_TNL_F_UDPIF flag not to be used in
> hash calculation for geneve tunnel when revalidating flows which
> resulted in different cache hash values and incorrect behaviour.
> 
> Reported-at: https://github.com/vmware-tanzu/antrea/issues/897
> Signed-off-by: Toms Atteka <cpp.code.lv@gmail.com>

Why was the check there?  It seems strange to have a very specific "if"
test and then just remove it without some idea of why it was there to
begin with.

...

> -    } else if (flow->metadata.present.len || is_mask) {
> +    } else {
Cpp Code Nov. 23, 2020, 2:45 p.m. UTC | #2
I would expect such checks to be commented as indeed it was not clear why
it was there. I looked in similar places where  FLOW_TNL_F_UDPIF is used
and there is no such check. The commit which created this condition is
quite large so I am not able to conclude from that.
I assume this is an optimization because if we don't set
OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS there is less code to be executed later
when the parsing is done. I believe the assumption was made to exclude this
without considering specifics of Geneve related  FLOW_TNL_F_UDPIF flag.


On Fri, Nov 20, 2020 at 8:52 AM Ben Pfaff <blp@ovn.org> wrote:

> On Fri, Nov 20, 2020 at 05:12:46AM -0800, Toms Atteka wrote:
> > This optimization caused FLOW_TNL_F_UDPIF flag not to be used in
> > hash calculation for geneve tunnel when revalidating flows which
> > resulted in different cache hash values and incorrect behaviour.
> >
> > Reported-at: https://github.com/vmware-tanzu/antrea/issues/897
> > Signed-off-by: Toms Atteka <cpp.code.lv@gmail.com>
>
> Why was the check there?  It seems strange to have a very specific "if"
> test and then just remove it without some idea of why it was there to
> begin with.
>
> ...
>
> > -    } else if (flow->metadata.present.len || is_mask) {
> > +    } else {
>
diff mbox series

Patch

diff --git a/lib/tun-metadata.c b/lib/tun-metadata.c
index c0b0ae044..af0bcbde8 100644
--- a/lib/tun-metadata.c
+++ b/lib/tun-metadata.c
@@ -828,7 +828,7 @@  tun_metadata_to_geneve_nlattr(const struct flow_tnl *tun,
         } else {
             tun_metadata_to_geneve_nlattr_mask(key, tun, flow, b);
         }
-    } else if (flow->metadata.present.len || is_mask) {
+    } else {
         nl_msg_put_unspec(b, OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS,
                           tun->metadata.opts.gnv,
                           flow->metadata.present.len);