|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

I'll review this DPDK patch series focusing on correctness, coding style, and guideline compliance.

commit 20260329232409.205940-2-stephen at networkplumber.org
Author: Stephen Hemminger <stephen at networkplumber.org>

    hash: move table of hash compare functions out of header

This commit moves the compare jump table from the header file to the
source file to prevent ABI breakage when adding new key sizes.

> 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

[ ... ]

> @@ -25,14 +25,51 @@
>  #include <rte_tailq.h>
> 
>  #include "rte_hash.h"
> +#include "rte_cuckoo_hash.h"
> 
> -/* needs to be before rte_cuckoo_hash.h */
>  RTE_LOG_REGISTER_DEFAULT(hash_logtype, INFO);

Does moving the header include after RTE_LOG_REGISTER_DEFAULT maintain
the same behavior? The original comment indicated ordering requirements.

> +/*
> + * 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,

Is this conditional compilation still correct when cmp_jump_table
references these enum values unconditionally in the static array
initializer below?

> +/*
> + * 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,

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

    hash: remove spurious warnings in CRC32 init

> diff --git a/lib/hash/rte_hash_crc.c b/lib/hash/rte_hash_crc.c
> index 523be0bf92..8b70982c4b 100644
> --- a/lib/hash/rte_hash_crc.c
> +++ b/lib/hash/rte_hash_crc.c

[ ... ]

> @@ -37,9 +37,6 @@ rte_hash_crc_set_alg(uint8_t alg)
>  		return;
> 
>  #if defined RTE_ARCH_X86
> -	if (!(alg & CRC32_SSE42_x64))
> -		HASH_CRC_LOG(WARNING,
> -			"Unsupported CRC32 algorithm requested using CRC32_x64/CRC32_SSE42");
>  	if (!rte_cpu_get_flag_enabled(RTE_CPUFLAG_EM64T) || alg == CRC32_SSE42)
>  		rte_hash_crc32_alg = CRC32_SSE42;
>  	else
> @@ -47,16 +44,13 @@ rte_hash_crc_set_alg(uint8_t alg)
>  #endif
> 
>  #if defined RTE_ARCH_ARM64
> -	if (!(alg & CRC32_ARM64))
> -		HASH_CRC_LOG(WARNING,
> -			"Unsupported CRC32 algorithm requested using CRC32_ARM64");
>  	if (rte_cpu_get_flag_enabled(RTE_CPUFLAG_CRC32))
>  		rte_hash_crc32_alg = CRC32_ARM64;
>  #endif

After these removals, what happens if the user explicitly requests
CRC32_ARM64 but the CPU flag check fails? Does this code silently fall
through to CRC32_SW without notification?

commit 20260329232409.205940-5-stephen at networkplumber.org
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);
> +	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);

Can this replacement of logical OR (||) with bitwise OR (|) affect
behavior? The comment mentions avoiding branching, but does this ensure
the second call is always evaluated even when the first returns nonzero?

> 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]));

Does this double negation (!!) correctly normalize all non-zero values
to exactly 1 for consistency with other compare functions?

> +#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

[ ... ]

> +/*
> + * Comparison functions for different key sizes.
> + * Each function is only called with a specific fixed key size.
> + *
> + * Return value is 0 on equality to allow direct use of memcmp.
> + * Recommend using XOR and | operator to avoid branching
> + * as long as key is smaller than cache line size.
> + *
> + * Key1 always points to key[] in rte_hash_key which is aligned.
> + * Key2 is parameter to insert which might not be.
> + *
> + * Special cases for 16 and 32 bytes to allow for architecture
> + * specific optimizations.
> + */

The comment states "Return value is 0 on equality" but the code uses !!()
which returns 1 for non-equal. Is the comment incorrect?

commit 20260329232409.205940-6-stephen at networkplumber.org
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

[ ... ]

> +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]);
> +}

Does this need the !! since XOR already produces 0 for equal values?

> +static int
> +rte_hash_k5_cmp_eq(const void *key1, const void *key2, size_t key_len __rte_unused)
> +{
> +	const uint8_t *k1 = key1;
> +	const uint8_t *k2 = key2;
> +
> +	return rte_hash_k4_cmp_eq(key1, key2, 4) | (k1[4] ^ k2[4]);
> +}

Does the lack of !! here on the single-byte compare match the style of
the returned value from rte_hash_k4_cmp_eq?

> +static int
> +rte_hash_k14_cmp_eq(const void *key1, const void *key2, size_t key_len __rte_unused)
> +{
> +	return rte_hash_k8_cmp_eq(key1, key2, 8) |
> +		rte_hash_k6_cmp_eq((const uint8_t *)key1 + 8,
> +				   (const uint8_t *)key2 + 8, 6);
> +}

Does passing the literal key_len parameters (8, 6) to functions marked
__rte_unused waste instruction encoding space?

> @@ -526,41 +657,15 @@ rte_hash_create(const struct rte_hash_parameters *params)
>  		goto err_unlock;
>  	}
> 
> -	/* Select function to compare keys */
> -	switch (params->key_len) {
> -#if defined(RTE_ARCH_X86) || defined(RTE_ARCH_ARM64)

[ ... ]

> -	default:
> -		/* If key is not multiple of 16, use generic memcmp */
> -		h->cmp_jump_table_idx = KEY_OTHER_BYTES;
> +	/* fallback if no special case */
> +	h->cmp_jump_table_idx = KEY_OTHER_BYTES;
> +
> +	/* Search table of enum values, 0 is reserved for custom */
> +	for (unsigned int key_idx = KEY_2_BYTES; key_idx < KEY_OTHER_BYTES; key_idx++) {
> +		if (params->key_len == cmp_jump_key_size[key_idx]) {
> +			h->cmp_jump_table_idx = key_idx;
> +			break;
> +		}
>  	}

Does this linear search through cmp_jump_key_size[] scale well if more
key sizes are added? Would a binary search or lookup table be better?

commit 20260329232409.205940-8-stephen at networkplumber.org
Author: Stephen Hemminger <stephen at networkplumber.org>

    test/hash: add test for key compare functions

> diff --git a/app/test/test_hash.c b/app/test/test_hash.c
> index 0ddf71ff3c..640a33e16c 100644
> --- a/app/test/test_hash.c

[ ... ]

> +		/* modify one byte and lookup should fail */
> +		key[key_len - 1] ^= 0xff;
> +		pos = rte_hash_lookup(handle, key);
> +		if (pos != -ENOENT) {
> +			printf("Lookup modified key with key_len %u should have failed\n",
> +			       key_len);
> +			rte_hash_free(handle);
> +			return -1;
> +		}

Does modifying only the last byte sufficiently test the compare
function? Could keys that differ only in earlier bytes also be tested?