On 5 Jul 2022, at 0:45, Ilya Maximets wrote:
> Current offloading code supports only limited number of tunnel keys > and silently ignores everything it doesn't understand. This is > causing, for example, offloaded ERSPAN tunnels to not work, because > flow is offloaded, but ERSPAN options are not provided to TC. > > There is a number of tunnel keys, which are supported by the userspace, > but silently ignored during offloading: > > OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT > OVS_TUNNEL_KEY_ATTR_OAM > OVS_TUNNEL_KEY_ATTR_VXLAN_OPTS > OVS_TUNNEL_KEY_ATTR_ERSPAN_OPTS > > OVS_TUNNEL_KEY_ATTR_CSUM is kind of supported, but only for actions > and for some reason is set from the tunnel port instead of the > provided action, and not currently supported for the tunnel key in > the match. > > Addig a default case to fail offloading of unknown attributes. For > now explicitly allowing incorrect behavior for the DONT_FRAGMENT flag, > otherwise we'll break all tunnel offloading by default. VXLAN and > ERSPAN options has to fail offloading, because the tunnel will not > work otherwise. OAM is not a default configurations, so failing it > as well. The missing DONT_FRAGMENT flag though should, probably, > cause frequent flow revalidation, but that is not new with this patch. > > Same for the 'match' key, only clearing masks that was actually > consumed, except for the DONT_FRAGMENT and CSUM flags, which are > explicitly allowed and highlighted as broken. > > Also, ttl and tos were incorrectly checked by the value instead of a > mask during the flow key dump. Destination port as well as CSUM > configuration for unknown reason was not taken from the actions list > and were passed via HW offload info. > > Reported-at: > https://mail.openvswitch.org/pipermail/ovs-dev/2022-July/395522.html > Reported-by: Eelco Chaudron <[email protected]> > Fixes: 8f283af89298 ("netdev-tc-offloads: Implement netdev flow put using tc > interface") > Signed-off-by: Ilya Maximets <[email protected]> Changes look good to me, and all system-traffic.at tests that were passing without the change are still passing, including the failing ERSPAN ones. Acked-by: Eelco Chaudron <[email protected]> Two small nits below. > --- > lib/dpif-netlink.c | 14 +--------- > lib/netdev-offload-tc.c | 57 ++++++++++++++++++++++++++++++++++------- > lib/netdev-offload.h | 3 --- > 3 files changed, 49 insertions(+), 25 deletions(-) > > + case OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT: { > + /* XXX: This is wrong! We're ignoring the DF flag configuration > + * requested by the user. However, TC for now has no way to pass > + * that flag and it is set by default, meaning tunnel offloading > + * will not work if 'options:df_default=false' is not set. > + * Keeping incorrect behavior for now. */ <SNIP> > + > + /* XXX: This is wrong! We're ignoring DF and CSUM flags > configuration > + * requested by the user. However, TC for now has no way to pass > + * these flags in a flower key and their masks are set by default, > + * meaning tunnel offloading will not work at all if not cleared. > + * Keeping incorrect behavior for now. */ > + tnl_mask->flags &= ~(FLOW_TNL_F_DONT_FRAGMENT | FLOW_TNL_F_CSUM); > + Small nit, 2x two spaces before “ However, TC for...”. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
