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

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

[Ashwin Sekhar T K]

* Added file lib/librte_efd/rte_efd_arm64.h to hold arm64
  specific definitions
* Tested using efd_autotest

Signed-off-by: Ashwin Sekhar T K <ashwin.sekhar@caviumnetworks.com>
---
 MAINTAINERS                    |  1 +
 lib/librte_efd/rte_efd.c       | 22 ++++++++++++
 lib/librte_efd/rte_efd_arm64.h | 76 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 99 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..4d9a088 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,7 @@ allocated memory
 enum efd_lookup_internal_function {
 	EFD_LOOKUP_SCALAR = 0,
 	EFD_LOOKUP_AVX2,
+	EFD_LOOKUP_NEON,
 	EFD_LOOKUP_NUM
 };
 
@@ -674,6 +677,16 @@ rte_efd_create(const char *name, uint32_t max_num_rules, uint32_t key_len,
 		table->lookup_fn = EFD_LOOKUP_AVX2;
 	else
 #endif
+#if defined(RTE_ARCH_ARM64)
+	/*
+	 * For less than or equal to 16 bits, scalar function performs better
+	 * than vectorised version
+	 */
+	if (RTE_EFD_VALUE_NUM_BITS > 16 &&
+	    rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
+		table->lookup_fn = EFD_LOOKUP_NEON;
+	else
+#endif
 		table->lookup_fn = EFD_LOOKUP_SCALAR;
 
 	/*
@@ -1271,6 +1284,15 @@ efd_lookup_internal(const struct efd_online_group_entry * const group,
 					group->lookup_table,
 					hash_val_a,
 					hash_val_b);
+		break;
+#endif
+#if defined(RTE_ARCH_ARM64)
+	case EFD_LOOKUP_NEON:
+		return efd_lookup_internal_neon(group->hash_idx,
+					group->lookup_table,
+					hash_val_a,
+					hash_val_b);
+		break;
 #endif
 	case EFD_LOOKUP_SCALAR:
 	/* Fall-through */
diff --git a/lib/librte_efd/rte_efd_arm64.h b/lib/librte_efd/rte_efd_arm64.h
new file mode 100644
index 0000000..cc93411
--- /dev/null
+++ b/lib/librte_efd/rte_efd_arm64.h
@@ -0,0 +1,76 @@
+/*
+ *   BSD LICENSE
+ *
+ *   Copyright (C) Cavium networks Ltd. 2017.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Cavium networks nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+/*
+ * rte_efd_arm64.h
+ * This file holds all arm64 specific EFD functions
+ */
+
+#ifndef __RTE_EFD_ARM64_H__
+#define __RTE_EFD_ARM64_H__
+
+#include <rte_vect.h>
+
+static inline efd_value_t
+efd_lookup_internal_neon(const efd_hashfunc_t *group_hash_idx,
+		const efd_lookuptbl_t *group_lookup_table,
+		const uint32_t hash_val_a, const uint32_t hash_val_b)
+{
+	efd_value_t value = 0;
+	uint32_t i = 0;
+	uint32x4_t vhash_val_a = vmovq_n_u32(hash_val_a);
+	uint32x4_t vhash_val_b = vmovq_n_u32(hash_val_b);
+	int32x4_t vshift = {0, 1, 2, 3};
+	uint32x4_t vmask = vdupq_n_u32(0x1);
+	int32x4_t vincr = vdupq_n_s32(4);
+
+	for (; i < RTE_EFD_VALUE_NUM_BITS; i += 4) {
+		uint32x4_t vhash_idx = vshll_n_u16(
+			vld1_u16((uint16_t const *)&group_hash_idx[i]), 0);
+		uint32x4_t vlookup_table = vshll_n_u16(
+			vld1_u16((uint16_t const *)&group_lookup_table[i]), 0);
+		uint32x4_t vhash = vaddq_u32(vhash_val_a,
+					vmulq_u32(vhash_idx, vhash_val_b));
+		int32x4_t vbucket_idx = vnegq_s32(vreinterpretq_s32_u32(
+				vshrq_n_u32(vhash, EFD_LOOKUPTBL_SHIFT)));
+		uint32x4_t vresult = vshlq_u32(vlookup_table, vbucket_idx);
+
+		vresult = vandq_u32(vresult, vmask);
+		vresult = vshlq_u32(vresult, vshift);
+		value |= vaddvq_u32(vresult);
+		vshift = vaddq_s32(vshift, vincr);
+	}
+
+	return value;
+}
+
+#endif /* __RTE_EFD_ARM64_H__ */
-- 
2.7.4

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

[Ashwin Sekhar T K]

* 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@caviumnetworks.com>
---
v2:
* Slightly modified the content of the commit message body
* Added prefix [dpdk-dev] to the email subject line

 MAINTAINERS                    |  1 +
 lib/librte_efd/rte_efd.c       | 22 ++++++++++++
 lib/librte_efd/rte_efd_arm64.h | 76 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 99 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..4d9a088 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,7 @@ allocated memory
 enum efd_lookup_internal_function {
 	EFD_LOOKUP_SCALAR = 0,
 	EFD_LOOKUP_AVX2,
+	EFD_LOOKUP_NEON,
 	EFD_LOOKUP_NUM
 };
 
@@ -674,6 +677,16 @@ rte_efd_create(const char *name, uint32_t max_num_rules, uint32_t key_len,
 		table->lookup_fn = EFD_LOOKUP_AVX2;
 	else
 #endif
+#if defined(RTE_ARCH_ARM64)
+	/*
+	 * For less than or equal to 16 bits, scalar function performs better
+	 * than vectorised version
+	 */
+	if (RTE_EFD_VALUE_NUM_BITS > 16 &&
+	    rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
+		table->lookup_fn = EFD_LOOKUP_NEON;
+	else
+#endif
 		table->lookup_fn = EFD_LOOKUP_SCALAR;
 
 	/*
@@ -1271,6 +1284,15 @@ efd_lookup_internal(const struct efd_online_group_entry * const group,
 					group->lookup_table,
 					hash_val_a,
 					hash_val_b);
+		break;
+#endif
+#if defined(RTE_ARCH_ARM64)
+	case EFD_LOOKUP_NEON:
+		return efd_lookup_internal_neon(group->hash_idx,
+					group->lookup_table,
+					hash_val_a,
+					hash_val_b);
+		break;
 #endif
 	case EFD_LOOKUP_SCALAR:
 	/* Fall-through */
diff --git a/lib/librte_efd/rte_efd_arm64.h b/lib/librte_efd/rte_efd_arm64.h
new file mode 100644
index 0000000..cc93411
--- /dev/null
+++ b/lib/librte_efd/rte_efd_arm64.h
@@ -0,0 +1,76 @@
+/*
+ *   BSD LICENSE
+ *
+ *   Copyright (C) Cavium networks Ltd. 2017.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Cavium networks nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+/*
+ * rte_efd_arm64.h
+ * This file holds all arm64 specific EFD functions
+ */
+
+#ifndef __RTE_EFD_ARM64_H__
+#define __RTE_EFD_ARM64_H__
+
+#include <rte_vect.h>
+
+static inline efd_value_t
+efd_lookup_internal_neon(const efd_hashfunc_t *group_hash_idx,
+		const efd_lookuptbl_t *group_lookup_table,
+		const uint32_t hash_val_a, const uint32_t hash_val_b)
+{
+	efd_value_t value = 0;
+	uint32_t i = 0;
+	uint32x4_t vhash_val_a = vmovq_n_u32(hash_val_a);
+	uint32x4_t vhash_val_b = vmovq_n_u32(hash_val_b);
+	int32x4_t vshift = {0, 1, 2, 3};
+	uint32x4_t vmask = vdupq_n_u32(0x1);
+	int32x4_t vincr = vdupq_n_s32(4);
+
+	for (; i < RTE_EFD_VALUE_NUM_BITS; i += 4) {
+		uint32x4_t vhash_idx = vshll_n_u16(
+			vld1_u16((uint16_t const *)&group_hash_idx[i]), 0);
+		uint32x4_t vlookup_table = vshll_n_u16(
+			vld1_u16((uint16_t const *)&group_lookup_table[i]), 0);
+		uint32x4_t vhash = vaddq_u32(vhash_val_a,
+					vmulq_u32(vhash_idx, vhash_val_b));
+		int32x4_t vbucket_idx = vnegq_s32(vreinterpretq_s32_u32(
+				vshrq_n_u32(vhash, EFD_LOOKUPTBL_SHIFT)));
+		uint32x4_t vresult = vshlq_u32(vlookup_table, vbucket_idx);
+
+		vresult = vandq_u32(vresult, vmask);
+		vresult = vshlq_u32(vresult, vshift);
+		value |= vaddvq_u32(vresult);
+		vshift = vaddq_s32(vshift, vincr);
+	}
+
+	return value;
+}
+
+#endif /* __RTE_EFD_ARM64_H__ */
-- 
2.7.4

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

[Jianbo Liu]

On 27 April 2017 at 20:44, Ashwin Sekhar T K
<ashwin.sekhar@caviumnetworks.com> wrote:
> * 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@caviumnetworks.com>
> ---
> v2:
> * Slightly modified the content of the commit message body
> * Added prefix [dpdk-dev] to the email subject line
>
>  MAINTAINERS                    |  1 +
>  lib/librte_efd/rte_efd.c       | 22 ++++++++++++
>  lib/librte_efd/rte_efd_arm64.h | 76 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 99 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..4d9a088 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,7 @@ allocated memory
>  enum efd_lookup_internal_function {
>         EFD_LOOKUP_SCALAR = 0,
>         EFD_LOOKUP_AVX2,
> +       EFD_LOOKUP_NEON,

Should it be included in "if defined(RTE_ARCH_ARM64)"?

>         EFD_LOOKUP_NUM
>  };
>
> @@ -674,6 +677,16 @@ rte_efd_create(const char *name, uint32_t max_num_rules, uint32_t key_len,
>                 table->lookup_fn = EFD_LOOKUP_AVX2;
>         else
>  #endif
> +#if defined(RTE_ARCH_ARM64)
> +       /*
> +        * For less than or equal to 16 bits, scalar function performs better
> +        * than vectorised version
> +        */
> +       if (RTE_EFD_VALUE_NUM_BITS > 16 &&
> +           rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
> +               table->lookup_fn = EFD_LOOKUP_NEON;
> +       else
> +#endif
>                 table->lookup_fn = EFD_LOOKUP_SCALAR;
>
>         /*
> @@ -1271,6 +1284,15 @@ efd_lookup_internal(const struct efd_online_group_entry * const group,
>                                         group->lookup_table,
>                                         hash_val_a,
>                                         hash_val_b);
> +               break;
> +#endif
> +#if defined(RTE_ARCH_ARM64)
> +       case EFD_LOOKUP_NEON:
> +               return efd_lookup_internal_neon(group->hash_idx,
> +                                       group->lookup_table,
> +                                       hash_val_a,
> +                                       hash_val_b);
> +               break;
>  #endif
>         case EFD_LOOKUP_SCALAR:
>         /* Fall-through */
> diff --git a/lib/librte_efd/rte_efd_arm64.h b/lib/librte_efd/rte_efd_arm64.h
> new file mode 100644
> index 0000000..cc93411
> --- /dev/null
> +++ b/lib/librte_efd/rte_efd_arm64.h
> @@ -0,0 +1,76 @@
> +/*
> + *   BSD LICENSE
> + *
> + *   Copyright (C) Cavium networks Ltd. 2017.
> + *
> + *   Redistribution and use in source and binary forms, with or without
> + *   modification, are permitted provided that the following conditions
> + *   are met:
> + *
> + *     * Redistributions of source code must retain the above copyright
> + *       notice, this list of conditions and the following disclaimer.
> + *     * Redistributions in binary form must reproduce the above copyright
> + *       notice, this list of conditions and the following disclaimer in
> + *       the documentation and/or other materials provided with the
> + *       distribution.
> + *     * Neither the name of Cavium networks nor the names of its
> + *       contributors may be used to endorse or promote products derived
> + *       from this software without specific prior written permission.
> + *
> + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
> + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
> + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
> + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
> + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
> + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
> + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
> + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
> + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
> + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
> + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
> + */
> +
> +/*
> + * rte_efd_arm64.h
> + * This file holds all arm64 specific EFD functions
> + */
> +
> +#ifndef __RTE_EFD_ARM64_H__
> +#define __RTE_EFD_ARM64_H__
> +
> +#include <rte_vect.h>
> +
> +static inline efd_value_t
> +efd_lookup_internal_neon(const efd_hashfunc_t *group_hash_idx,
> +               const efd_lookuptbl_t *group_lookup_table,
> +               const uint32_t hash_val_a, const uint32_t hash_val_b)
> +{
> +       efd_value_t value = 0;
> +       uint32_t i = 0;
> +       uint32x4_t vhash_val_a = vmovq_n_u32(hash_val_a);
> +       uint32x4_t vhash_val_b = vmovq_n_u32(hash_val_b);
> +       int32x4_t vshift = {0, 1, 2, 3};
> +       uint32x4_t vmask = vdupq_n_u32(0x1);
> +       int32x4_t vincr = vdupq_n_s32(4);
> +
> +       for (; i < RTE_EFD_VALUE_NUM_BITS; i += 4) {
> +               uint32x4_t vhash_idx = vshll_n_u16(
> +                       vld1_u16((uint16_t const *)&group_hash_idx[i]), 0);
> +               uint32x4_t vlookup_table = vshll_n_u16(
> +                       vld1_u16((uint16_t const *)&group_lookup_table[i]), 0);
> +               uint32x4_t vhash = vaddq_u32(vhash_val_a,
> +                                       vmulq_u32(vhash_idx, vhash_val_b));
> +               int32x4_t vbucket_idx = vnegq_s32(vreinterpretq_s32_u32(
> +                               vshrq_n_u32(vhash, EFD_LOOKUPTBL_SHIFT)));
> +               uint32x4_t vresult = vshlq_u32(vlookup_table, vbucket_idx);
> +
> +               vresult = vandq_u32(vresult, vmask);
> +               vresult = vshlq_u32(vresult, vshift);
> +               value |= vaddvq_u32(vresult);
> +               vshift = vaddq_s32(vshift, vincr);
> +       }
> +
> +       return value;
> +}
> +
> +#endif /* __RTE_EFD_ARM64_H__ */
> --
> 2.7.4
>

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

[Sekhar, Ashwin]

On Friday 28 April 2017 03:36 PM, Jianbo Liu wrote:
> On 27 April 2017 at 20:44, Ashwin Sekhar T K
> <ashwin.sekhar@caviumnetworks.com> wrote:
>> * 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@caviumnetworks.com>
>> ---
>> v2:
>> * Slightly modified the content of the commit message body
>> * Added prefix [dpdk-dev] to the email subject line
>>
>>  MAINTAINERS                    |  1 +
>>  lib/librte_efd/rte_efd.c       | 22 ++++++++++++
>>  lib/librte_efd/rte_efd_arm64.h | 76 ++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 99 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..4d9a088 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,7 @@ allocated memory
>>  enum efd_lookup_internal_function {
>>         EFD_LOOKUP_SCALAR = 0,
>>         EFD_LOOKUP_AVX2,
>> +       EFD_LOOKUP_NEON,
>
> Should it be included in "if defined(RTE_ARCH_ARM64)"?
>
The enum can be wrapped under "if defined(RTE_ARCH_ARM64)" with no 
issues, as all its usages are also under "if defined(RTE_ARCH_ARM64)".
I followed EFD_LOOKUP_AVX2 and defined EFD_LOOKUP_NEON on the same lines.
Please advise on whether this change is to be made. Will follow your advice.
>>         EFD_LOOKUP_NUM
>>  };
>>
>> @@ -674,6 +677,16 @@ rte_efd_create(const char *name, uint32_t max_num_rules, uint32_t key_len,
>>                 table->lookup_fn = EFD_LOOKUP_AVX2;
>>         else
>>  #endif
>> +#if defined(RTE_ARCH_ARM64)
>> +       /*
>> +        * For less than or equal to 16 bits, scalar function performs better
>> +        * than vectorised version
>> +        */
>> +       if (RTE_EFD_VALUE_NUM_BITS > 16 &&
>> +           rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
>> +               table->lookup_fn = EFD_LOOKUP_NEON;
>> +       else
>> +#endif
>>                 table->lookup_fn = EFD_LOOKUP_SCALAR;
>>
>>         /*
>> @@ -1271,6 +1284,15 @@ efd_lookup_internal(const struct efd_online_group_entry * const group,
>>                                         group->lookup_table,
>>                                         hash_val_a,
>>                                         hash_val_b);
>> +               break;
>> +#endif
>> +#if defined(RTE_ARCH_ARM64)
>> +       case EFD_LOOKUP_NEON:
>> +               return efd_lookup_internal_neon(group->hash_idx,
>> +                                       group->lookup_table,
>> +                                       hash_val_a,
>> +                                       hash_val_b);
>> +               break;
>>  #endif
>>         case EFD_LOOKUP_SCALAR:
>>         /* Fall-through */
>> diff --git a/lib/librte_efd/rte_efd_arm64.h b/lib/librte_efd/rte_efd_arm64.h
>> new file mode 100644
>> index 0000000..cc93411
>> --- /dev/null
>> +++ b/lib/librte_efd/rte_efd_arm64.h
>> @@ -0,0 +1,76 @@
>> +/*
>> + *   BSD LICENSE
>> + *
>> + *   Copyright (C) Cavium networks Ltd. 2017.
>> + *
>> + *   Redistribution and use in source and binary forms, with or without
>> + *   modification, are permitted provided that the following conditions
>> + *   are met:
>> + *
>> + *     * Redistributions of source code must retain the above copyright
>> + *       notice, this list of conditions and the following disclaimer.
>> + *     * Redistributions in binary form must reproduce the above copyright
>> + *       notice, this list of conditions and the following disclaimer in
>> + *       the documentation and/or other materials provided with the
>> + *       distribution.
>> + *     * Neither the name of Cavium networks nor the names of its
>> + *       contributors may be used to endorse or promote products derived
>> + *       from this software without specific prior written permission.
>> + *
>> + *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
>> + *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
>> + *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
>> + *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
>> + *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
>> + *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
>> + *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
>> + *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
>> + *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>> + *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
>> + *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
>> + */
>> +
>> +/*
>> + * rte_efd_arm64.h
>> + * This file holds all arm64 specific EFD functions
>> + */
>> +
>> +#ifndef __RTE_EFD_ARM64_H__
>> +#define __RTE_EFD_ARM64_H__
>> +
>> +#include <rte_vect.h>
>> +
>> +static inline efd_value_t
>> +efd_lookup_internal_neon(const efd_hashfunc_t *group_hash_idx,
>> +               const efd_lookuptbl_t *group_lookup_table,
>> +               const uint32_t hash_val_a, const uint32_t hash_val_b)
>> +{
>> +       efd_value_t value = 0;
>> +       uint32_t i = 0;
>> +       uint32x4_t vhash_val_a = vmovq_n_u32(hash_val_a);
>> +       uint32x4_t vhash_val_b = vmovq_n_u32(hash_val_b);
>> +       int32x4_t vshift = {0, 1, 2, 3};
>> +       uint32x4_t vmask = vdupq_n_u32(0x1);
>> +       int32x4_t vincr = vdupq_n_s32(4);
>> +
>> +       for (; i < RTE_EFD_VALUE_NUM_BITS; i += 4) {
>> +               uint32x4_t vhash_idx = vshll_n_u16(
>> +                       vld1_u16((uint16_t const *)&group_hash_idx[i]), 0);
>> +               uint32x4_t vlookup_table = vshll_n_u16(
>> +                       vld1_u16((uint16_t const *)&group_lookup_table[i]), 0);
>> +               uint32x4_t vhash = vaddq_u32(vhash_val_a,
>> +                                       vmulq_u32(vhash_idx, vhash_val_b));
>> +               int32x4_t vbucket_idx = vnegq_s32(vreinterpretq_s32_u32(
>> +                               vshrq_n_u32(vhash, EFD_LOOKUPTBL_SHIFT)));
>> +               uint32x4_t vresult = vshlq_u32(vlookup_table, vbucket_idx);
>> +
>> +               vresult = vandq_u32(vresult, vmask);
>> +               vresult = vshlq_u32(vresult, vshift);
>> +               value |= vaddvq_u32(vresult);
>> +               vshift = vaddq_s32(vshift, vincr);
>> +       }
>> +
>> +       return value;
>> +}
>> +
>> +#endif /* __RTE_EFD_ARM64_H__ */
>> --
>> 2.7.4
>>
>

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

[Jianbo Liu]

On 28 April 2017 at 18:38, Sekhar, Ashwin <Ashwin.Sekhar@cavium.com> wrote:
> On Friday 28 April 2017 03:36 PM, Jianbo Liu wrote:
>> On 27 April 2017 at 20:44, Ashwin Sekhar T K
>> <ashwin.sekhar@caviumnetworks.com> wrote:
>>> * 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@caviumnetworks.com>
>>> ---
>>> v2:
>>> * Slightly modified the content of the commit message body
>>> * Added prefix [dpdk-dev] to the email subject line
>>>
>>>  MAINTAINERS                    |  1 +
>>>  lib/librte_efd/rte_efd.c       | 22 ++++++++++++
>>>  lib/librte_efd/rte_efd_arm64.h | 76 ++++++++++++++++++++++++++++++++++++++++++
>>>  3 files changed, 99 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..4d9a088 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,7 @@ allocated memory
>>>  enum efd_lookup_internal_function {
>>>         EFD_LOOKUP_SCALAR = 0,
>>>         EFD_LOOKUP_AVX2,
>>> +       EFD_LOOKUP_NEON,
>>
>> Should it be included in "if defined(RTE_ARCH_ARM64)"?
>>
> The enum can be wrapped under "if defined(RTE_ARCH_ARM64)" with no
> issues, as all its usages are also under "if defined(RTE_ARCH_ARM64)".
> I followed EFD_LOOKUP_AVX2 and defined EFD_LOOKUP_NEON on the same lines.
> Please advise on whether this change is to be made. Will follow your advice.

Yes, please do that.

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

[Thomas Monjalon]

27/04/2017 14:44, Ashwin Sekhar T K:
> * 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@caviumnetworks.com>
> ---
> v2:
> * Slightly modified the content of the commit message body
> * Added prefix [dpdk-dev] to the email subject line

v2 applied, thanks

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

[Ashwin Sekhar T K]

* 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@caviumnetworks.com>
---
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
 	EFD_LOOKUP_NUM
 };
 
@@ -674,6 +679,16 @@ rte_efd_create(const char *name, uint32_t max_num_rules, uint32_t key_len,
 		table->lookup_fn = EFD_LOOKUP_AVX2;
 	else
 #endif
+#if defined(RTE_ARCH_ARM64)
+	/*
+	 * For less than or equal to 16 bits, scalar function performs better
+	 * than vectorised version
+	 */
+	if (RTE_EFD_VALUE_NUM_BITS > 16 &&
+	    rte_cpu_get_flag_enabled(RTE_CPUFLAG_NEON))
+		table->lookup_fn = EFD_LOOKUP_NEON;
+	else
+#endif
 		table->lookup_fn = EFD_LOOKUP_SCALAR;
 
 	/*
@@ -1271,6 +1286,15 @@ efd_lookup_internal(const struct efd_online_group_entry * const group,
 					group->lookup_table,
 					hash_val_a,
 					hash_val_b);
+		break;
+#endif
+#if defined(RTE_ARCH_ARM64)
+	case EFD_LOOKUP_NEON:
+		return efd_lookup_internal_neon(group->hash_idx,
+					group->lookup_table,
+					hash_val_a,
+					hash_val_b);
+		break;
 #endif
 	case EFD_LOOKUP_SCALAR:
 	/* Fall-through */
diff --git a/lib/librte_efd/rte_efd_arm64.h b/lib/librte_efd/rte_efd_arm64.h
new file mode 100644
index 0000000..cc93411
--- /dev/null
+++ b/lib/librte_efd/rte_efd_arm64.h
@@ -0,0 +1,76 @@
+/*
+ *   BSD LICENSE
+ *
+ *   Copyright (C) Cavium networks Ltd. 2017.
+ *
+ *   Redistribution and use in source and binary forms, with or without
+ *   modification, are permitted provided that the following conditions
+ *   are met:
+ *
+ *     * Redistributions of source code must retain the above copyright
+ *       notice, this list of conditions and the following disclaimer.
+ *     * Redistributions in binary form must reproduce the above copyright
+ *       notice, this list of conditions and the following disclaimer in
+ *       the documentation and/or other materials provided with the
+ *       distribution.
+ *     * Neither the name of Cavium networks nor the names of its
+ *       contributors may be used to endorse or promote products derived
+ *       from this software without specific prior written permission.
+ *
+ *   THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS
+ *   "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT
+ *   LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR
+ *   A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT
+ *   OWNER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ *   SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ *   LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE,
+ *   DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY
+ *   THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ *   (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ *   OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+/*
+ * rte_efd_arm64.h
+ * This file holds all arm64 specific EFD functions
+ */
+
+#ifndef __RTE_EFD_ARM64_H__
+#define __RTE_EFD_ARM64_H__
+
+#include <rte_vect.h>
+
+static inline efd_value_t
+efd_lookup_internal_neon(const efd_hashfunc_t *group_hash_idx,
+		const efd_lookuptbl_t *group_lookup_table,
+		const uint32_t hash_val_a, const uint32_t hash_val_b)
+{
+	efd_value_t value = 0;
+	uint32_t i = 0;
+	uint32x4_t vhash_val_a = vmovq_n_u32(hash_val_a);
+	uint32x4_t vhash_val_b = vmovq_n_u32(hash_val_b);
+	int32x4_t vshift = {0, 1, 2, 3};
+	uint32x4_t vmask = vdupq_n_u32(0x1);
+	int32x4_t vincr = vdupq_n_s32(4);
+
+	for (; i < RTE_EFD_VALUE_NUM_BITS; i += 4) {
+		uint32x4_t vhash_idx = vshll_n_u16(
+			vld1_u16((uint16_t const *)&group_hash_idx[i]), 0);
+		uint32x4_t vlookup_table = vshll_n_u16(
+			vld1_u16((uint16_t const *)&group_lookup_table[i]), 0);
+		uint32x4_t vhash = vaddq_u32(vhash_val_a,
+					vmulq_u32(vhash_idx, vhash_val_b));
+		int32x4_t vbucket_idx = vnegq_s32(vreinterpretq_s32_u32(
+				vshrq_n_u32(vhash, EFD_LOOKUPTBL_SHIFT)));
+		uint32x4_t vresult = vshlq_u32(vlookup_table, vbucket_idx);
+
+		vresult = vandq_u32(vresult, vmask);
+		vresult = vshlq_u32(vresult, vshift);
+		value |= vaddvq_u32(vresult);
+		vshift = vaddq_s32(vshift, vincr);
+	}
+
+	return value;
+}
+
+#endif /* __RTE_EFD_ARM64_H__ */
-- 
2.7.4

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

[Jerin Jacob]

-----Original Message-----
> Date: Mon,  1 May 2017 22:59:53 -0700
> From: Ashwin Sekhar T K <ashwin.sekhar@caviumnetworks.com>
> To: byron.marohn@intel.com, pablo.de.lara.guarch@intel.com,
>  jerin.jacob@caviumnetworks.com, jianbo.liu@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.sekhar@caviumnetworks.com>
> ---
> 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.

Any valid point to keep under RTE_ARCH_ARM64?

>  	EFD_LOOKUP_NUM
>  };

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

[Jianbo Liu]

On 2 May 2017 at 14:41, Jerin Jacob <jerin.jacob@caviumnetworks.com> wrote:
> -----Original Message-----
>> Date: Mon,  1 May 2017 22:59:53 -0700
>> From: Ashwin Sekhar T K <ashwin.sekhar@caviumnetworks.com>
>> To: byron.marohn@intel.com, pablo.de.lara.guarch@intel.com,
>>  jerin.jacob@caviumnetworks.com, jianbo.liu@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.sekhar@caviumnetworks.com>
>> ---
>> 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.

> Any valid point to keep under RTE_ARCH_ARM64?
>
>>       EFD_LOOKUP_NUM
>>  };

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

[Sekhar, Ashwin]

On Tue, 2017-05-02 at 15:59 +0800, Jianbo Liu wrote:
> On 2 May 2017 at 14:41, Jerin Jacob <jerin.jacob@caviumnetworks.com>
> wrote:
> > 
> > -----Original Message-----
> > > 
> > > Date: Mon,  1 May 2017 22:59:53 -0700
> > > From: Ashwin Sekhar T K <ashwin.sekhar@caviumnetworks.com>
> > > To: byron.marohn@intel.com, pablo.de.lara.guarch@intel.com,
> > >  jerin.jacob@caviumnetworks.com, jianbo.liu@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.sekhar@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.
> > 
> > Any valid point to keep under RTE_ARCH_ARM64?
> > 
> > > 
> > >       EFD_LOOKUP_NUM
> > >  };

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

[De Lara Guarch, Pablo]

Hi,

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