[dpdk-dev] [PATCH 0/4] Enhance CPU flag support

[dpdk-dev] [PATCH 0/4] Enhance CPU flag support

[Bruce Richardson]

Currently in DPDK, any checks for the presence or absense of specific
CPU flags has to be enclosed in #ifdef blocks, since the flags are
conditionally defined per architecture as enums. This set attempts to
improve this situation by allowing the flags to be queried as strings,
with a CPU architecture type also specified. One new public API is
provided for this. [Other APIs used by that are internal for now, but
have an rte_ prefix in case we want to expose them publically later]

To test out this new functionality the crc code in the net library is
updated to use this, demonstrating how the code can be simplified and
the use of #ifdef's reduced.

Bruce Richardson (4):
  build: fix quoting on RTE_ARCH string value
  config/arm: fix missing define for arm platforms
  eal: allow checking CPU flags by name
  net: replace ifdefs with runtime branches

 config/arm/meson.build                        |  2 +
 config/ppc_64/meson.build                     |  2 +-
 config/x86/meson.build                        |  4 +-
 lib/librte_eal/common/eal_common_cpuflags.c   | 41 +++++++++++++++++
 .../common/include/generic/rte_cpuflags.h     | 44 ++++++++++++++++++-
 lib/librte_eal/rte_eal_version.map            |  3 ++
 lib/librte_net/rte_net_crc.c                  | 38 ++++++++++------
 7 files changed, 117 insertions(+), 17 deletions(-)

-- 
2.21.0

[dpdk-dev] [PATCH 1/4] build: fix quoting on RTE_ARCH string value

[Bruce Richardson]

The value for RTE_ARCH needs to be quoted when output to the
rte_build_config.h file, so use "set_quoted" rather than "set" when
assigning it.

Fixes: a25a650be5f0 ("build: add infrastructure for meson and ninja builds")
Fixes: 54d609a13876 ("build: add ppc64 meson build")
Cc: bluca@debian.org
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 config/ppc_64/meson.build | 2 +-
 config/x86/meson.build    | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/config/ppc_64/meson.build b/config/ppc_64/meson.build
index 7ceae1d39..9246d319c 100644
--- a/config/ppc_64/meson.build
+++ b/config/ppc_64/meson.build
@@ -4,7 +4,7 @@
 if not dpdk_conf.get('RTE_ARCH_64')
 	error('Only 64-bit compiles are supported for this platform type')
 endif
-dpdk_conf.set('RTE_ARCH', 'ppc_64')
+dpdk_conf.set_quoted('RTE_ARCH', 'ppc_64')
 dpdk_conf.set('RTE_ARCH_PPC_64', 1)
 
 # overrides specific to ppc64
diff --git a/config/x86/meson.build b/config/x86/meson.build
index bb23771b4..1a88e52d9 100644
--- a/config/x86/meson.build
+++ b/config/x86/meson.build
@@ -31,10 +31,10 @@ endforeach
 dpdk_conf.set('RTE_ARCH_X86', 1)
 if dpdk_conf.get('RTE_ARCH_64')
 	dpdk_conf.set('RTE_ARCH_X86_64', 1)
-	dpdk_conf.set('RTE_ARCH', 'x86_64')
+	dpdk_conf.set_quoted('RTE_ARCH', 'x86_64')
 else
 	dpdk_conf.set('RTE_ARCH_I686', 1)
-	dpdk_conf.set('RTE_ARCH', 'i686')
+	dpdk_conf.set_quoted('RTE_ARCH', 'i686')
 endif
 
 if cc.get_define('__AES__', args: machine_args) != ''
-- 
2.21.0

[dpdk-dev] [PATCH 2/4] config/arm: fix missing define for arm platforms

[Bruce Richardson]

The RTE_ARCH macro was defined for all platforms when using make, but was
only defined for x86 and ppc platforms when using meson. Fix this omission.

Fixes: b1d48c41189a ("build: support ARM with meson")
Cc: stable@dpdk.org

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 config/arm/meson.build | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/config/arm/meson.build b/config/arm/meson.build
index 7fa6ed310..a4eb1fc50 100644
--- a/config/arm/meson.build
+++ b/config/arm/meson.build
@@ -113,6 +113,7 @@ dpdk_conf.set('RTE_FORCE_INTRINSICS', 1)
 
 if not dpdk_conf.get('RTE_ARCH_64')
 	dpdk_conf.set('RTE_CACHE_LINE_SIZE', 64)
+	dpdk_conf.set_quoted('RTE_ARCH', 'arm')
 	dpdk_conf.set('RTE_ARCH_ARM', 1)
 	dpdk_conf.set('RTE_ARCH_ARMv7', 1)
 	# the minimum architecture supported, armv7-a, needs the following,
@@ -120,6 +121,7 @@ if not dpdk_conf.get('RTE_ARCH_64')
 	machine_args += '-mfpu=neon'
 else
 	dpdk_conf.set('RTE_CACHE_LINE_SIZE', 128)
+	dpdk_conf.set_quoted('RTE_ARCH', 'arm64')
 	dpdk_conf.set('RTE_ARCH_ARM64', 1)
 
 	machine = []
-- 
2.21.0

[dpdk-dev] [PATCH 3/4] eal: allow checking CPU flags by name

[Bruce Richardson]

Rather than using enum values for CPU flags, which means the symbols don't
exist on other architectures, provide a flag lookup by name, allowing us to
unconditionally check for a CPU flag.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/librte_eal/common/eal_common_cpuflags.c   | 41 +++++++++++++++++
 .../common/include/generic/rte_cpuflags.h     | 44 ++++++++++++++++++-
 lib/librte_eal/rte_eal_version.map            |  3 ++
 3 files changed, 87 insertions(+), 1 deletion(-)

diff --git a/lib/librte_eal/common/eal_common_cpuflags.c b/lib/librte_eal/common/eal_common_cpuflags.c
index 3a055f7c7..5084a3e7a 100644
--- a/lib/librte_eal/common/eal_common_cpuflags.c
+++ b/lib/librte_eal/common/eal_common_cpuflags.c
@@ -3,6 +3,7 @@
  */
 
 #include <stdio.h>
+#include <string.h>
 
 #include <rte_common.h>
 #include <rte_cpuflags.h>
@@ -48,3 +49,43 @@ rte_cpu_is_supported(void)
 
 	return 1;
 }
+
+static enum rte_cpu_flag_t
+rte_cpu_get_flag(const char *flagname)
+{
+	int i;
+
+	if (flagname == NULL)
+		return RTE_CPUFLAG_NUMFLAGS;
+
+	for (i = 0; i < RTE_CPUFLAG_NUMFLAGS; i++)
+		if (strcmp(flagname, rte_cpu_get_flag_name(i)) == 0)
+			break;
+	return i;
+}
+
+static int
+rte_cpu_is_architecture(enum rte_cpu_arch arch)
+{
+	switch (arch) {
+	case rte_cpu_arch_arm:
+		return strcmp(RTE_ARCH, "arm") == 0 ||
+				strcmp(RTE_ARCH, "arm64") == 0;
+	case rte_cpu_arch_ppc:
+		return strcmp(RTE_ARCH, "ppc_64") == 0;
+	case rte_cpu_arch_x86:
+		return strcmp(RTE_ARCH, "x86_64") == 0 ||
+				strcmp(RTE_ARCH, "i686") == 0;
+	default:
+		return -EINVAL;
+	}
+}
+
+int
+rte_cpu_get_flagname_enabled(enum rte_cpu_arch arch, const char *flagname)
+{
+	if (!rte_cpu_is_architecture(arch))
+		return 0;
+
+	return rte_cpu_get_flag_enabled(rte_cpu_get_flag(flagname)) == 1;
+}
diff --git a/lib/librte_eal/common/include/generic/rte_cpuflags.h b/lib/librte_eal/common/include/generic/rte_cpuflags.h
index 156ea0029..a53551eba 100644
--- a/lib/librte_eal/common/include/generic/rte_cpuflags.h
+++ b/lib/librte_eal/common/include/generic/rte_cpuflags.h
@@ -10,7 +10,8 @@
  * Architecture specific API to determine available CPU features at runtime.
  */
 
-#include "rte_common.h"
+#include <rte_common.h>
+#include <rte_compat.h>
 #include <errno.h>
 
 /**
@@ -46,6 +47,47 @@ __extension__
 int
 rte_cpu_get_flag_enabled(enum rte_cpu_flag_t feature);
 
+/**
+ * Enumeration of the various CPU architectures supported by DPDK.
+ *
+ * When checking for CPU flags by name, it's possible that multiple
+ * architectures have flags with the same name e.g. AES is defined in
+ * both arm and x86 feature lists. Therefore we need to pass in at runtime
+ * the architecture we are checking for as well as the CPU flag. This enum
+ * defines the various supported architectures to be used for that checking.
+ */
+enum rte_cpu_arch {
+	rte_cpu_arch_arm = 0,
+	rte_cpu_arch_ppc,
+	rte_cpu_arch_x86,
+
+	rte_cpu_num_arch /* must always be the last */
+};
+
+/**
+ * Function for checking if a named CPU flag is enabled
+ *
+ * Wrapper around the rte_cpu_get_flag() and rte_cpu_get_flag_enabled()
+ * calls, which is safe to use even if the flag doesn't exist on target
+ * architecture. The function also verifies the target architecture so that
+ * we can distinguish e.g. AES support for arm vs x86 platforms.
+ *
+ * Note: This function uses multiple string compares in its operation and
+ * so is not recommended for data-path use. It should be called once, and
+ * the return value cached for later use.
+ *
+ * @param arch
+ *   The architecture on which we need to check the flag, since multiple
+ *   architectures could have flags with the same name.
+ * @param flagname
+ *   The name of the flag to query
+ * @return
+ *   1 if flag is available
+ *   0 if flag is not unavailable or invalid
+ */
+__rte_experimental int
+rte_cpu_get_flagname_enabled(enum rte_cpu_arch arch, const char *flagname);
+
 /**
  * This function checks that the currently used CPU supports the CPU features
  * that were specified at compile time. It is called automatically within the
diff --git a/lib/librte_eal/rte_eal_version.map b/lib/librte_eal/rte_eal_version.map
index 245493461..5ca5584a5 100644
--- a/lib/librte_eal/rte_eal_version.map
+++ b/lib/librte_eal/rte_eal_version.map
@@ -378,4 +378,7 @@ EXPERIMENTAL {
 	rte_service_lcore_attr_get;
 	rte_service_lcore_attr_reset_all;
 	rte_service_may_be_active;
+
+	# added in 19.08
+	rte_cpu_get_flagname_enabled;
 };
-- 
2.21.0

[dpdk-dev] [PATCH 4/4] net: replace ifdefs with runtime branches

[Bruce Richardson]

Use the flag checking functions and a couple of empty stubs to remove the
ifdefs from the middle of the C code, and replace them with more readable
regular if statements. Other ifdefs at the top of the file are kept, but
are not mixed with C code, so there is a clean separation.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/librte_net/rte_net_crc.c | 38 ++++++++++++++++++++++++------------
 1 file changed, 25 insertions(+), 13 deletions(-)

diff --git a/lib/librte_net/rte_net_crc.c b/lib/librte_net/rte_net_crc.c
index dca0830e2..3b8a09504 100644
--- a/lib/librte_net/rte_net_crc.c
+++ b/lib/librte_net/rte_net_crc.c
@@ -18,8 +18,17 @@
 
 #ifdef X86_64_SSE42_PCLMULQDQ
 #include <net_crc_sse.h>
-#elif defined ARM64_NEON_PMULL
+#else
+/* define stubs for the SSE functions to avoid compiler errors */
+#define handlers_sse42 handlers_scalar
+#define rte_net_crc_sse42_init() do { } while(0)
+#endif
+
+#ifdef ARM64_NEON_PMULL
 #include <net_crc_neon.h>
+#else
+#define handlers_neon handlers_scalar
+#define rte_net_crc_neon_init() do { } while(0)
 #endif
 
 /* crc tables */
@@ -140,18 +149,19 @@ void
 rte_net_crc_set_alg(enum rte_net_crc_alg alg)
 {
 	switch (alg) {
-#ifdef X86_64_SSE42_PCLMULQDQ
 	case RTE_NET_CRC_SSE42:
-		handlers = handlers_sse42;
-		break;
-#elif defined ARM64_NEON_PMULL
+		if (rte_cpu_get_flagname_enabled(rte_cpu_arch_x86,
+				"RTE_CPUFLAG_SSE4_2")) {
+			handlers = handlers_sse42;
+			break;
+		}
 		/* fall-through */
 	case RTE_NET_CRC_NEON:
-		if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_PMULL)) {
+		if (rte_cpu_get_flagname_enabled(rte_cpu_arch_arm,
+				"RTE_CPUFLAG_PMULL")) {
 			handlers = handlers_neon;
 			break;
 		}
-#endif
 		/* fall-through */
 	case RTE_NET_CRC_SCALAR:
 		/* fall-through */
@@ -182,15 +192,17 @@ RTE_INIT(rte_net_crc_init)
 
 	rte_net_crc_scalar_init();
 
-#ifdef X86_64_SSE42_PCLMULQDQ
-	alg = RTE_NET_CRC_SSE42;
-	rte_net_crc_sse42_init();
-#elif defined ARM64_NEON_PMULL
-	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_PMULL)) {
+	if (rte_cpu_get_flagname_enabled(rte_cpu_arch_x86,
+			"RTE_CPUFLAG_SSE4_2")) {
+		alg = RTE_NET_CRC_SSE42;
+		rte_net_crc_sse42_init();
+	}
+
+	if (rte_cpu_get_flagname_enabled(rte_cpu_arch_arm,
+			"RTE_CPUFLAG_PMULL")) {
 		alg = RTE_NET_CRC_NEON;
 		rte_net_crc_neon_init();
 	}
-#endif
 
 	rte_net_crc_set_alg(alg);
 }
-- 
2.21.0

Re: [dpdk-dev] [PATCH 1/4] build: fix quoting on RTE_ARCH string value

[Luca Boccassi]

On Wed, 2019-05-29 at 16:41 +0100, Bruce Richardson wrote:
> The value for RTE_ARCH needs to be quoted when output to the
> rte_build_config.h file, so use "set_quoted" rather than "set" when
> assigning it.
> 
> Fixes: a25a650be5f0 ("build: add infrastructure for meson and ninja
> builds")
> Fixes: 54d609a13876 ("build: add ppc64 meson build")
> Cc: 
> bluca@debian.org
> 
> Cc: 
> stable@dpdk.org
> 
> 
> Signed-off-by: Bruce Richardson <
> bruce.richardson@intel.com
> >
> ---
>  config/ppc_64/meson.build | 2 +-
>  config/x86/meson.build    | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)

Acked-by: Luca Boccassi <bluca@debian.org>

-- 
Kind regards,
Luca Boccassi

Re: [dpdk-dev] [PATCH 0/4] Enhance CPU flag support

[Ananyev, Konstantin]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Bruce Richardson
> Sent: Wednesday, May 29, 2019 4:41 PM
> To: dev@dpdk.org
> Cc: Richardson, Bruce <bruce.richardson@intel.com>
> Subject: [dpdk-dev] [PATCH 0/4] Enhance CPU flag support
> 
> Currently in DPDK, any checks for the presence or absense of specific
> CPU flags has to be enclosed in #ifdef blocks, since the flags are
> conditionally defined per architecture as enums. This set attempts to
> improve this situation by allowing the flags to be queried as strings,
> with a CPU architecture type also specified. One new public API is
> provided for this. [Other APIs used by that are internal for now, but
> have an rte_ prefix in case we want to expose them publically later]
> 
> To test out this new functionality the crc code in the net library is
> updated to use this, demonstrating how the code can be simplified and
> the use of #ifdef's reduced.
> 
> Bruce Richardson (4):
>   build: fix quoting on RTE_ARCH string value
>   config/arm: fix missing define for arm platforms
>   eal: allow checking CPU flags by name
>   net: replace ifdefs with runtime branches
> 
>  config/arm/meson.build                        |  2 +
>  config/ppc_64/meson.build                     |  2 +-
>  config/x86/meson.build                        |  4 +-
>  lib/librte_eal/common/eal_common_cpuflags.c   | 41 +++++++++++++++++
>  .../common/include/generic/rte_cpuflags.h     | 44 ++++++++++++++++++-
>  lib/librte_eal/rte_eal_version.map            |  3 ++
>  lib/librte_net/rte_net_crc.c                  | 38 ++++++++++------
>  7 files changed, 117 insertions(+), 17 deletions(-)
> 
> --

Series Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> 2.21.0

Re: [dpdk-dev] [PATCH 3/4] eal: allow checking CPU flags by name

[David Marchand]

On Wed, May 29, 2019 at 5:42 PM Bruce Richardson <bruce.richardson@intel.com>
wrote:

> Rather than using enum values for CPU flags, which means the symbols don't
> exist on other architectures, provide a flag lookup by name, allowing us to
> unconditionally check for a CPU flag.
>

Did you consider passing a string for the CPU architecture rather than an
enum?
It would have to be compared to RTE_ARCH in rte_cpu_get_flagname_enabled.
Or to accomodate with x86_64/i686, this could be a cpu arch family.

This avoids adding a new C type that seems quite limited wrt its uses.

-- 
David Marchand

Re: [dpdk-dev] [PATCH 3/4] eal: allow checking CPU flags by name

[Bruce Richardson]

On Thu, Jun 27, 2019 at 03:22:14PM +0200, David Marchand wrote:
>    On Wed, May 29, 2019 at 5:42 PM Bruce Richardson
>    <[1]bruce.richardson@intel.com> wrote:
> 
>      Rather than using enum values for CPU flags, which means the symbols
>      don't
>      exist on other architectures, provide a flag lookup by name,
>      allowing us to
>      unconditionally check for a CPU flag.
> 
>    Did you consider passing a string for the CPU architecture rather than
>    an enum?
>    It would have to be compared to RTE_ARCH in
>    rte_cpu_get_flagname_enabled.
>    Or to accomodate with x86_64/i686, this could be a cpu arch family.
>    This avoids adding a new C type that seems quite limited wrt its uses.
>    --
>    David Marchand
>

I'm not sure I really see the value in having string names for the
architecture values, I think it would be a lot more clunky to manage rather
than having an enum value. The key difference vs the flags is that the
flags are only valid per-architecture while the architecture defines can be
globally valid, and secondly there is a finite, and small, number of
architectures compared to the number of flags supported.

If you feel strongly about it I can investigate it, but I'm not sure I see
the value in doing so right now if the only benefit is avoiding the enum.

/Bruce

Re: [dpdk-dev] [PATCH 3/4] eal: allow checking CPU flags by name

[David Marchand]

On Fri, Jun 28, 2019 at 2:40 PM Bruce Richardson <bruce.richardson@intel.com>
wrote:

> On Thu, Jun 27, 2019 at 03:22:14PM +0200, David Marchand wrote:
> >    On Wed, May 29, 2019 at 5:42 PM Bruce Richardson
> >    <[1]bruce.richardson@intel.com> wrote:
> >
> >      Rather than using enum values for CPU flags, which means the symbols
> >      don't
> >      exist on other architectures, provide a flag lookup by name,
> >      allowing us to
> >      unconditionally check for a CPU flag.
> >
> >    Did you consider passing a string for the CPU architecture rather than
> >    an enum?
> >    It would have to be compared to RTE_ARCH in
> >    rte_cpu_get_flagname_enabled.
> >    Or to accomodate with x86_64/i686, this could be a cpu arch family.
> >    This avoids adding a new C type that seems quite limited wrt its uses.
> >    --
> >    David Marchand
> >
>
> I'm not sure I really see the value in having string names for the
> architecture values, I think it would be a lot more clunky to manage rather
> than having an enum value. The key difference vs the flags is that the
> flags are only valid per-architecture while the architecture defines can be
> globally valid, and secondly there is a finite, and small, number of
> architectures compared to the number of flags supported.
>
> If you feel strongly about it I can investigate it, but I'm not sure I see
> the value in doing so right now if the only benefit is avoiding the enum.
>

I suppose we won't have too much trouble handling ABI breakage (thinking
about when we will remove x86 support).
Ok, let's go with this.


-- 
David Marchand

Re: [dpdk-dev] [PATCH 4/4] net: replace ifdefs with runtime branches

[Thomas Monjalon]

29/05/2019 17:41, Bruce Richardson:
> Use the flag checking functions and a couple of empty stubs to remove the
> ifdefs from the middle of the C code, and replace them with more readable
> regular if statements. Other ifdefs at the top of the file are kept, but
> are not mixed with C code, so there is a clean separation.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  lib/librte_net/rte_net_crc.c | 38 ++++++++++++++++++++++++------------
>  1 file changed, 25 insertions(+), 13 deletions(-)

The result is more lines of code :)

> --- a/lib/librte_net/rte_net_crc.c
> +++ b/lib/librte_net/rte_net_crc.c
> @@ -18,8 +18,17 @@
>  
>  #ifdef X86_64_SSE42_PCLMULQDQ
>  #include <net_crc_sse.h>
> -#elif defined ARM64_NEON_PMULL
> +#else
> +/* define stubs for the SSE functions to avoid compiler errors */
> +#define handlers_sse42 handlers_scalar
> +#define rte_net_crc_sse42_init() do { } while(0)
> +#endif
> +
> +#ifdef ARM64_NEON_PMULL
>  #include <net_crc_neon.h>
> +#else
> +#define handlers_neon handlers_scalar
> +#define rte_net_crc_neon_init() do { } while(0)
>  #endif

Looking at the need for stubs, I don't see the benefit.

>  rte_net_crc_set_alg(enum rte_net_crc_alg alg)
>  {
>  	switch (alg) {
> -#ifdef X86_64_SSE42_PCLMULQDQ
>  	case RTE_NET_CRC_SSE42:
> -		handlers = handlers_sse42;
> -		break;
> -#elif defined ARM64_NEON_PMULL
> +		if (rte_cpu_get_flagname_enabled(rte_cpu_arch_x86,
> +				"RTE_CPUFLAG_SSE4_2")) {

Why the CPU flags strings are prefixed with RTE_CPUFLAG_?

Re: [dpdk-dev] [PATCH 4/4] net: replace ifdefs with runtime branches

[Bruce Richardson]

On Mon, Jul 01, 2019 at 09:30:02PM +0200, Thomas Monjalon wrote:
> 29/05/2019 17:41, Bruce Richardson:
> > Use the flag checking functions and a couple of empty stubs to remove the
> > ifdefs from the middle of the C code, and replace them with more readable
> > regular if statements. Other ifdefs at the top of the file are kept, but
> > are not mixed with C code, so there is a clean separation.
> > 
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> >  lib/librte_net/rte_net_crc.c | 38 ++++++++++++++++++++++++------------
> >  1 file changed, 25 insertions(+), 13 deletions(-)
> 
> The result is more lines of code :)
> 
> > --- a/lib/librte_net/rte_net_crc.c
> > +++ b/lib/librte_net/rte_net_crc.c
> > @@ -18,8 +18,17 @@
> >  
> >  #ifdef X86_64_SSE42_PCLMULQDQ
> >  #include <net_crc_sse.h>
> > -#elif defined ARM64_NEON_PMULL
> > +#else
> > +/* define stubs for the SSE functions to avoid compiler errors */
> > +#define handlers_sse42 handlers_scalar
> > +#define rte_net_crc_sse42_init() do { } while(0)
> > +#endif
> > +
> > +#ifdef ARM64_NEON_PMULL
> >  #include <net_crc_neon.h>
> > +#else
> > +#define handlers_neon handlers_scalar
> > +#define rte_net_crc_neon_init() do { } while(0)
> >  #endif
> 
> Looking at the need for stubs, I don't see the benefit.
>
Yes, one needs stubs, but those are placed in a single place, and the main
C-code itself is free of ifdefs running through it. I'd view this as a good
thing in limiting the scope of any ifdef-ery, since it annoys me looking at
#ifdefs in the middle of functions (since it messes up the indentation flow
of the code if nothing else!).

If you don't see this as a big benefit, then there is not a lot else to
commend this set for you, sadly. It just seemed a nice improvement to me -
irrespective of net lines of code.

/Bruce 

Re: [dpdk-dev] [PATCH 4/4] net: replace ifdefs with runtime branches

[Thomas Monjalon]

01/07/2019 22:41, Bruce Richardson:
> On Mon, Jul 01, 2019 at 09:30:02PM +0200, Thomas Monjalon wrote:
> > 29/05/2019 17:41, Bruce Richardson:
> > > Use the flag checking functions and a couple of empty stubs to remove the
> > > ifdefs from the middle of the C code, and replace them with more readable
> > > regular if statements. Other ifdefs at the top of the file are kept, but
> > > are not mixed with C code, so there is a clean separation.
> > > 
> > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > > ---
> > >  lib/librte_net/rte_net_crc.c | 38 ++++++++++++++++++++++++------------
> > >  1 file changed, 25 insertions(+), 13 deletions(-)
> > 
> > The result is more lines of code :)
> > 
> > > --- a/lib/librte_net/rte_net_crc.c
> > > +++ b/lib/librte_net/rte_net_crc.c
> > > @@ -18,8 +18,17 @@
> > >  
> > >  #ifdef X86_64_SSE42_PCLMULQDQ
> > >  #include <net_crc_sse.h>
> > > -#elif defined ARM64_NEON_PMULL
> > > +#else
> > > +/* define stubs for the SSE functions to avoid compiler errors */
> > > +#define handlers_sse42 handlers_scalar
> > > +#define rte_net_crc_sse42_init() do { } while(0)
> > > +#endif
> > > +
> > > +#ifdef ARM64_NEON_PMULL
> > >  #include <net_crc_neon.h>
> > > +#else
> > > +#define handlers_neon handlers_scalar
> > > +#define rte_net_crc_neon_init() do { } while(0)
> > >  #endif
> > 
> > Looking at the need for stubs, I don't see the benefit.
> >
> Yes, one needs stubs, but those are placed in a single place, and the main
> C-code itself is free of ifdefs running through it. I'd view this as a good
> thing in limiting the scope of any ifdef-ery, since it annoys me looking at
> #ifdefs in the middle of functions (since it messes up the indentation flow
> of the code if nothing else!).
> 
> If you don't see this as a big benefit, then there is not a lot else to
> commend this set for you, sadly. It just seemed a nice improvement to me -
> irrespective of net lines of code.

Any other opinion?

Re: [dpdk-dev] [PATCH 4/4] net: replace ifdefs with runtime branches

[David Christensen]

> 01/07/2019 22:41, Bruce Richardson:
>> On Mon, Jul 01, 2019 at 09:30:02PM +0200, Thomas Monjalon wrote:
>>> 29/05/2019 17:41, Bruce Richardson:
>>>> Use the flag checking functions and a couple of empty stubs to remove the
>>>> ifdefs from the middle of the C code, and replace them with more readable
>>>> regular if statements. Other ifdefs at the top of the file are kept, but
>>>> are not mixed with C code, so there is a clean separation.
>>>>
>>>> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
>>>> ---
>>>>   lib/librte_net/rte_net_crc.c | 38 ++++++++++++++++++++++++------------
>>>>   1 file changed, 25 insertions(+), 13 deletions(-)
>>>
>>> The result is more lines of code :)
>>>
>>>> --- a/lib/librte_net/rte_net_crc.c
>>>> +++ b/lib/librte_net/rte_net_crc.c
>>>> @@ -18,8 +18,17 @@
>>>>   
>>>>   #ifdef X86_64_SSE42_PCLMULQDQ
>>>>   #include <net_crc_sse.h>
>>>> -#elif defined ARM64_NEON_PMULL
>>>> +#else
>>>> +/* define stubs for the SSE functions to avoid compiler errors */
>>>> +#define handlers_sse42 handlers_scalar
>>>> +#define rte_net_crc_sse42_init() do { } while(0)
>>>> +#endif
>>>> +
>>>> +#ifdef ARM64_NEON_PMULL
>>>>   #include <net_crc_neon.h>
>>>> +#else
>>>> +#define handlers_neon handlers_scalar
>>>> +#define rte_net_crc_neon_init() do { } while(0)
>>>>   #endif
>>>
>>> Looking at the need for stubs, I don't see the benefit.
>>>
>> Yes, one needs stubs, but those are placed in a single place, and the main
>> C-code itself is free of ifdefs running through it. I'd view this as a good
>> thing in limiting the scope of any ifdef-ery, since it annoys me looking at
>> #ifdefs in the middle of functions (since it messes up the indentation flow
>> of the code if nothing else!).
>>
>> If you don't see this as a big benefit, then there is not a lot else to
>> commend this set for you, sadly. It just seemed a nice improvement to me -
>> irrespective of net lines of code.
> 
> Any other opinion?

I support adding a few lines of code in the slow path to provide cleaner 
separation of architecture specific code, though the existing IFDEF code 
doesn't appear very intrusive in this case.  My preference would be 
architecture specific header files and strong/weak linking to pull in 
the appropriate code.

Dave