[dpdk-dev] [PATCH v3] efd: support lookup using neon intrinsics

De Lara Guarch, Pablo pablo.de.lara.guarch at intel.com
Thu Jun 29 22:58:25 CEST 2017


Hi,

> -----Original Message-----
> From: Sekhar, Ashwin [mailto:Ashwin.Sekhar at cavium.com]
> Sent: Tuesday, May 2, 2017 9:05 AM
> To: Jacob, Jerin <Jerin.JacobKollanukkaran at cavium.com>;
> jianbo.liu at linaro.org
> Cc: Marohn, Byron <byron.marohn at intel.com>; De Lara Guarch, Pablo
> <pablo.de.lara.guarch at intel.com>; dev at 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.jacob at caviumnetworks.com>
> > wrote:
> > >
> > > -----Original Message-----
> > > >
> > > > Date: Mon,  1 May 2017 22:59:53 -0700
> > > > From: Ashwin Sekhar T K <ashwin.sekhar at caviumnetworks.com>
> > > > To: byron.marohn at intel.com, pablo.de.lara.guarch at intel.com,
> > > >  jerin.jacob at caviumnetworks.com, jianbo.liu at linaro.org
> > > > Cc: dev at dpdk.org, Ashwin Sekhar T K
> <ashwin.sekhar at 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.sekhar at 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
> > > >  };


More information about the dev mailing list