Hi Bruce,
> -----Original Message-----
> From: Richardson, Bruce
> Sent: Wednesday, July 08, 2015 4:04 PM
> To: De Lara Guarch, Pablo
> Cc: dev at 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 at 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();
> >
