|SUCCESS| [v6,7/7] test/hash: add test for key compare functions

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/162772

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-03-30

commit d8f5a2e9c3
Author: Stephen Hemminger <stephen at networkplumber.org>

    hash: move table of hash compare functions out of header

This patch series refactors the hash library's key comparison functions
to reduce code duplication and improve maintainability.

> diff --git a/lib/hash/rte_cuckoo_hash.c b/lib/hash/rte_cuckoo_hash.c
> index 69dcfdfc39..9af02f2abd 100644
> --- a/lib/hash/rte_cuckoo_hash.c
> +++ b/lib/hash/rte_cuckoo_hash.c

[ ... ]

> +/*
> + * All different options to select a key compare function,
> + * based on the key size and custom function.
> + * Not in rte_cuckoo_hash.h to avoid ABI issues.
> + */
> +enum cmp_jump_table_case {
> +	KEY_CUSTOM = 0,
> +#if defined(RTE_ARCH_X86) || defined(RTE_ARCH_ARM64)
> +	KEY_16_BYTES,
> +	KEY_32_BYTES,
> +	KEY_48_BYTES,
> +	KEY_64_BYTES,
> +	KEY_80_BYTES,
> +	KEY_96_BYTES,
> +	KEY_112_BYTES,
> +	KEY_128_BYTES,
> +#endif
> +	KEY_OTHER_BYTES,
> +	NUM_KEY_CMP_CASES,
> +};

Does this enum definition introduce portability concerns when the same
hash table is accessed from multiple architectures in shared memory?
Since the enum values would differ between x86/ARM64 and other
platforms, can cmp_jump_table_idx remain consistent?

> +/*
> + * Table storing all different key compare functions
> + * (multi-process supported)
> + */
> +static const rte_hash_cmp_eq_t cmp_jump_table[NUM_KEY_CMP_CASES] = {
> +	[KEY_CUSTOM] = NULL,
> +#if defined(RTE_ARCH_X86) || defined(RTE_ARCH_ARM64)
> +	[KEY_16_BYTES] = rte_hash_k16_cmp_eq,
> +	[KEY_32_BYTES] = rte_hash_k32_cmp_eq,
> +	[KEY_48_BYTES] = rte_hash_k48_cmp_eq,
> +	[KEY_64_BYTES] = rte_hash_k64_cmp_eq,
> +	[KEY_80_BYTES] = rte_hash_k80_cmp_eq,
> +	[KEY_96_BYTES] = rte_hash_k96_cmp_eq,
> +	[KEY_112_BYTES] = rte_hash_k112_cmp_eq,
> +	[KEY_128_BYTES] = rte_hash_k128_cmp_eq,
> +#endif
> +	[KEY_OTHER_BYTES] = memcmp,
> +};

[ ... ]

> @@ -406,13 +462,13 @@ rte_hash_create(const struct rte_hash_parameters *params)
>  		goto err_unlock;
>  	}
>  
> -/*
> - * If x86 architecture is used, select appropriate compare function,
> - * which may use x86 intrinsics, otherwise use memcmp
> - */
> -#if defined(RTE_ARCH_X86) || defined(RTE_ARCH_ARM64)
>  	/* Select function to compare keys */
>  	switch (params->key_len) {
> +#if defined(RTE_ARCH_X86) || defined(RTE_ARCH_ARM64)
> +	/*
> +	 * If x86 architecture is used, select appropriate compare function,
> +	 * which may use x86 intrinsics, otherwise use memcmp
> +	 */
>  	case 16:
>  		h->cmp_jump_table_idx = KEY_16_BYTES;
>  		break;

[ ... ]

> diff --git a/lib/hash/rte_cuckoo_hash.h b/lib/hash/rte_cuckoo_hash.h
> index cf5cd29144..5b3c51348e 100644
> --- a/lib/hash/rte_cuckoo_hash.h
> +++ b/lib/hash/rte_cuckoo_hash.h

[ ... ]

> @@ -189,7 +112,7 @@ struct __rte_cache_aligned rte_hash {
>  	uint32_t hash_func_init_val;    /**< Init value used by hash_func. */
>  	rte_hash_cmp_eq_t rte_hash_custom_cmp_eq;
>  	/**< Custom function used to compare keys. */
> -	enum cmp_jump_table_case cmp_jump_table_idx;
> +	unsigned int cmp_jump_table_idx;
>  	/**< Indicates which compare function to use. */
>  	unsigned int sig_cmp_fn;
>  	/**< Indicates which signature compare function to use. */

commit 1f3b8d4a2e
Author: Stephen Hemminger <stephen at networkplumber.org>

    hash: use static_assert

[ ... ]

commit 5e9c7f2d3a
Author: Stephen Hemminger <stephen at networkplumber.org>

    hash: remove spurious warnings in CRC32 init

[ ... ]

commit 4a8e6b9f1c
Author: Stephen Hemminger <stephen at networkplumber.org>

    hash: simplify key comparison across architectures

> diff --git a/lib/hash/rte_cmp_arm64.h b/lib/hash/rte_cmp_arm64.h
> index a3e85635eb..f209aaf474 100644
> --- a/lib/hash/rte_cmp_arm64.h
> +++ b/lib/hash/rte_cmp_arm64.h

[ ... ]

>  static inline int
> -rte_hash_k32_cmp_eq(const void *key1, const void *key2, size_t key_len)
> +rte_hash_k32_cmp_eq(const void *key1, const void *key2, size_t key_len __rte_unused)
>  {
> -	return rte_hash_k16_cmp_eq(key1, key2, key_len) ||
> -		rte_hash_k16_cmp_eq((const char *) key1 + 16,
> -				(const char *) key2 + 16, key_len);

Does the switch from logical OR to bitwise OR change the short-circuit
behavior in a way that affects performance?

> +	return rte_hash_k16_cmp_eq(key1, key2, 16) |
> +		rte_hash_k16_cmp_eq((const uint8_t *)key1 + 16,
> +				    (const uint8_t *)key2 + 16, 16);
>  }

[ ... ]

> diff --git a/lib/hash/rte_cmp_generic.h b/lib/hash/rte_cmp_generic.h
> new file mode 100644
> index 0000000000..771180e97a
> --- /dev/null
> +++ b/lib/hash/rte_cmp_generic.h

[ ... ]

> +/* Function to compare 16 byte keys */
> +static inline int
> +rte_hash_k16_cmp_eq(const void *key1, const void *key2, size_t key_len __rte_unused)
> +{
> +#ifdef RTE_ARCH_64
> +	const unaligned_uint64_t *k1 = key1;
> +	const unaligned_uint64_t *k2 = key2;
> +
> +	return !!((k1[0] ^ k2[0]) | (k1[1] ^ k2[1]));
> +#else
> +	const unaligned_uint32_t *k1 = key1;
> +	const unaligned_uint32_t *k2 = key2;
> +
> +	return !!((k1[0] ^ k2[0]) | (k1[1] ^ k2[1]) |
> +		  (k1[2] ^ k2[2]) | (k1[3] ^ k2[3]));
> +#endif
> +}

[ ... ]

> diff --git a/lib/hash/rte_cuckoo_hash.c b/lib/hash/rte_cuckoo_hash.c
> index 9af02f2abd..5bbc3c5464 100644
> --- a/lib/hash/rte_cuckoo_hash.c
> +++ b/lib/hash/rte_cuckoo_hash.c

[ ... ]

> +static int
> +rte_hash_k48_cmp_eq(const void *key1, const void *key2, size_t key_len)
> +{
> +	return rte_hash_k16_cmp_eq(key1, key2, key_len) |
> +		rte_hash_k16_cmp_eq((const uint8_t *) key1 + 16,
> +				    (const uint8_t *) key2 + 16, key_len) |
> +		rte_hash_k16_cmp_eq((const uint8_t *) key1 + 32,
> +				    (const uint8_t *) key2 + 32, key_len);
> +}

Does this code leak the key_len parameter? rte_hash_k48_cmp_eq is
called with a fixed key_len=48, but passes this value to
rte_hash_k16_cmp_eq which expects key_len=16 (or marks it __rte_unused).

[ ... ]

commit a9f2c5e8d1
Author: Stephen Hemminger <stephen at networkplumber.org>

    hash: add support for common small key sizes

> diff --git a/lib/hash/rte_cuckoo_hash.c b/lib/hash/rte_cuckoo_hash.c
> index 5bbc3c5464..5d495ae973 100644
> --- a/lib/hash/rte_cuckoo_hash.c
> +++ b/lib/hash/rte_cuckoo_hash.c

[ ... ]

> +static inline int
> +rte_hash_k2_cmp_eq(const void *key1, const void *key2, size_t key_len __rte_unused)
> +{
> +	const unaligned_uint16_t *k1 = key1;
> +	const unaligned_uint16_t *k2 = key2;
> +
> +	return !!(k1[0] ^ k2[0]);
> +}

[ ... ]

> +static int
> +rte_hash_k3_cmp_eq(const void *key1, const void *key2, size_t key_len __rte_unused)
> +{
> +	return rte_hash_k2_cmp_eq(key1, key2, 2)
> +		| (((const uint8_t *)key1)[2] ^ ((const uint8_t *)key2)[2]);
> +}

Does this code pass the wrong key_len to rte_hash_k2_cmp_eq? The
function is marked __rte_unused, but the call passes 2 explicitly.

[ ... ]

> +static inline int
> +rte_hash_k64_cmp_eq(const void *key1, const void *key2, size_t key_len __rte_unused)
>  {
> -	return rte_hash_k32_cmp_eq(key1, key2, key_len) |
> -		rte_hash_k32_cmp_eq((const uint8_t *) key1 + 32,
> -				    (const uint8_t *) key2 + 32, key_len);
> +	return rte_hash_k32_cmp_eq(key1, key2, 32) |
> +		rte_hash_k32_cmp_eq((const uint8_t *)key1 + 32,
> +				    (const uint8_t *)key2 + 32, 32);
>  }
>  
>  static int
> -rte_hash_k80_cmp_eq(const void *key1, const void *key2, size_t key_len)
> +rte_hash_k80_cmp_eq(const void *key1, const void *key2, size_t key_len __rte_unused)
>  {
> -	return rte_hash_k64_cmp_eq(key1, key2, key_len) |
> -		rte_hash_k16_cmp_eq((const uint8_t *) key1 + 64,
> -				    (const uint8_t *) key2 + 64, key_len);
> +	return rte_hash_k64_cmp_eq(key1, key2, 64) |
> +		rte_hash_k16_cmp_eq((const uint8_t *)key1 + 64,
> +				    (const uint8_t *)key2 + 64, 64);

Does this code overflow the key buffer? rte_hash_k80_cmp_eq is called
for 80-byte keys, but compares 64+16 bytes and then passes key_len=64 to
rte_hash_k16_cmp_eq when it should be 16.

[ ... ]