Re: [dpdk-dev] [PATCH] test/hash: improve hash unit tests

[Bruce Richardson <bruce.richardson@intel.com>]

On Wed, Jul 08, 2015 at 02:12:06PM +0100, Pablo de Lara wrote:
> Add new unit test for calculating the average table utilization,
> using random keys, based on number of entries that can be added
> until we encounter one that cannot be added (bucket if full)
> 
> Also, replace current hash_perf unit test to see performance more clear.
> The current hash_perf unit test takes too long and add keys that
> may or may not fit in the table and look up/delete that may not be
> in the table. This new unit test gets a set of keys that we know
> that fits in the table, and then measure the time to add/look up/delete
> them.
> 
> Signed-off-by: Pablo de Lara <pablo.de.lara.guarch@intel.com>

Few more comments on the change to test_hash.c

/Bruce
> ---
>  app/test/test_hash.c      |  61 ++++
>  app/test/test_hash_perf.c | 906 +++++++++++++++++++---------------------------
>  2 files changed, 439 insertions(+), 528 deletions(-)
> 
> diff --git a/app/test/test_hash.c b/app/test/test_hash.c
> index 4300de9..4d538b2 100644
> --- a/app/test/test_hash.c
> +++ b/app/test/test_hash.c
> @@ -1147,6 +1147,65 @@ test_hash_creation_with_good_parameters(void)
>  	return 0;
>  }
>  
> +#define ITERATIONS 50
> +/*
> + * Test to see the average table utilization (entries added/max entries)
> + * before hitting a random entry that cannot be added
> + */
> +static int test_average_table_utilization(void)
> +{
> +	struct rte_hash *handle;
> +	void *simple_key;
> +	unsigned i, j, no_space = 0;
> +	double added_keys_until_no_space = 0;
> +	int ret;
> +
> +	ut_params.entries = 1 << 20;
> +	ut_params.name = "test_average_utilization";
> +	ut_params.hash_func = rte_jhash;
> +	handle = rte_hash_create(&ut_params);
> +	RETURN_IF_ERROR(handle == NULL, "hash creation failed");
> +
> +	simple_key = rte_zmalloc(NULL, ut_params.key_len, 0);
> +
> +	for (j = 0; j < ITERATIONS; j++) {
> +		while (!no_space) {
> +			for (i = 0; i < ut_params.key_len; i++)
> +				((uint8_t *) simple_key)[i] = rte_rand() % 255;
> +
> +			ret = rte_hash_add_key(handle, simple_key);
> +			print_key_info("Add", simple_key, ret);
> +
> +			if (ret == -ENOSPC) {
> +				if (rte_hash_lookup(handle, simple_key) != -ENOENT)
> +					printf("Found key that should not be present\n");
Should this not be an immediate test failure?
In fact, is it really worth testing, for this condition. Why not just have
the loop and test as:

do {
	/*set up simple key */
} while ((ret = rte_hash_add_key(...)) >= 0);
if (ret != -ENOSPC) {
	/* print error */
	return -1;
}

> +				no_space = 1;
> +			} else {
> +				if (ret < 0)
> +					rte_free(simple_key);

Rather than using malloc free, why not just make simple_key a local array of
size MAX_KEY_SIZE.

> +				RETURN_IF_ERROR(ret < 0,
> +						"failed to add key (ret=%d)", ret);
> +				added_keys_until_no_space++;
> +			}
> +		}
> +		no_space = 0;
> +
> +		/* Reset the table */
> +		rte_hash_free(handle);
> +		handle = rte_hash_create(&ut_params);
> +		RETURN_IF_ERROR(handle == NULL, "hash creation failed");

Would a reset call work better than a free/recreate?

> +	}
> +
> +	const unsigned average_keys_added = added_keys_until_no_space / ITERATIONS;
> +
> +	printf("Average table utilization = %.2f%% (%u/%u)\n",
> +		((double) average_keys_added / ut_params.entries * 100),
> +		average_keys_added, ut_params.entries);
> +	rte_hash_free(handle);
> +
> +	return 0;
> +}
> +
>  static uint8_t key[16] = {0x00, 0x01, 0x02, 0x03,
>  			0x04, 0x05, 0x06, 0x07,
>  			0x08, 0x09, 0x0a, 0x0b,
> @@ -1405,6 +1464,8 @@ test_hash(void)
>  		return -1;
>  	if (test_hash_creation_with_good_parameters() < 0)
>  		return -1;
> +	if (test_average_table_utilization() < 0)
> +		return -1;
>  
>  	run_hash_func_tests();
>