On 11/3/2020 2:17 PM, Yi-Hung Wei wrote:
On Tue, Oct 20, 2020 at 10:07 AM Greg Rose <gvrose8...@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 <gvrose8...@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
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
