On 9/16/21 12:04 PM, Mark Gray wrote: > On 16/09/2021 09:40, Dumitru Ceara wrote: >> On 9/15/21 5:08 PM, Mark Gray wrote: >>> Older kernels do not reject unsupported features. This can lead >>> to a situation in which 'ovs-vswitchd' believes that a feature is >>> supported when, in fact, it is not. >>> >>> This patch probes for this by attempting to set a known unsupported >>> feature. >>> >>> Reported-by: Dumitru Ceara <dce...@redhat.com> >>> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2004083 >>> Suggested-by: Ilya Maximets <i.maxim...@ovn.org> >>> Signed-off-by: Mark Gray <mark.d.g...@redhat.com> >>> --- >> >> This works for me, for this version: >> >> Tested-by: Dumitru Ceara <dce...@redhat.com> >> >> I do have a small comment below, thanks! >> >>> >>> Notes: >>> v2: Fix case raised by Dumitru in which kernel is already configured >>> with >>> unsupported features. >>> >>> lib/dpif-netlink.c | 72 ++++++++++++++++++++++++++++++++-------------- >>> 1 file changed, 50 insertions(+), 22 deletions(-) >>> >>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c >>> index 34fc04237333..5f4b60c5a6d6 100644 >>> --- a/lib/dpif-netlink.c >>> +++ b/lib/dpif-netlink.c >>> @@ -84,6 +84,8 @@ enum { MAX_PORTS = USHRT_MAX }; >>> #define EPOLLEXCLUSIVE (1u << 28) >>> #endif >>> >>> +#define OVS_DP_F_UNSUPPORTED (1 << 31); >>> + >>> /* This PID is not used by the kernel datapath when using dispatch per CPU, >>> * but it is required to be set (not zero). */ >>> #define DPIF_NETLINK_PER_CPU_PID UINT32_MAX >>> @@ -382,36 +384,62 @@ dpif_netlink_open(const struct dpif_class *class >>> OVS_UNUSED, const char *name, >>> dp_request.cmd = OVS_DP_CMD_SET; >>> } >>> >>> - /* The Open vSwitch kernel module has two modes for dispatching >>> upcalls: >>> - * per-vport and per-cpu. >>> - * >>> - * When dispatching upcalls per-vport, the kernel will >>> - * send the upcall via a Netlink socket that has been selected based >>> on the >>> - * vport that received the packet that is causing the upcall. >>> - * >>> - * When dispatching upcall per-cpu, the kernel will send the upcall via >>> - * a Netlink socket that has been selected based on the cpu that >>> received >>> - * the packet that is causing the upcall. >>> - * >>> - * First we test to see if the kernel module supports per-cpu >>> dispatching >>> - * (the preferred method). If it does not support per-cpu dispatching, >>> we >>> - * fall back to the per-vport dispatch mode. >>> + /* Some older kernels will not reject unknown features. This will cause >>> + * 'ovs-vswitchd' to incorrectly assume a feature is supported. In >>> order to >>> + * test for that, we attempt to set a feature that we know is not >>> supported >>> + * by any kernel. If this feature is not rejected, we can assume we are >>> + * running on one of these older kernels. >>> */ >>> 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; >>> + dp_request.user_features |= OVS_DP_F_VPORT_PIDS; >>> + dp_request.user_features |= OVS_DP_F_UNSUPPORTED; >> >> Nit: Sorry, I missed this on v1, but I guess we could just remove the >> lines that set OVS_DP_F_UNALIGNED and OVS_DP_F_VPORT_PIDS here, and just >> request OVS_DP_F_UNSUPPORTED. We set the correct features anyway, in >> all cases, below. > > I don't want to do that because it would change the behaviour of the > "create" case. In the previous code, when we are creating the datapath, > we set UNALIGNED, VPORT_PIDs using the NEW command. If I remove that, I > will call NEW without the flags and then SET them flags after. Not sure > if it will have any adverse consequences on some version of the kernel > so it may not be worth the risk. What do you think? >
Ok, I see now, thanks for the follow up! Let's leave it as is then. Regards, Dumitru _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
