[ovs-dev] [PATCH 1/2] compat: Fix build issue on RHEL 7.7

Yi-Hung Wei yihung.wei at gmail.com
Wed Nov 4 01:07:57 UTC 2020


On Tue, Nov 3, 2020 at 4:06 PM Gregory Rose <gvrose8192 at gmail.com> wrote:
> On 11/3/2020 2:17 PM, Yi-Hung Wei wrote:
> > On Tue, Oct 20, 2020 at 10:07 AM Greg Rose <gvrose8192 at gmail.com> wrote:
> >>
> >> RHEL 7.7 has a KABI fixup in struct sk_buff to backport the member
> >> name change of l4_rxhash to l4_hash.  This exposed a couple of
> >> issues in patch 8063e0958780 which was intended to remove support
> >> for kernels older than 3.10.
> >>
> >> Remove stale code and add a compat level check to detect the change.
> >> This fixes a compile error on RHEL 7.7.
> >>
> >> Fixes: 8063e0958780 ("datapath: Drop support for kernel older than 3.10")
> >> Signed-off-by: Greg Rose <gvrose8192 at gmail.com>
> >
> > Hi Greg,
> >
> > Thanks for the patch.  I found the compilation error on RHEL 7.7
> > actually happens starting from a more recent patch as follows.  If
> > that is the case, please update the fix tag.
> >
> >   2020-05-25 9ba57fc7cccc ("datapath: Add hash info to upcall")
>
> Hi Yi-Hung,
>
> I don't think I'm fixing anything in that patch.  There's nothing
> wrong with it that I can see.  It properly depended on the
> HAVE_L4_RXHASH define which is not part of that patch but was
> from 75e93287db ("datapath: improve l4_rxhash regex").
>
> Seems to me fixing something would require changes to the code
> that's being fixed.

Hi Greg,

I thought your change in datapath.c is to fix the following new added
lines in 9ba57fc7cccc ("datapath: Add hash info to upcall").  I found
it because the compilation goes fine before this patch on RHEL 7.7.

9ba57fc7cccc ("datapath: Add hash info to upcall")
@@ -523,6 +525,25 @@ static int queue_userspace_packet(struct datapath
*dp, struct sk_buff *skb,
.....
+#ifdef HAVE_L4_RXHASH
+       if (skb->l4_rxhash)
+#else
+       if (skb->l4_hash)
+#endif
+               hash |= OVS_PACKET_HASH_L4_BIT;



> >> diff --git a/acinclude.m4 b/acinclude.m4
> >> index 1460289ca..8e80d7930 100644
> >> --- a/acinclude.m4
> >> +++ b/acinclude.m4
> >> @@ -879,6 +879,8 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
> >>                     [OVS_DEFINE([HAVE_SKB_ZEROCOPY])])
> >>     OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [u8.*l4_rxhash],
> >>                     [OVS_DEFINE([HAVE_L4_RXHASH])])
> >> +  OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [u8.*l4_hash],
> >> +                  [OVS_DEFINE([HAVE_L4_HASH])])
> > Looks like the main compatibility issue on using either l4_rxhash or
> > l4_hash is due to the kernel ABI update on skbuff.h from the following
> > commit that firstly introduced in 3.15 kernel.
> >
> > 2014-03-24 61b905da33ae (("net: Rename skb->rxhash to skb->hash").
> >
> > I also found that starting from RHEL 7.2, RHEL introduced the new ABI.
> >
> > __u8 RH_KABI_RENAME(l4_rxhash, l4_hash):1;
> >
> >  From Documentation/faq/releases.rst, the oldest kernel that OVS
> > supports from 2.10.x is 3.16, I think we can drop the compatibility
> > support on "l4_rxhash" and only use "l4_hash" in the code base.
> >
> > If that makes sense to you, let's drop the following in acindlue.m4,
> > and clean up the update on datapath.c and
> > ./datapath/linux/compat/include/linux/skbuff.h accordingly.
>
> So we don't want to support RHEL 7.2 and older?  I know we only
> "officially" support 3.16 and above but RHEL 7.x 3.10 kernel is
> a special case.

The new ABI is introduced in RHEL7.2, so the support that we will drop
is RHEL 7.0 and RHEL 7.1. Since RHEL 7.1 was introduced in 2015 March,
I think it is probably fine.


> >> @@ -975,8 +977,6 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [
> >>
> >>     OVS_GREP_IFELSE([$KSRC/include/net/sock.h], [sk_no_check_tx])
> >>     OVS_GREP_IFELSE([$KSRC/include/linux/udp.h], [no_check6_tx])
> >> -  OVS_GREP_IFELSE([$KSRC/include/linux/utsrelease.h], [el6],
> >> -                  [OVS_DEFINE([HAVE_RHEL6_PER_CPU])])
> >>     OVS_FIND_PARAM_IFELSE([$KSRC/include/net/protocol.h],
> >>                           [udp_add_offload], [net],
> >>                           [OVS_DEFINE([HAVE_UDP_ADD_OFFLOAD_TAKES_NET])])
> > Good catch. I think it is a valid clean up.  But since it does not fix
> > 9ba57fc7cccc ("datapath: Add hash info to upcall"), should we move it
> > along with the changes in
> > ./datapath/linux/compat/include/linux/percpu.h to a separate patch?
>
> But it does fix 8063e0958780
> ("datapath: Drop support for kernel older than 3.10")
Yes, it does, and we should clean it up in the compat code. My
suggestion is to put it to a separate patch so that we may apply this
clean up patch to the older branches ( before this 9ba57fc7cccc
("datapath: Add hash info to upcall") is introduced), if needed.

> So if we agree to drop support for RHEL 7.2 then yes, we can do
> as you suggest.
>
> As I think about it, dropping support for the OOT kernel driver
> for RHEL 7.2 is probably fine.  I doubt anyone will complain
> and that will simplify the code as you have pointed out.
>


> >> diff --git a/datapath/linux/compat/include/linux/skbuff.h b/datapath/linux/compat/include/linux/skbuff.h
> >> index 204ce5497..94479f57b 100644
> >> --- a/datapath/linux/compat/include/linux/skbuff.h
> >> +++ b/datapath/linux/compat/include/linux/skbuff.h
> >> @@ -278,8 +278,10 @@ static inline void skb_clear_hash(struct sk_buff *skb)
> >>   #ifdef HAVE_RXHASH
> >>          skb->rxhash = 0;
> >>   #endif
> >> -#if defined(HAVE_L4_RXHASH) && !defined(HAVE_RHEL_OVS_HOOK)
> >> +#if defined(HAVE_L4_RXHASH) && !defined(HAVE_L4_HASH)
> >>          skb->l4_rxhash = 0;
> >> +#else
> >> +       skb->l4_hash = 0;
> >>   #endif
> >>   }
> >>   #endif
> > We can also clean up skb_clear_hash() if we only care l4_hash.
>
> So yes, we can forget about RHEL 7.2 OOT kernel driver support.
> I doubt anyone uses it.
>
> I'll rework the patch, break out the cleanup code and send a
> V3 *if* I can get it to pass Travis.
>
> By the way, what is your opinion on this?
>
> https://mail.openvswitch.org/pipermail/ovs-dev/2020-October/376816.html

I think OOT kernel driver support still provides value IMO.  For
example, Ubuntu 18.04 LTS uses 4.15 kernel by default, and the end of
life of 18.04 LTS is 2023.  If we drop OOT kernel driver support, we
will make the 18.04 LTS users' life much harder.

Thanks,

-Yi-Hung


More information about the dev mailing list