| Message ID | 20201120131246.877066-1-cpp.code.lv@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] netlink: removed incorrect optimization | expand |
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 {
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 { >
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 --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);
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(-)