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

Wed Nov 4 00:06:50 UTC 2020

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.

> 
> 
>> 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.

> 
>>     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])])
> 
> 
> 
>> @@ -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")

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/datapath.c b/datapath/datapath.c
>> index 52a59f135..09fb3b1fc 100644
>> --- a/datapath/datapath.c
>> +++ b/datapath/datapath.c
>> @@ -529,7 +529,7 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb,
>>                  hash |= OVS_PACKET_HASH_SW_BIT;
>>   #endif
>>
>> -#ifdef HAVE_L4_RXHASH
>> +#if defined(HAVE_L4_RXHASH) && !defined(HAVE_L4_HASH)
>>          if (skb->l4_rxhash)
>>   #else
>>          if (skb->l4_hash)
> 
> Looks like we can get rid of all #ifdef, and only leave
>>          if (skb->l4_hash)
> 
> 
> 
>> 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

Thanks,

- Greg