Hi Konstantin, thanks for your review
> -----Original Message-----
> From: Ananyev, Konstantin <konstantin.anan...@intel.com>
> Sent: Wednesday, October 7, 2020 3:59 PM
<snip>
>
> >
> > This patch adds support for run-time selection of the optimal
> > architecture-specific CRC path, based on the supported instruction
> > set(s) of the CPU.
> >
> > The compiler option checks have been moved from the C files to the
> > meson script. The rte_cpu_get_flag_enabled function is called
> > automatically by the library at process initialization time to
> > determine which instructions the CPU supports, with the most optimal
> > supported CRC path ultimately selected.
> >
> > Signed-off-by: Mairtin o Loingsigh <mairtin.oloings...@intel.com>
> > Signed-off-by: David Coyle <david.co...@intel.com>
>
> LGTM, just one nit see below.
> With that:
> Series acked-by: Konstantin Ananyev <konstantin.anan...@intel.com>
>
> > ---
> > doc/guides/rel_notes/release_20_11.rst | 4 ++
> > lib/librte_net/meson.build | 34 +++++++++++-
> > lib/librte_net/net_crc.h | 34 ++++++++++++
> > lib/librte_net/{net_crc_neon.h => net_crc_neon.c} | 26 +++------
> > lib/librte_net/{net_crc_sse.h => net_crc_sse.c} | 34 ++++--------
> > lib/librte_net/rte_net_crc.c | 67
> > ++++++++++++++---------
> > 6 files changed, 131 insertions(+), 68 deletions(-) create mode
> > 100644 lib/librte_net/net_crc.h rename lib/librte_net/{net_crc_neon.h
> > => net_crc_neon.c} (95%) rename lib/librte_net/{net_crc_sse.h =>
> > net_crc_sse.c} (94%)
> >
> >
<snip>
> > +#ifdef CC_X86_64_SSE42_PCLMULQDQ_SUPPORT static uint8_t
> > +sse42_pclmulqdq_cpu_supported(void)
> > +{
> > + return rte_cpu_get_flag_enabled(RTE_CPUFLAG_PCLMULQDQ);
> > +}
>
> As a nit, I think it would be better to hide #fidef inside the function, and
> return an 0 when define is not set.
> Something like:
>
> static int
> sse42_pclmulqdq_cpu_supported(void)
> {
> #ifdef CC_X86_64_SSE42_PCLMULQDQ_SUPPORT
> return rte_cpu_get_flag_enabled(RTE_CPUFLAG_PCLMULQDQ);
> #else
> return 0;
> }
>
> Same for other cpu_supported functions.
> And then you can remove these ifdefs in set_alg and other palces, i.e.:
>
> void
> rte_net_crc_set_alg(enum rte_net_crc_alg alg) {
> switch (alg) {
> #ifdef RTE_ARCH_X86_64
> case RTE_NET_CRC_AVX512:
> if (avx512_vpclmulqdq_cpu_supported()) {
> handlers = handlers_avx512;
> break;
> }
> /* fall-through */
> case RTE_NET_CRC_SSE42:
> if (sse42_pclmulqdq_cpu_supported()) {
> handlers = handlers_sse42;
> break;
> }
> #endif
> ...
>
> Same for rte_net_crc_init()
[DC] I have reworked the ifdefs in this file based on your comments here and
off-list discussions.
These are available now in the v5.
All ifdef's have been removed out the API function definitions and moved down
into 'helper' type
functions - looks much cleaner now.
Your Ack has been carried through too to v5 as you mentioned
>
> > +#endif
> > +
> > +#ifdef CC_ARM64_NEON_PMULL_SUPPORT
> > +static uint8_t
> > +neon_pmull_cpu_supported(void)
> > +{
> > + return rte_cpu_get_flag_enabled(RTE_CPUFLAG_PMULL);
> > +}
> > +#endif
> > +
> > void
> > 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;
> > -#elif defined ARM64_NEON_PMULL
> > - /* fall-through */
> > +#ifdef CC_X86_64_SSE42_PCLMULQDQ_SUPPORT
> > + if (sse42_pclmulqdq_cpu_supported()) {
> > + handlers = handlers_sse42;
> > + break;
> > + }
> > +#endif
> > +#endif /* RTE_ARCH_X86_64 */
> > +#ifdef RTE_ARCH_ARM64
> > case RTE_NET_CRC_NEON:
> > - if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_PMULL)) {
> > +#ifdef CC_ARM64_NEON_PMULL_SUPPORT
> > + if (neon_pmull_cpu_supported()) {
> > handlers = handlers_neon;
> > break;
> > }
> > #endif
> > +#endif /* RTE_ARCH_ARM64 */
> > /* fall-through */
> > case RTE_NET_CRC_SCALAR:
> > /* fall-through */
> > @@ -188,11 +200,14 @@ 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)) {
> > +#ifdef CC_X86_64_SSE42_PCLMULQDQ_SUPPORT
> > + if (sse42_pclmulqdq_cpu_supported()) {
> > + alg = RTE_NET_CRC_SSE42;
> > + rte_net_crc_sse42_init();
> > + }
> > +#endif
> > +#ifdef CC_ARM64_NEON_PMULL_SUPPORT
> > + if (neon_pmull_cpu_supported()) {
> > alg = RTE_NET_CRC_NEON;
> > rte_net_crc_neon_init();
> > }
> > --
> > 2.12.3
