DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH 0/4] Enhance CPU flag support
@ 2019-05-29 15:41 Bruce Richardson
  2019-05-29 15:41 ` [dpdk-dev] [PATCH 1/4] build: fix quoting on RTE_ARCH string value Bruce Richardson
                   ` (4 more replies)
  0 siblings, 5 replies; 14+ messages in thread
From: Bruce Richardson @ 2019-05-29 15:41 UTC (permalink / raw)
  To: dev; +Cc: 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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [dpdk-dev] [PATCH 1/4] build: fix quoting on RTE_ARCH string value
  2019-05-29 15:41 [dpdk-dev] [PATCH 0/4] Enhance CPU flag support Bruce Richardson
@ 2019-05-29 15:41 ` Bruce Richardson
  2019-05-29 15:53   ` Luca Boccassi
  2019-05-29 15:41 ` [dpdk-dev] [PATCH 2/4] config/arm: fix missing define for arm platforms Bruce Richardson
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 14+ messages in thread
From: Bruce Richardson @ 2019-05-29 15:41 UTC (permalink / raw)
  To: dev; +Cc: Bruce Richardson, bluca, stable

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [dpdk-dev] [PATCH 2/4] config/arm: fix missing define for arm platforms
  2019-05-29 15:41 [dpdk-dev] [PATCH 0/4] Enhance CPU flag support Bruce Richardson
  2019-05-29 15:41 ` [dpdk-dev] [PATCH 1/4] build: fix quoting on RTE_ARCH string value Bruce Richardson
@ 2019-05-29 15:41 ` Bruce Richardson
  2019-05-29 15:41 ` [dpdk-dev] [PATCH 3/4] eal: allow checking CPU flags by name Bruce Richardson
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 14+ messages in thread
From: Bruce Richardson @ 2019-05-29 15:41 UTC (permalink / raw)
  To: dev; +Cc: Bruce Richardson, stable

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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [dpdk-dev] [PATCH 3/4] eal: allow checking CPU flags by name
  2019-05-29 15:41 [dpdk-dev] [PATCH 0/4] Enhance CPU flag support Bruce Richardson
  2019-05-29 15:41 ` [dpdk-dev] [PATCH 1/4] build: fix quoting on RTE_ARCH string value Bruce Richardson
  2019-05-29 15:41 ` [dpdk-dev] [PATCH 2/4] config/arm: fix missing define for arm platforms Bruce Richardson
@ 2019-05-29 15:41 ` Bruce Richardson
  2019-06-27 13:22   ` David Marchand
  2019-05-29 15:41 ` [dpdk-dev] [PATCH 4/4] net: replace ifdefs with runtime branches Bruce Richardson
  2019-06-27 12:39 ` [dpdk-dev] [PATCH 0/4] Enhance CPU flag support Ananyev, Konstantin
  4 siblings, 1 reply; 14+ messages in thread
From: Bruce Richardson @ 2019-05-29 15:41 UTC (permalink / raw)
  To: dev; +Cc: 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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* [dpdk-dev] [PATCH 4/4] net: replace ifdefs with runtime branches
  2019-05-29 15:41 [dpdk-dev] [PATCH 0/4] Enhance CPU flag support Bruce Richardson
                   ` (2 preceding siblings ...)
  2019-05-29 15:41 ` [dpdk-dev] [PATCH 3/4] eal: allow checking CPU flags by name Bruce Richardson
@ 2019-05-29 15:41 ` Bruce Richardson
  2019-07-01 19:30   ` Thomas Monjalon
  2019-06-27 12:39 ` [dpdk-dev] [PATCH 0/4] Enhance CPU flag support Ananyev, Konstantin
  4 siblings, 1 reply; 14+ messages in thread
From: Bruce Richardson @ 2019-05-29 15:41 UTC (permalink / raw)
  To: dev; +Cc: 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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [PATCH 1/4] build: fix quoting on RTE_ARCH string value
  2019-05-29 15:41 ` [dpdk-dev] [PATCH 1/4] build: fix quoting on RTE_ARCH string value Bruce Richardson
@ 2019-05-29 15:53   ` Luca Boccassi
  0 siblings, 0 replies; 14+ messages in thread
From: Luca Boccassi @ 2019-05-29 15:53 UTC (permalink / raw)
  To: Bruce Richardson, dev; +Cc: stable

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [PATCH 0/4] Enhance CPU flag support
  2019-05-29 15:41 [dpdk-dev] [PATCH 0/4] Enhance CPU flag support Bruce Richardson
                   ` (3 preceding siblings ...)
  2019-05-29 15:41 ` [dpdk-dev] [PATCH 4/4] net: replace ifdefs with runtime branches Bruce Richardson
@ 2019-06-27 12:39 ` Ananyev, Konstantin
  4 siblings, 0 replies; 14+ messages in thread
From: Ananyev, Konstantin @ 2019-06-27 12:39 UTC (permalink / raw)
  To: Richardson, Bruce, dev; +Cc: Richardson, Bruce



> -----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


^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [PATCH 3/4] eal: allow checking CPU flags by name
  2019-05-29 15:41 ` [dpdk-dev] [PATCH 3/4] eal: allow checking CPU flags by name Bruce Richardson
@ 2019-06-27 13:22   ` David Marchand
  2019-06-28 12:40     ` Bruce Richardson
  0 siblings, 1 reply; 14+ messages in thread
From: David Marchand @ 2019-06-27 13:22 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: dev

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [PATCH 3/4] eal: allow checking CPU flags by name
  2019-06-27 13:22   ` David Marchand
@ 2019-06-28 12:40     ` Bruce Richardson
  2019-06-28 13:34       ` David Marchand
  0 siblings, 1 reply; 14+ messages in thread
From: Bruce Richardson @ 2019-06-28 12:40 UTC (permalink / raw)
  To: David Marchand; +Cc: dev

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [PATCH 3/4] eal: allow checking CPU flags by name
  2019-06-28 12:40     ` Bruce Richardson
@ 2019-06-28 13:34       ` David Marchand
  0 siblings, 0 replies; 14+ messages in thread
From: David Marchand @ 2019-06-28 13:34 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: dev

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

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [PATCH 4/4] net: replace ifdefs with runtime branches
  2019-05-29 15:41 ` [dpdk-dev] [PATCH 4/4] net: replace ifdefs with runtime branches Bruce Richardson
@ 2019-07-01 19:30   ` Thomas Monjalon
  2019-07-01 20:41     ` Bruce Richardson
  0 siblings, 1 reply; 14+ messages in thread
From: Thomas Monjalon @ 2019-07-01 19:30 UTC (permalink / raw)
  To: Bruce Richardson; +Cc: dev

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_?



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [PATCH 4/4] net: replace ifdefs with runtime branches
  2019-07-01 19:30   ` Thomas Monjalon
@ 2019-07-01 20:41     ` Bruce Richardson
  2019-07-04 20:20       ` Thomas Monjalon
  0 siblings, 1 reply; 14+ messages in thread
From: Bruce Richardson @ 2019-07-01 20:41 UTC (permalink / raw)
  To: Thomas Monjalon; +Cc: dev

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 

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [PATCH 4/4] net: replace ifdefs with runtime branches
  2019-07-01 20:41     ` Bruce Richardson
@ 2019-07-04 20:20       ` Thomas Monjalon
  2019-07-08 17:24         ` David Christensen
  0 siblings, 1 reply; 14+ messages in thread
From: Thomas Monjalon @ 2019-07-04 20:20 UTC (permalink / raw)
  To: dev
  Cc: Bruce Richardson, david.marchand, David Christensen, Jerin Jacob,
	Gavin Hu

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?



^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [dpdk-dev] [PATCH 4/4] net: replace ifdefs with runtime branches
  2019-07-04 20:20       ` Thomas Monjalon
@ 2019-07-08 17:24         ` David Christensen
  0 siblings, 0 replies; 14+ messages in thread
From: David Christensen @ 2019-07-08 17:24 UTC (permalink / raw)
  To: Thomas Monjalon, dev
  Cc: Bruce Richardson, david.marchand, Jerin Jacob, Gavin Hu


> 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


^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, other threads:[~2019-07-08 17:24 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-29 15:41 [dpdk-dev] [PATCH 0/4] Enhance CPU flag support Bruce Richardson
2019-05-29 15:41 ` [dpdk-dev] [PATCH 1/4] build: fix quoting on RTE_ARCH string value Bruce Richardson
2019-05-29 15:53   ` Luca Boccassi
2019-05-29 15:41 ` [dpdk-dev] [PATCH 2/4] config/arm: fix missing define for arm platforms Bruce Richardson
2019-05-29 15:41 ` [dpdk-dev] [PATCH 3/4] eal: allow checking CPU flags by name Bruce Richardson
2019-06-27 13:22   ` David Marchand
2019-06-28 12:40     ` Bruce Richardson
2019-06-28 13:34       ` David Marchand
2019-05-29 15:41 ` [dpdk-dev] [PATCH 4/4] net: replace ifdefs with runtime branches Bruce Richardson
2019-07-01 19:30   ` Thomas Monjalon
2019-07-01 20:41     ` Bruce Richardson
2019-07-04 20:20       ` Thomas Monjalon
2019-07-08 17:24         ` David Christensen
2019-06-27 12:39 ` [dpdk-dev] [PATCH 0/4] Enhance CPU flag support Ananyev, Konstantin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).