[4/4] net: replace ifdefs with runtime branches
Checks
Commit Message
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
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_?
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
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?
> 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
@@ -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);
}