diff mbox

[dpdk-dev,3/3] net: run-time function selection

Message ID 1509991543-26521-1-git-send-email-elza.mathew@intel.com (mailing list archive)
State Changes Requested, archived
Delegated to: Thomas Monjalon
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation fail Compilation issues

Commit Message

Elza Mathew Nov. 6, 2017, 6:05 p.m. UTC 
  Compile-time function selection can potentially lead to
lower performance on generic builds done by distros.
Replaced compile time flag checks with run-time
function selection.

Signed-off-by: Elza Mathew <elza.mathew@intel.com>
---
 lib/librte_net/rte_net_crc.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

Comments

Bruce Richardson Dec. 11, 2017, 3:31 p.m. UTC | #1 
On Mon, Nov 06, 2017 at 10:05:43AM -0800, Elza Mathew wrote:
> Compile-time function selection can potentially lead to
> lower performance on generic builds done by distros.
> Replaced compile time flag checks with run-time
> function selection.
> 
> Signed-off-by: Elza Mathew <elza.mathew@intel.com>
> ---
>  lib/librte_net/rte_net_crc.c | 22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
Patch looks good to me, but unfortunately the compilation testing shows
an issue with clang compliation on FreeBSD 10. I've also tested with gcc
on both BSD 10 and 11 and saw no issues, and BSD 11 clang compilation is
also fine. 

The compilation error is due to missing _mm_clmulepi64_si128 instrinsic:
In file included from /home/patchWorkOrg/compilation/lib/librte_net/rte_net_crc.c:43:
/home/patchWorkOrg/compilation/lib/librte_net/net_crc_sse.h:81:17: error: implicit declaration of function '_mm_clmulepi64_si128' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
        __m128i tmp0 = _mm_clmulepi64_si128(fold, precomp, 0x01);

/Bruce
Thomas Monjalon Jan. 18, 2018, 11:43 p.m. UTC | #2 
11/12/2017 16:31, Bruce Richardson:
> On Mon, Nov 06, 2017 at 10:05:43AM -0800, Elza Mathew wrote:
> > Compile-time function selection can potentially lead to
> > lower performance on generic builds done by distros.
> > Replaced compile time flag checks with run-time
> > function selection.
> > 
> > Signed-off-by: Elza Mathew <elza.mathew@intel.com>
> > ---
> >  lib/librte_net/rte_net_crc.c | 22 +++++++++++++---------
> >  1 file changed, 13 insertions(+), 9 deletions(-)
> > 
> Patch looks good to me, but unfortunately the compilation testing shows
> an issue with clang compliation on FreeBSD 10. I've also tested with gcc
> on both BSD 10 and 11 and saw no issues, and BSD 11 clang compilation is
> also fine. 
> 
> The compilation error is due to missing _mm_clmulepi64_si128 instrinsic:
> In file included from /home/patchWorkOrg/compilation/lib/librte_net/rte_net_crc.c:43:
> /home/patchWorkOrg/compilation/lib/librte_net/net_crc_sse.h:81:17: error: implicit declaration of function '_mm_clmulepi64_si128' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
>         __m128i tmp0 = _mm_clmulepi64_si128(fold, precomp, 0x01);

Any update please?
Bruce Richardson Jan. 19, 2018, 9:20 a.m. UTC | #3 
On Fri, Jan 19, 2018 at 12:43:09AM +0100, Thomas Monjalon wrote:
> 11/12/2017 16:31, Bruce Richardson:
> > On Mon, Nov 06, 2017 at 10:05:43AM -0800, Elza Mathew wrote:
> > > Compile-time function selection can potentially lead to
> > > lower performance on generic builds done by distros.
> > > Replaced compile time flag checks with run-time
> > > function selection.
> > > 
> > > Signed-off-by: Elza Mathew <elza.mathew@intel.com>
> > > ---
> > >  lib/librte_net/rte_net_crc.c | 22 +++++++++++++---------
> > >  1 file changed, 13 insertions(+), 9 deletions(-)
> > > 
> > Patch looks good to me, but unfortunately the compilation testing shows
> > an issue with clang compliation on FreeBSD 10. I've also tested with gcc
> > on both BSD 10 and 11 and saw no issues, and BSD 11 clang compilation is
> > also fine. 
> > 
> > The compilation error is due to missing _mm_clmulepi64_si128 instrinsic:
> > In file included from /home/patchWorkOrg/compilation/lib/librte_net/rte_net_crc.c:43:
> > /home/patchWorkOrg/compilation/lib/librte_net/net_crc_sse.h:81:17: error: implicit declaration of function '_mm_clmulepi64_si128' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
> >         __m128i tmp0 = _mm_clmulepi64_si128(fold, precomp, 0x01);
> 
> Any update please?

I'll work with Elza to see if I can find a workaround for this compiler
issue. In the meantime, perhaps patches 1 and 2 of the set can be
applied so that we can do a new patch 3 only when we get it fixed.

/Bruce
Bruce Richardson Jan. 22, 2018, 5:36 p.m. UTC | #4 
On Mon, Dec 11, 2017 at 03:31:16PM +0000, Bruce Richardson wrote:
> On Mon, Nov 06, 2017 at 10:05:43AM -0800, Elza Mathew wrote:
> > Compile-time function selection can potentially lead to
> > lower performance on generic builds done by distros.
> > Replaced compile time flag checks with run-time
> > function selection.
> > 
> > Signed-off-by: Elza Mathew <elza.mathew@intel.com>
> > ---
> >  lib/librte_net/rte_net_crc.c | 22 +++++++++++++---------
> >  1 file changed, 13 insertions(+), 9 deletions(-)
> > 
> Patch looks good to me, but unfortunately the compilation testing shows
> an issue with clang compliation on FreeBSD 10. I've also tested with gcc
> on both BSD 10 and 11 and saw no issues, and BSD 11 clang compilation is
> also fine. 
> 
> The compilation error is due to missing _mm_clmulepi64_si128 instrinsic:
> In file included from /home/patchWorkOrg/compilation/lib/librte_net/rte_net_crc.c:43:
> /home/patchWorkOrg/compilation/lib/librte_net/net_crc_sse.h:81:17: error: implicit declaration of function '_mm_clmulepi64_si128' is invalid in C99 [-Werror,-Wimplicit-function-declaration]
>         __m128i tmp0 = _mm_clmulepi64_si128(fold, precomp, 0x01);
> 
> /Bruce

Checking with latest images for 10.3 or 10.4 no longer shows this
problem. I think the patch can be merged as-is.

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Thomas, do you need a resubmit to appear as a new patch in patchwork?
diff mbox

Patch

diff --git a/lib/librte_net/rte_net_crc.c b/lib/librte_net/rte_net_crc.c
index 661fe32..8f6a0e7 100644
--- a/lib/librte_net/rte_net_crc.c
+++ b/lib/librte_net/rte_net_crc.c
@@ -39,8 +39,8 @@ 
 #include <rte_common.h>
 #include <rte_net_crc.h>
 
-#if defined(RTE_ARCH_X86_64) && defined(RTE_MACHINE_CPUFLAG_PCLMULQDQ)
-#define X86_64_SSE42_PCLMULQDQ     1
+#ifdef RTE_ARCH_X86_64
+#include <net_crc_sse.h>
 #elif defined(RTE_ARCH_ARM64) && defined(RTE_MACHINE_CPUFLAG_PMULL)
 #define ARM64_NEON_PMULL           1
 #endif
@@ -71,7 +71,7 @@ 
 	[RTE_NET_CRC32_ETH] = rte_crc32_eth_handler,
 };
 
-#ifdef X86_64_SSE42_PCLMULQDQ
+#ifdef RTE_ARCH_X86_64
 static rte_net_crc_handler handlers_sse42[] = {
 	[RTE_NET_CRC16_CCITT] = rte_crc16_ccitt_sse42_handler,
 	[RTE_NET_CRC32_ETH] = rte_crc32_eth_sse42_handler,
@@ -169,10 +169,12 @@ 
 rte_net_crc_set_alg(enum rte_net_crc_alg alg)
 {
 	switch (alg) {
-#ifdef X86_64_SSE42_PCLMULQDQ
+#ifdef RTE_ARCH_X86_64
 	case RTE_NET_CRC_SSE42:
-		handlers = handlers_sse42;
-		break;
+		if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_PCLMULQDQ)) {
+			handlers = handlers_sse42;
+			break;
+		}
 #elif defined ARM64_NEON_PMULL
 		/* fall-through */
 	case RTE_NET_CRC_NEON:
@@ -212,9 +214,11 @@  static inline void __attribute__((constructor))
 
 	rte_net_crc_scalar_init();
 
-#ifdef X86_64_SSE42_PCLMULQDQ
-	alg = RTE_NET_CRC_SSE42;
-	rte_net_crc_sse42_init();
+#ifdef RTE_ARCH_X86_64
+	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_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)) {
 		alg = RTE_NET_CRC_NEON;