Ilya Maximets <[email protected]> writes: > On 11/11/21 19:06, Paolo Valerio wrote: >> Hi Chris, >> >> Chris Mi via dev <[email protected]> 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 <[email protected]> >>> --- >>> 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. > > Paolo, should I consider this as 'Acked-by' ? >
yes, sorry, I must have forgotten to add it. Acked-by: Paolo Valerio <[email protected]> >> >> 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.") _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
