| Message ID | 20200917195905.10423-1-gvrose8192@gmail.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev] compat: Replace the HAVE_L4_RXHASH define with HAVE_L4_HASH | expand |
On 9/17/20 9:59 PM, Greg Rose wrote: > The skb now uses l4_hash and it is easier to check for it. Also > fixes a compile error for RHEL 7.7. > > Signed-off-by: Greg Rose <gvrose8192@gmail.com> > --- > acinclude.m4 | 4 ++-- > datapath/datapath.c | 6 +++--- > datapath/linux/compat/include/linux/skbuff.h | 4 ++-- > 3 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/acinclude.m4 b/acinclude.m4 > index 84f344da0..d4e249dac 100644 > --- a/acinclude.m4 > +++ b/acinclude.m4 > @@ -874,8 +874,8 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [ > OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [skb_clear_hash]) > OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [int.skb_zerocopy(], > [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], [l4_hash], > + [OVS_DEFINE([HAVE_L4_HASH])]) > OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [skb_ensure_writable]) > OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [skb_vlan_pop]) > OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [__skb_vlan_pop]) > diff --git a/datapath/datapath.c b/datapath/datapath.c > index 05c1e4274..c3f57f62a 100644 > --- a/datapath/datapath.c > +++ b/datapath/datapath.c > @@ -532,10 +532,10 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb, > hash |= OVS_PACKET_HASH_SW_BIT; > #endif > > -#ifdef HAVE_L4_RXHASH > - if (skb->l4_rxhash) > -#else > +#ifdef HAVE_L4_HASH > if (skb->l4_hash) > +#else > + if (skb->l4_rxhash) > #endif > hash |= OVS_PACKET_HASH_L4_BIT; > > diff --git a/datapath/linux/compat/include/linux/skbuff.h b/datapath/linux/compat/include/linux/skbuff.h > index 6d248b3ed..29253e433 100644 > --- a/datapath/linux/compat/include/linux/skbuff.h > +++ b/datapath/linux/compat/include/linux/skbuff.h > @@ -278,7 +278,7 @@ 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_HASH) && !defined(HAVE_RHEL_OVS_HOOK) It looks like commit 21a719d658b4 ("datapath: check for el6 kernels for per_cpu") missed updating of this line with HAVE_RHEL6_PER_CPU instead of HAVE_RHEL_OVS_HOOK. Maybe that is the root cause of your build issue? I'm not sure why, but this check here was introduced as part of the percpu API fix in commit 3d174bfd1a6d ("datapath: compat: Fix build on RHEL 6.6"). Flavio, could you, please, take a look too? Best regards, Ilya Maximets.
On Tue, Sep 22, 2020 at 12:45:03PM +0200, Ilya Maximets wrote: > On 9/17/20 9:59 PM, Greg Rose wrote: > > The skb now uses l4_hash and it is easier to check for it. Also > > fixes a compile error for RHEL 7.7. > > > > Signed-off-by: Greg Rose <gvrose8192@gmail.com> > > --- > > acinclude.m4 | 4 ++-- > > datapath/datapath.c | 6 +++--- > > datapath/linux/compat/include/linux/skbuff.h | 4 ++-- > > 3 files changed, 7 insertions(+), 7 deletions(-) > > > > diff --git a/acinclude.m4 b/acinclude.m4 > > index 84f344da0..d4e249dac 100644 > > --- a/acinclude.m4 > > +++ b/acinclude.m4 > > @@ -874,8 +874,8 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [ > > OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [skb_clear_hash]) > > OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [int.skb_zerocopy(], > > [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], [l4_hash], > > + [OVS_DEFINE([HAVE_L4_HASH])]) > > OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [skb_ensure_writable]) > > OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [skb_vlan_pop]) > > OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [__skb_vlan_pop]) > > diff --git a/datapath/datapath.c b/datapath/datapath.c > > index 05c1e4274..c3f57f62a 100644 > > --- a/datapath/datapath.c > > +++ b/datapath/datapath.c > > @@ -532,10 +532,10 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb, > > hash |= OVS_PACKET_HASH_SW_BIT; > > #endif > > > > -#ifdef HAVE_L4_RXHASH > > - if (skb->l4_rxhash) > > -#else > > +#ifdef HAVE_L4_HASH > > if (skb->l4_hash) > > +#else > > + if (skb->l4_rxhash) > > #endif > > hash |= OVS_PACKET_HASH_L4_BIT; > > > > diff --git a/datapath/linux/compat/include/linux/skbuff.h b/datapath/linux/compat/include/linux/skbuff.h > > index 6d248b3ed..29253e433 100644 > > --- a/datapath/linux/compat/include/linux/skbuff.h > > +++ b/datapath/linux/compat/include/linux/skbuff.h > > @@ -278,7 +278,7 @@ 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_HASH) && !defined(HAVE_RHEL_OVS_HOOK) > > It looks like commit 21a719d658b4 ("datapath: check for el6 kernels for per_cpu") > missed updating of this line with HAVE_RHEL6_PER_CPU instead of HAVE_RHEL_OVS_HOOK. > Maybe that is the root cause of your build issue? > > I'm not sure why, but this check here was introduced as part of the percpu API fix > in commit 3d174bfd1a6d ("datapath: compat: Fix build on RHEL 6.6"). > > Flavio, could you, please, take a look too? Those were separate things. Going back to RHEL-6, the skb field was there, but got commented out, so it used OVS_HOOK to identify the kernel and so the l4_hash and the "broken" per_cpu. Then the hooks got backported, but the per_cpu was still in the same condition (Aug/2015). Then we dropped support to kernels older than 3.10 and rhel-6 was 2.6.32, so most probably there is room for a clean up. For instance, the HAVE_RHEL_OVS_HOOK could be removed completely as part of Feb/2016 '8063e095878 ("datapath: Drop support for kernel older than 3.10")'. What happened in RHEL-7.7 is that the field was renamed but preserving the kABI with a macro: - __u8 l4_rxhash:1; + RH_KABI_REPLACE_P(__u8 l4_rxhash:1, + __u8 l4_hash:1) So, the regex above gets l4_rxhash, but the actual field name is l4_hash.
On 9/24/2020 6:05 AM, Flavio Leitner wrote: > On Tue, Sep 22, 2020 at 12:45:03PM +0200, Ilya Maximets wrote: >> On 9/17/20 9:59 PM, Greg Rose wrote: >>> The skb now uses l4_hash and it is easier to check for it. Also >>> fixes a compile error for RHEL 7.7. >>> >>> Signed-off-by: Greg Rose <gvrose8192@gmail.com> >>> --- >>> acinclude.m4 | 4 ++-- >>> datapath/datapath.c | 6 +++--- >>> datapath/linux/compat/include/linux/skbuff.h | 4 ++-- >>> 3 files changed, 7 insertions(+), 7 deletions(-) >>> >>> diff --git a/acinclude.m4 b/acinclude.m4 >>> index 84f344da0..d4e249dac 100644 >>> --- a/acinclude.m4 >>> +++ b/acinclude.m4 >>> @@ -874,8 +874,8 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [ >>> OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [skb_clear_hash]) >>> OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [int.skb_zerocopy(], >>> [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], [l4_hash], >>> + [OVS_DEFINE([HAVE_L4_HASH])]) >>> OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [skb_ensure_writable]) >>> OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [skb_vlan_pop]) >>> OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [__skb_vlan_pop]) >>> diff --git a/datapath/datapath.c b/datapath/datapath.c >>> index 05c1e4274..c3f57f62a 100644 >>> --- a/datapath/datapath.c >>> +++ b/datapath/datapath.c >>> @@ -532,10 +532,10 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb, >>> hash |= OVS_PACKET_HASH_SW_BIT; >>> #endif >>> >>> -#ifdef HAVE_L4_RXHASH >>> - if (skb->l4_rxhash) >>> -#else >>> +#ifdef HAVE_L4_HASH >>> if (skb->l4_hash) >>> +#else >>> + if (skb->l4_rxhash) >>> #endif >>> hash |= OVS_PACKET_HASH_L4_BIT; >>> >>> diff --git a/datapath/linux/compat/include/linux/skbuff.h b/datapath/linux/compat/include/linux/skbuff.h >>> index 6d248b3ed..29253e433 100644 >>> --- a/datapath/linux/compat/include/linux/skbuff.h >>> +++ b/datapath/linux/compat/include/linux/skbuff.h >>> @@ -278,7 +278,7 @@ 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_HASH) && !defined(HAVE_RHEL_OVS_HOOK) >> >> It looks like commit 21a719d658b4 ("datapath: check for el6 kernels for per_cpu") >> missed updating of this line with HAVE_RHEL6_PER_CPU instead of HAVE_RHEL_OVS_HOOK. >> Maybe that is the root cause of your build issue? >> >> I'm not sure why, but this check here was introduced as part of the percpu API fix >> in commit 3d174bfd1a6d ("datapath: compat: Fix build on RHEL 6.6"). >> >> Flavio, could you, please, take a look too? > > Those were separate things. Going back to RHEL-6, the skb field > was there, but got commented out, so it used OVS_HOOK to identify > the kernel and so the l4_hash and the "broken" per_cpu. > > Then the hooks got backported, but the per_cpu was still in the > same condition (Aug/2015). > > Then we dropped support to kernels older than 3.10 and rhel-6 > was 2.6.32, so most probably there is room for a clean up. > For instance, the HAVE_RHEL_OVS_HOOK could be removed completely > as part of Feb/2016 '8063e095878 ("datapath: Drop support for > kernel older than 3.10")'. > > What happened in RHEL-7.7 is that the field was renamed but > preserving the kABI with a macro: > > - __u8 l4_rxhash:1; > + RH_KABI_REPLACE_P(__u8 l4_rxhash:1, > + __u8 l4_hash:1) > > > So, the regex above gets l4_rxhash, but the actual field name is > l4_hash. > Flavio and Ilya, Thanks for the review and insightful comments. Let me rework this. - Greg
diff --git a/acinclude.m4 b/acinclude.m4 index 84f344da0..d4e249dac 100644 --- a/acinclude.m4 +++ b/acinclude.m4 @@ -874,8 +874,8 @@ AC_DEFUN([OVS_CHECK_LINUX_COMPAT], [ OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [skb_clear_hash]) OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [int.skb_zerocopy(], [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], [l4_hash], + [OVS_DEFINE([HAVE_L4_HASH])]) OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [skb_ensure_writable]) OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [skb_vlan_pop]) OVS_GREP_IFELSE([$KSRC/include/linux/skbuff.h], [__skb_vlan_pop]) diff --git a/datapath/datapath.c b/datapath/datapath.c index 05c1e4274..c3f57f62a 100644 --- a/datapath/datapath.c +++ b/datapath/datapath.c @@ -532,10 +532,10 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb, hash |= OVS_PACKET_HASH_SW_BIT; #endif -#ifdef HAVE_L4_RXHASH - if (skb->l4_rxhash) -#else +#ifdef HAVE_L4_HASH if (skb->l4_hash) +#else + if (skb->l4_rxhash) #endif hash |= OVS_PACKET_HASH_L4_BIT; diff --git a/datapath/linux/compat/include/linux/skbuff.h b/datapath/linux/compat/include/linux/skbuff.h index 6d248b3ed..29253e433 100644 --- a/datapath/linux/compat/include/linux/skbuff.h +++ b/datapath/linux/compat/include/linux/skbuff.h @@ -278,7 +278,7 @@ 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_HASH) && !defined(HAVE_RHEL_OVS_HOOK) skb->l4_rxhash = 0; #endif } @@ -465,7 +465,7 @@ __skb_set_hash(struct sk_buff *skb, __u32 hash, bool is_sw, bool is_l4) #else skb->hash = hash; #endif -#if defined(HAVE_L4_RXHASH) +#if !defined(HAVE_L4_HASH) skb->l4_rxhash = is_l4; #else skb->l4_hash = is_l4;
The skb now uses l4_hash and it is easier to check for it. Also fixes a compile error for RHEL 7.7. Signed-off-by: Greg Rose <gvrose8192@gmail.com> --- acinclude.m4 | 4 ++-- datapath/datapath.c | 6 +++--- datapath/linux/compat/include/linux/skbuff.h | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-)