| Message ID | 20210915100123.42759-1-mark.d.gray@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [ovs-dev] dpif-netlink: Fix feature negotiation for older kernels | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 9/15/21 12:01 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 <dceara@redhat.com> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2004083 > Suggested-by: Ilya Maximets <i.maximets@ovn.org> > Signed-off-by: Mark Gray <mark.d.gray@redhat.com> > --- Hi Mark, Thanks for working on this, it fixes the issue I was having when starting with a fresh configuration on an old kernel. There is however another case this patch doesn't seem to cover, although I'm not 100% sure we need to address it. Please see below. > lib/dpif-netlink.c | 66 ++++++++++++++++++++++++++++++---------------- > 1 file changed, 43 insertions(+), 23 deletions(-) > > diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c > index 34fc04237333..c16323f7ee21 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,54 @@ 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; > error = dpif_netlink_dp_transact(&dp_request, &dp, &buf); > + dp_request.user_features &= ~OVS_DP_F_UNSUPPORTED; > if (error) { > - dp_request.user_features &= ~OVS_DP_F_DISPATCH_UPCALL_PER_CPU; > - dp_request.user_features |= OVS_DP_F_VPORT_PIDS; > + /* 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. > + */ > + 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) { > - return error; > + if (error) { > + 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); > + } > + if (error) { > + return error; > + } > + > + error = open_dpif(&dp, dpifp); > + dpif_netlink_set_features(*dpifp, OVS_DP_F_TC_RECIRC_SHARING); > + } else { > + VLOG_INFO("Kernel does not correctly support feature negotiation. " > + "Using standard features"); > + error = open_dpif(&dp, dpifp); If the datapath was already configured with OVS_DP_F_DISPATCH_UPCALL_PER_CPU (e.g., running code that didn't include this patch), but the kernel doesn't support it *and* also doesn't reject it, we need to reset it. I guess we need another call to dpif_netlink_dp_transact() here. Regards, Dumitru > } > > - error = open_dpif(&dp, dpifp); > - dpif_netlink_set_features(*dpifp, OVS_DP_F_TC_RECIRC_SHARING); > ofpbuf_delete(buf); > > if (create) { >
On 15/09/2021 11:27, Dumitru Ceara wrote: > On 9/15/21 12:01 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 <dceara@redhat.com> >> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2004083 >> Suggested-by: Ilya Maximets <i.maximets@ovn.org> >> Signed-off-by: Mark Gray <mark.d.gray@redhat.com> >> --- > > Hi Mark, > > Thanks for working on this, it fixes the issue I was having when > starting with a fresh configuration on an old kernel. > > There is however another case this patch doesn't seem to cover, although > I'm not 100% sure we need to address it. Please see below. Not sure either but it's a good idea to fix it. Thanks. > >> lib/dpif-netlink.c | 66 ++++++++++++++++++++++++++++++---------------- >> 1 file changed, 43 insertions(+), 23 deletions(-) >> >> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c >> index 34fc04237333..c16323f7ee21 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,54 @@ 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; >> error = dpif_netlink_dp_transact(&dp_request, &dp, &buf); >> + dp_request.user_features &= ~OVS_DP_F_UNSUPPORTED; >> if (error) { >> - dp_request.user_features &= ~OVS_DP_F_DISPATCH_UPCALL_PER_CPU; >> - dp_request.user_features |= OVS_DP_F_VPORT_PIDS; >> + /* 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. >> + */ >> + 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) { >> - return error; >> + if (error) { >> + 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); >> + } >> + if (error) { >> + return error; >> + } >> + >> + error = open_dpif(&dp, dpifp); >> + dpif_netlink_set_features(*dpifp, OVS_DP_F_TC_RECIRC_SHARING); >> + } else { >> + VLOG_INFO("Kernel does not correctly support feature negotiation. " >> + "Using standard features"); >> + error = open_dpif(&dp, dpifp); > > If the datapath was already configured with > OVS_DP_F_DISPATCH_UPCALL_PER_CPU (e.g., running code that didn't include > this patch), but the kernel doesn't support it *and* also doesn't reject > it, we need to reset it. I guess we need another call to > dpif_netlink_dp_transact() here. > > Regards, > Dumitru > >> } >> >> - error = open_dpif(&dp, dpifp); >> - dpif_netlink_set_features(*dpifp, OVS_DP_F_TC_RECIRC_SHARING); >> ofpbuf_delete(buf); >> >> if (create) { >> >
diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index 34fc04237333..c16323f7ee21 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,54 @@ 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; error = dpif_netlink_dp_transact(&dp_request, &dp, &buf); + dp_request.user_features &= ~OVS_DP_F_UNSUPPORTED; if (error) { - dp_request.user_features &= ~OVS_DP_F_DISPATCH_UPCALL_PER_CPU; - dp_request.user_features |= OVS_DP_F_VPORT_PIDS; + /* 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. + */ + 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) { - return error; + if (error) { + 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); + } + if (error) { + return error; + } + + error = open_dpif(&dp, dpifp); + dpif_netlink_set_features(*dpifp, OVS_DP_F_TC_RECIRC_SHARING); + } else { + VLOG_INFO("Kernel does not correctly support feature negotiation. " + "Using standard features"); + error = open_dpif(&dp, dpifp); } - error = open_dpif(&dp, dpifp); - dpif_netlink_set_features(*dpifp, OVS_DP_F_TC_RECIRC_SHARING); ofpbuf_delete(buf); if (create) {
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 <dceara@redhat.com> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2004083 Suggested-by: Ilya Maximets <i.maximets@ovn.org> Signed-off-by: Mark Gray <mark.d.gray@redhat.com> --- lib/dpif-netlink.c | 66 ++++++++++++++++++++++++++++++---------------- 1 file changed, 43 insertions(+), 23 deletions(-)