diff mbox series

[ovs-dev] netlink: removed incorrect optimization

Message ID 20201120131246.877066-1-cpp.code.lv@gmail.com
State Superseded
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 {
>
Yi-Hung Wei Dec. 15, 2020, 11:43 p.m. UTC | #3
On Mon, Nov 23, 2020 at 6:45 AM Cpp Code <cpp.code.lv@gmail.com> wrote:
>
> 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 {
> >

Hi Toms and Ben,

I think the root cause for the reported issue observed on Antrea is
because the netdev native tunnel always sets FLOW_TNL_F_UDPIF no
matter the geneve option is present or not.   So Toms's fix here will
always generate OVS_TUNNEL_KEY_ATTR_GENEVE_OPTS so that
FLOW_TNL_F_UDPIF can be set when we convert the netlink attributes
back to flow key.

Instead of fixing the issue on this function, I think we can fix on
the tunnel receive function, and only set FLOW_TNL_F_UDPIF when the
geneve tunnel metadata is present. Here is the proposed fix:

https://mail.openvswitch.org/pipermail/ovs-dev/2020-December/378711.html

Thanks,

-Yi-Hung
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);