[dpdk-dev] [PATCH] kni: fixed compilation error on Ubuntu 14.04 LTS (kernel 3.13.0-30.54)

[dpdk-dev] [PATCH] kni: fixed compilation error on Ubuntu 14.04 LTS (kernel 3.13.0-30.54)

[Pablo de Lara]

Recent Ubuntu kernel 3.13.0-30.54, although based on Linux kernel 3.13.11,
already provides skb_set_hash() inline function, slightly different than
the one provided by lib/librte_eal/linuxapp/kni/ethtool/igb/kcompat.h

Ubuntu kernel 3.13.0-30.54 provides:
    * i40e/i40evf: i40e implementation for skb_set_hash
    - https://bugs.launchpad.net/bugs/1328037
    - http://changelogs.ubuntu.com/changelogs/pool/main/l/linux/linux_3.13.0-30.54/changelog

As a result, the implementation provided by kcompat.h must be skipped.
It is not appropriate to test whether LINUX_VERSION_CODE >= KERNEL_VERSION(3,13,11)
because previous Ubuntu kernel 3.13.0-29.53, already based on 3.13.11, needs to
get the implementation provided by kcompat.h

So the full Ubuntu kernel version numbering scheme must be tested:
<base kernel version>-<ABI number>.<upload number>-<flavour>
See "What does a specific Ubuntu kernel version number mean?"
and "How can we determine the version of the running kernel?"
at: https://wiki.ubuntu.com/Kernel/FAQ

Unlike RHEL_RELEASE_CODE, there is no such UBUNTU_RELEASE_CODE available out of
the box, so it needs to be crafted from the Makefile
Similarly, UBUNTU_KERNEL_CODE is generated with ABI and upload numbers.

`lsb_release -si` is first used to check whether we are running Ubuntu
`lsb_release -sr` provides release number 14.04, then converted to integer 1404
/proc/version_signature is parsed to get base kernel version, ABI and upload
numbers, and flavour is dropped

UBUNTU_KERNEL_CODE is indirectly defined using the UBUNTU_KERNEL_VERSION macro,
which in turn is defined in kcompat.h
This makes a single place to define the Ubuntu kernel version numbering scheme,
which is slightly different than the usual "shift by 8" scheme: ABI numbers can
be big (see: https://wiki.ubuntu.com/Kernel/Dev/TopicBranches), so 16-bits have
been reserved for them.

Finally, the implementaion of skb_set_hash is skipped in kcompat.h if we are
running Ubuntu 14.04 with an Ubuntu kernel >= 3.13.0-30.54

Signed-off-by: Patrice Buriez <patrice.buriez@intel.com>
---
 lib/librte_eal/linuxapp/kni/Makefile              |    9 +++++++++
 lib/librte_eal/linuxapp/kni/ethtool/igb/kcompat.h |   16 ++++++++++++++++
 2 files changed, 25 insertions(+), 0 deletions(-)

diff --git a/lib/librte_eal/linuxapp/kni/Makefile b/lib/librte_eal/linuxapp/kni/Makefile
index fb9462f..725d3e7 100644
--- a/lib/librte_eal/linuxapp/kni/Makefile
+++ b/lib/librte_eal/linuxapp/kni/Makefile
@@ -44,6 +44,15 @@ MODULE_CFLAGS += -I$(RTE_OUTPUT)/include -I$(SRCDIR)/ethtool/ixgbe -I$(SRCDIR)/e
 MODULE_CFLAGS += -include $(RTE_OUTPUT)/include/rte_config.h
 MODULE_CFLAGS += -Wall -Werror
 
+ifeq ($(shell type lsb_release >/dev/null 2>&1 && lsb_release -si),Ubuntu)
+MODULE_CFLAGS += -DUBUNTU_RELEASE_CODE=$(subst .,,$(shell lsb_release -sr))
+UBUNTU_KERNEL_CODE := $(shell cut -d' ' -f2 /proc/version_signature |cut -d- -f1,2)
+UBUNTU_KERNEL_CODE := $(subst -,$(comma),$(UBUNTU_KERNEL_CODE))
+UBUNTU_KERNEL_CODE := $(subst .,$(comma),$(UBUNTU_KERNEL_CODE))
+MODULE_CFLAGS += -D"UBUNTU_KERNEL_CODE=UBUNTU_KERNEL_VERSION($(UBUNTU_KERNEL_CODE))"
+endif
+
+
 # this lib needs main eal
 DEPDIRS-y += lib/librte_eal/linuxapp/eal
 
diff --git a/lib/librte_eal/linuxapp/kni/ethtool/igb/kcompat.h b/lib/librte_eal/linuxapp/kni/ethtool/igb/kcompat.h
index 521a35d..5a06383 100644
--- a/lib/librte_eal/linuxapp/kni/ethtool/igb/kcompat.h
+++ b/lib/librte_eal/linuxapp/kni/ethtool/igb/kcompat.h
@@ -713,6 +713,20 @@ struct _kc_ethtool_pauseparam {
 #define SLE_VERSION_CODE 0
 #endif /* SLE_VERSION_CODE */
 
+/* Ubuntu release and kernel codes must be specified from Makefile */
+#ifndef UBUNTU_RELEASE_VERSION
+#define UBUNTU_RELEASE_VERSION(a,b) (((a) * 100) + (b))
+#endif
+#ifndef UBUNTU_KERNEL_VERSION
+#define UBUNTU_KERNEL_VERSION(a,b,c,abi,upload) (((a) << 40) + ((b) << 32) + ((c) << 24) + ((abi) << 8) + (upload))
+#endif
+#ifndef UBUNTU_RELEASE_CODE
+#define UBUNTU_RELEASE_CODE 0
+#endif
+#ifndef UBUNTU_KERNEL_CODE
+#define UBUNTU_KERNEL_CODE 0
+#endif
+
 #ifdef __KLOCWORK__
 #ifdef ARRAY_SIZE
 #undef ARRAY_SIZE
@@ -3847,6 +3861,7 @@ static inline struct sk_buff *__kc__vlan_hwaccel_put_tag(struct sk_buff *skb,
 
 #if ( LINUX_VERSION_CODE < KERNEL_VERSION(3,14,0) )
 #if (!(RHEL_RELEASE_CODE && RHEL_RELEASE_CODE >= RHEL_RELEASE_VERSION(7,0)))
+#if (!(UBUNTU_RELEASE_CODE == UBUNTU_RELEASE_VERSION(14,4) && UBUNTU_KERNEL_CODE >= UBUNTU_KERNEL_VERSION(3,13,0,30,54)))
 #ifdef NETIF_F_RXHASH
 #define PKT_HASH_TYPE_L3 0
 static inline void
@@ -3855,6 +3870,7 @@ skb_set_hash(struct sk_buff *skb, __u32 hash, __always_unused int type)
 	skb->rxhash = hash;
 }
 #endif /* NETIF_F_RXHASH */
+#endif /* < 3.13.0-30.54 (Ubuntu 14.04) */
 #endif /* < RHEL7 */
 #endif /* < 3.14.0 */
 
-- 
1.7.0.7

Re: [dpdk-dev] [PATCH] kni: fixed compilation error on Ubuntu 14.04 LTS (kernel 3.13.0-30.54)

[Thomas Monjalon]

> Unlike RHEL_RELEASE_CODE, there is no such UBUNTU_RELEASE_CODE available out of
> the box, so it needs to be crafted from the Makefile
> Similarly, UBUNTU_KERNEL_CODE is generated with ABI and upload numbers.

It's quite amazing to see that Linux distributions do backports and do not
provide a way to check them.
Anyway, thanks for the fix.

> +ifeq ($(shell type lsb_release >/dev/null 2>&1 && lsb_release -si),Ubuntu)

Why not this simpler form?
$(shell lsb_release -si 2>/dev/null)

> +MODULE_CFLAGS += -DUBUNTU_RELEASE_CODE=$(subst .,,$(shell lsb_release -sr))

Or you can use | tr -d . instead of subst and keep the flow from left to right.

> +UBUNTU_KERNEL_CODE := $(shell cut -d' ' -f2 /proc/version_signature |cut -d- -f1,2)
                                                                        ^
                                                         space missing here

> +UBUNTU_KERNEL_CODE := $(subst -,$(comma),$(UBUNTU_KERNEL_CODE))
> +UBUNTU_KERNEL_CODE := $(subst .,$(comma),$(UBUNTU_KERNEL_CODE))

Would be simpler with | tr -d .-

-- 
Thomas

Re: [dpdk-dev] [PATCH] kni: fixed compilation error on Ubuntu 14.04 LTS (kernel 3.13.0-30.54)

[Thomas Monjalon]

2014-07-24 16:54, Thomas Monjalon:
> > Unlike RHEL_RELEASE_CODE, there is no such UBUNTU_RELEASE_CODE available out of
> > the box, so it needs to be crafted from the Makefile
> > Similarly, UBUNTU_KERNEL_CODE is generated with ABI and upload numbers.
> 
> It's quite amazing to see that Linux distributions do backports and do not
> provide a way to check them.
> Anyway, thanks for the fix.
> 
> > +ifeq ($(shell type lsb_release >/dev/null 2>&1 && lsb_release -si),Ubuntu)
> 
> Why not this simpler form?
> $(shell lsb_release -si 2>/dev/null)
> 
> > +MODULE_CFLAGS += -DUBUNTU_RELEASE_CODE=$(subst .,,$(shell lsb_release -sr))
> 
> Or you can use | tr -d . instead of subst and keep the flow from left to right.
> 
> > +UBUNTU_KERNEL_CODE := $(shell cut -d' ' -f2 /proc/version_signature |cut -d- -f1,2)
>                                                                         ^
>                                                          space missing here
> 
> > +UBUNTU_KERNEL_CODE := $(subst -,$(comma),$(UBUNTU_KERNEL_CODE))
> > +UBUNTU_KERNEL_CODE := $(subst .,$(comma),$(UBUNTU_KERNEL_CODE))
> 
> Would be simpler with | tr -d .-

Sorry, I mean tr -d .- $(comma)

-- 
Thomas

Re: [dpdk-dev] [PATCH] kni: fixed compilation error on Ubuntu 14.04 LTS (kernel 3.13.0-30.54)

[Thomas Monjalon]

2014-07-24 16:31, Buriez, Patrice:
> > Why not this simpler form?
> > $(shell lsb_release -si 2>/dev/null)
> 
> I didn't want "make" to stop on error or to display a warning if lsb_release is not available on other distributions.
> I must admit that I focused on identifying the exact 5-tuple UBUNTU_KERNEL_CODE that was triggering the compilation error.
> Then I tried to keep the Makefile as simple and readable as possible, and took no shortcut.
> If your simpler form works the same, then indeed it's nicer than mine. ;-)
> 
> > > +MODULE_CFLAGS += -DUBUNTU_RELEASE_CODE=$(subst .,,$(shell lsb_release -sr))
> > 
> > Or you can use | tr -d . instead of subst and keep the flow from left to right.
> 
> Agreed. I seldom use tr and didn't figure out that it would perfectly fit here. Thanks!
> 
> > > +UBUNTU_KERNEL_CODE := $(shell cut -d' ' -f2 /proc/version_signature |cut -d- -f1,2)
> >                                                                         ^
> >                                                          space missing here
> 
> I usually pipe into the next command with no space in between. So that's somewhat on purpose.
> Is your comment cosmetic or about readability?
> Or are there situations that would fail, unless the space is provided between the pipe and the command?

Yes, only cosmetic.

> > > +UBUNTU_KERNEL_CODE := $(subst -,$(comma),$(UBUNTU_KERNEL_CODE))
> > > +UBUNTU_KERNEL_CODE := $(subst .,$(comma),$(UBUNTU_KERNEL_CODE))
> > 
> > Would be simpler with | tr -d .-
> 
> Agreed again (with the $(comma) from your next email, and without the -d in order to actually translate, not delete. ;-)

Yes "tr .- $(comma)" :)

> Again, I mainly focused on extracting and transmitting the 5-tuple UBUNTU_KERNEL_CODE from shell to compiler.
> I agree this can be rewritten in nicer ways, but it works, and hopefully does not break compilation on other distributions.

Acked and applied with above modifications.

Thanks
-- 
Thomas

Re: [dpdk-dev] [PATCH] kni: fixed compilation error on Ubuntu 14.04 LTS (kernel 3.13.0-30.54)

[Chris Wright]

* Pablo de Lara (pablo.de.lara.guarch@intel.com) wrote:
> Signed-off-by: Patrice Buriez <patrice.buriez@intel.com>

Just a mechanical nitpick on DCO.  Pablo, this patch appears to be
written by Patrice.  If so, it should begin with "From: Patrice Buriez
<patrice.buriez@intel.com>" and should include your own Signed-off-by.

thanks,
-chris

> ---
>  lib/librte_eal/linuxapp/kni/Makefile              |    9 +++++++++
>  lib/librte_eal/linuxapp/kni/ethtool/igb/kcompat.h |   16 ++++++++++++++++
>  2 files changed, 25 insertions(+), 0 deletions(-)
> 
> diff --git a/lib/librte_eal/linuxapp/kni/Makefile b/lib/librte_eal/linuxapp/kni/Makefile
> index fb9462f..725d3e7 100644
> --- a/lib/librte_eal/linuxapp/kni/Makefile
> +++ b/lib/librte_eal/linuxapp/kni/Makefile
> @@ -44,6 +44,15 @@ MODULE_CFLAGS += -I$(RTE_OUTPUT)/include -I$(SRCDIR)/ethtool/ixgbe -I$(SRCDIR)/e
>  MODULE_CFLAGS += -include $(RTE_OUTPUT)/include/rte_config.h
>  MODULE_CFLAGS += -Wall -Werror
>  
> +ifeq ($(shell type lsb_release >/dev/null 2>&1 && lsb_release -si),Ubuntu)
> +MODULE_CFLAGS += -DUBUNTU_RELEASE_CODE=$(subst .,,$(shell lsb_release -sr))
> +UBUNTU_KERNEL_CODE := $(shell cut -d' ' -f2 /proc/version_signature |cut -d- -f1,2)
> +UBUNTU_KERNEL_CODE := $(subst -,$(comma),$(UBUNTU_KERNEL_CODE))
> +UBUNTU_KERNEL_CODE := $(subst .,$(comma),$(UBUNTU_KERNEL_CODE))
> +MODULE_CFLAGS += -D"UBUNTU_KERNEL_CODE=UBUNTU_KERNEL_VERSION($(UBUNTU_KERNEL_CODE))"
> +endif
> +
> +
>  # this lib needs main eal
>  DEPDIRS-y += lib/librte_eal/linuxapp/eal
>  
> diff --git a/lib/librte_eal/linuxapp/kni/ethtool/igb/kcompat.h b/lib/librte_eal/linuxapp/kni/ethtool/igb/kcompat.h
> index 521a35d..5a06383 100644
> --- a/lib/librte_eal/linuxapp/kni/ethtool/igb/kcompat.h
> +++ b/lib/librte_eal/linuxapp/kni/ethtool/igb/kcompat.h
> @@ -713,6 +713,20 @@ struct _kc_ethtool_pauseparam {
>  #define SLE_VERSION_CODE 0
>  #endif /* SLE_VERSION_CODE */
>  
> +/* Ubuntu release and kernel codes must be specified from Makefile */
> +#ifndef UBUNTU_RELEASE_VERSION
> +#define UBUNTU_RELEASE_VERSION(a,b) (((a) * 100) + (b))
> +#endif
> +#ifndef UBUNTU_KERNEL_VERSION
> +#define UBUNTU_KERNEL_VERSION(a,b,c,abi,upload) (((a) << 40) + ((b) << 32) + ((c) << 24) + ((abi) << 8) + (upload))
> +#endif
> +#ifndef UBUNTU_RELEASE_CODE
> +#define UBUNTU_RELEASE_CODE 0
> +#endif
> +#ifndef UBUNTU_KERNEL_CODE
> +#define UBUNTU_KERNEL_CODE 0
> +#endif
> +
>  #ifdef __KLOCWORK__
>  #ifdef ARRAY_SIZE
>  #undef ARRAY_SIZE
> @@ -3847,6 +3861,7 @@ static inline struct sk_buff *__kc__vlan_hwaccel_put_tag(struct sk_buff *skb,
>  
>  #if ( LINUX_VERSION_CODE < KERNEL_VERSION(3,14,0) )
>  #if (!(RHEL_RELEASE_CODE && RHEL_RELEASE_CODE >= RHEL_RELEASE_VERSION(7,0)))
> +#if (!(UBUNTU_RELEASE_CODE == UBUNTU_RELEASE_VERSION(14,4) && UBUNTU_KERNEL_CODE >= UBUNTU_KERNEL_VERSION(3,13,0,30,54)))
>  #ifdef NETIF_F_RXHASH
>  #define PKT_HASH_TYPE_L3 0
>  static inline void
> @@ -3855,6 +3870,7 @@ skb_set_hash(struct sk_buff *skb, __u32 hash, __always_unused int type)
>  	skb->rxhash = hash;
>  }
>  #endif /* NETIF_F_RXHASH */
> +#endif /* < 3.13.0-30.54 (Ubuntu 14.04) */
>  #endif /* < RHEL7 */
>  #endif /* < 3.14.0 */
>  
> -- 
> 1.7.0.7
> 

Re: [dpdk-dev] [PATCH] kni: fixed compilation error on Ubuntu 14.04 LTS (kernel 3.13.0-30.54)

[Thomas Monjalon]

Hi Chris,

2014-07-24 08:20, Chris Wright:
> * Pablo de Lara (pablo.de.lara.guarch@intel.com) wrote:
> > Signed-off-by: Patrice Buriez <patrice.buriez@intel.com>
> 
> Just a mechanical nitpick on DCO.  Pablo, this patch appears to be
> written by Patrice.  If so, it should begin with "From: Patrice Buriez
> <patrice.buriez@intel.com>" and should include your own Signed-off-by.

I agree that the author name (From:) should be fixed.
But I'm not sure we should follow Linux guidelines for Signed-off-by
when patch is simply forwarded. Does it mean more than "I am authorized to
send it" ?

-- 
Thomas

Re: [dpdk-dev] [PATCH] kni: fixed compilation error on Ubuntu 14.04 LTS (kernel 3.13.0-30.54)

[Chris Wright]

* Thomas Monjalon (thomas.monjalon@6wind.com) wrote:
> 2014-07-24 08:20, Chris Wright:
> > * Pablo de Lara (pablo.de.lara.guarch@intel.com) wrote:
> > > Signed-off-by: Patrice Buriez <patrice.buriez@intel.com>
> > 
> > Just a mechanical nitpick on DCO.  Pablo, this patch appears to be
> > written by Patrice.  If so, it should begin with "From: Patrice Buriez
> > <patrice.buriez@intel.com>" and should include your own Signed-off-by.
> 
> I agree that the author name (From:) should be fixed.
> But I'm not sure we should follow Linux guidelines for Signed-off-by
> when patch is simply forwarded. Does it mean more than "I am authorized to
> send it" ?

Yeah, that's right.  Basically showing the chain of people who touched
the patch on the way to the repo.

thanks,
-chris