Hi, > -----Original Message----- > From: Sekhar, Ashwin [mailto:ashwin.sek...@cavium.com] > Sent: Tuesday, May 2, 2017 9:05 AM > To: Jacob, Jerin <jerin.jacobkollanukka...@cavium.com>; > jianbo....@linaro.org > Cc: Marohn, Byron <byron.mar...@intel.com>; De Lara Guarch, Pablo > <pablo.de.lara.gua...@intel.com>; dev@dpdk.org > Subject: Re: [dpdk-dev] [PATCH v3] efd: support lookup using neon > intrinsics > > On Tue, 2017-05-02 at 15:59 +0800, Jianbo Liu wrote: > > On 2 May 2017 at 14:41, Jerin Jacob <jerin.ja...@caviumnetworks.com> > > wrote: > > > > > > -----Original Message----- > > > > > > > > Date: Mon, 1 May 2017 22:59:53 -0700 > > > > From: Ashwin Sekhar T K <ashwin.sek...@caviumnetworks.com> > > > > To: byron.mar...@intel.com, pablo.de.lara.gua...@intel.com, > > > > jerin.ja...@caviumnetworks.com, jianbo....@linaro.org > > > > Cc: dev@dpdk.org, Ashwin Sekhar T K > <ashwin.sekhar@caviumnetworks > > > > .com> > > > > Subject: [dpdk-dev] [PATCH v3] efd: support lookup using neon > > > > intrinsics > > > > X-Mailer: git-send-email 2.13.0.rc1 > > > > > > > > * Added file lib/librte_efd/rte_efd_arm64.h to hold arm64 > > > > specific definitions > > > > * Verified the changes with efd_autotest unit test case > > > > > > > > Signed-off-by: Ashwin Sekhar T K <ashwin.sek...@caviumnetworks.co > > > > m> > > > > --- > > > > v2: > > > > * Slightly modified the content of the commit message body > > > > * Added prefix [dpdk-dev] to the email subject line > > > > > > > > v3: > > > > * Moved enum 'EFD_LOOKUP_NEON' under '#if > defined(RTE_ARCH_ARM64)' > > > > > > > > MAINTAINERS | 1 + > > > > lib/librte_efd/rte_efd.c | 24 +++++++++++++ > > > > lib/librte_efd/rte_efd_arm64.h | 76 > > > > ++++++++++++++++++++++++++++++++++++++++++ > > > > 3 files changed, 101 insertions(+) > > > > create mode 100644 lib/librte_efd/rte_efd_arm64.h > > > > > > > > diff --git a/MAINTAINERS b/MAINTAINERS index b6495d2..7d708ae > > > > 100644 > > > > --- a/MAINTAINERS > > > > +++ b/MAINTAINERS > > > > @@ -147,6 +147,7 @@ F: > > > > lib/librte_eal/common/include/arch/arm/*_64.h > > > > F: lib/librte_acl/acl_run_neon.* > > > > F: lib/librte_lpm/rte_lpm_neon.h > > > > F: lib/librte_hash/rte*_arm64.h > > > > +F: lib/librte_efd/rte*_arm64.h > > > > F: drivers/net/ixgbe/ixgbe_rxtx_vec_neon.c > > > > F: drivers/net/i40e/i40e_rxtx_vec_neon.c > > > > F: drivers/net/virtio/virtio_rxtx_simple_neon.c > > > > diff --git a/lib/librte_efd/rte_efd.c b/lib/librte_efd/rte_efd.c > > > > index f601d62..5cc6283 100644 > > > > --- a/lib/librte_efd/rte_efd.c > > > > +++ b/lib/librte_efd/rte_efd.c > > > > @@ -53,6 +53,8 @@ > > > > #include "rte_efd.h" > > > > #if defined(RTE_ARCH_X86) > > > > #include "rte_efd_x86.h" > > > > +#elif defined(RTE_ARCH_ARM64) > > > > +#include "rte_efd_arm64.h" > > > > #endif > > > > > > > > #define EFD_KEY(key_idx, table) (table->keys + ((key_idx) * > > > > table->key_len)) > > > > @@ -103,6 +105,9 @@ allocated memory > > > > enum efd_lookup_internal_function { > > > > EFD_LOOKUP_SCALAR = 0, > > > > EFD_LOOKUP_AVX2, > > > > +#if defined(RTE_ARCH_ARM64) > > > > + EFD_LOOKUP_NEON, > > > > +#endif > > > I think, we can remove this ifdef to > > > - Make code looks clean > > > - In future, in some case a new enum value gets added then the value > > > will be different for each build. > > > > > But the enum items are same for each ARCH. > > Besides, the ifdef could be considered as explanation to that enum. > > If > > someone knows nothing about arm/neon, he can ignore it totally after > > see the ifdef. > > > Have added the #if defined on your advice, but in my opinion also its better > not to have "#if defined" for enums. Because the same enum can take > different values for different builds. > > For eg: If somebody adds an EFD_LOOKUP_AVX512 > after EFD_LOOKUP_NEON here, it will take value 2 for x86 builds but value > 3 for arm64 builds.
Sorry for not having replied earlier on this. I don't see the reason why the new enumeration value needs to be within an ifdef. If the values mattered, then the EFD_LOOKUP_AVX2 should be also within an ifdef for X86, and as far as I know, it is not necessary. The rest of the the patch looks good to me, I would just remove that ifdef. Pablo > > > > > > Any valid point to keep under RTE_ARCH_ARM64? > > > > > > > > > > > EFD_LOOKUP_NUM > > > > };
