On Tue, Oct 20, 2020 at 10:07 AM Greg Rose <[email protected]> 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 <[email protected]>
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")
> 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.
> 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?
> 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.
Thanks,
-Yi-Hung
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
