[ovs-dev] [PATCH] dpif-netlink: Improve feature negotiation for older kernels.

[Paolo Valerio]

Hi Chris,

Chris Mi via dev <ovs-dev at openvswitch.org> writes:

> OVS_DP_F_UNALIGNED is already set, no need to set again. If restarting ovs,
> dp is already created. So dpif_netlink_dp_transact() will return EEXIST.
> No need to probe again.
>
> Signed-off-by: Chris Mi <cmi at nvidia.com>
> ---
>  lib/dpif-netlink.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c
> index 5f4b60c5a..baa8e4d2a 100644
> --- a/lib/dpif-netlink.c
> +++ b/lib/dpif-netlink.c
> @@ -411,11 +411,10 @@ dpif_netlink_open(const struct dpif_class *class OVS_UNUSED, const char *name,
>           * dispatching, we fall back to the per-vport dispatch mode.
>           */
>          dp_request.user_features &= ~OVS_DP_F_UNSUPPORTED;
> -        dp_request.user_features |= OVS_DP_F_UNALIGNED;
>          dp_request.user_features &= ~OVS_DP_F_VPORT_PIDS;
>          dp_request.user_features |= OVS_DP_F_DISPATCH_UPCALL_PER_CPU;
>          error = dpif_netlink_dp_transact(&dp_request, &dp, &buf);
> -        if (error) {
> +        if (error == EOPNOTSUPP) {
>              dp_request.user_features &= ~OVS_DP_F_DISPATCH_UPCALL_PER_CPU;
>              dp_request.user_features |= OVS_DP_F_VPORT_PIDS;
>              error = dpif_netlink_dp_transact(&dp_request, &dp, &buf);

The patch LGTM, there's a remark about this function, though.

Before [1] the datapath did not check what user_features were supported,
for this reason [2] was needed to avoid the case in which we set
OVS_DP_F_DISPATCH_UPCALL_PER_CPU on old kernels without supporting it.

I wonder what happens if, in case of kernel without [1] (prior to
5.4), we try to create the datapath during a restart?

My impression is that we'll keep transacting receiving EEXIST, and only
after opening (without trying to create it) we set the things as we
intend.

This seems to be confirmed by:

ovs-vswitchd   834 [000]   146.180045: probe:ovs_dp_cmd_new__return: (ffffffffc075d290 <- ffffffffacba1a5a) retval=0xffffffef
ovs-vswitchd   834 [000]   146.180111: probe:ovs_dp_cmd_new__return: (ffffffffc075d290 <- ffffffffacba1a5a) retval=0xffffffef
ovs-vswitchd   834 [000]   146.180212: probe:ovs_dp_cmd_new__return: (ffffffffc075d290 <- ffffffffacba1a5a) retval=0xffffffef

Note that I didn't really test it against an old kernel, but I just
removed the user_features validation from the kernel code.

If confirmed, this is not a problem per se (there should not be a case
where this can become a functional problem), it's more about knowing that
there's this further chance to clean things up.

[1] 95a7233c452a ("net: openvswitch: Set OvS recirc_id from tc chain index")
[2] b841e3cd4a28 ("dpif-netlink: Fix feature negotiation for older kernels.")