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

["De Lara Guarch, Pablo" <pablo.de.lara.guarch@intel.com>]

Hi Bruce,

> -----Original Message-----
> From: Richardson, Bruce
> Sent: Wednesday, July 08, 2015 4:04 PM
> To: De Lara Guarch, Pablo
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] test/hash: improve hash unit tests
> 
> 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;
> }
> 

Sure, I forgot to return an error in this case.
And yes, you are right, that's more elegant.
Only thing missing there is freeing the hash table.

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

Will do.

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

It would, but that function was not present in the current implementation.
I have added it in the new implementation, so I changed this as soon as
I implement it.

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