diff mbox series

[4/4] net: replace ifdefs with runtime branches

Message ID 20190529154132.49955-5-bruce.richardson@intel.com (mailing list archive)
State Rejected, archived
Delegated to: Thomas Monjalon
Headers
Series Enhance CPU flag support |

Checks

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

Commit Message

Bruce Richardson May 29, 2019, 3:41 p.m. UTC 
  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(-)

Comments

Thomas Monjalon July 1, 2019, 7:30 p.m. UTC | #1 
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_?
Bruce Richardson July 1, 2019, 8:41 p.m. UTC | #2 
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
Thomas Monjalon July 4, 2019, 8:20 p.m. UTC | #3 
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?
David Christensen July 8, 2019, 5:24 p.m. UTC | #4 
> 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
diff mbox series

Patch

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);
 }