From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by dpdk.org (Postfix) with ESMTP id B3A725A57 for ; Wed, 8 Jul 2015 17:17:59 +0200 (CEST) Received: from fmsmga001.fm.intel.com ([10.253.24.23]) by fmsmga102.fm.intel.com with ESMTP; 08 Jul 2015 08:17:58 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.15,432,1432623600"; d="scan'208";a="743034496" Received: from irsmsx154.ger.corp.intel.com ([163.33.192.96]) by fmsmga001.fm.intel.com with ESMTP; 08 Jul 2015 08:17:58 -0700 Received: from irsmsx108.ger.corp.intel.com ([169.254.11.201]) by IRSMSX154.ger.corp.intel.com ([169.254.12.91]) with mapi id 14.03.0224.002; Wed, 8 Jul 2015 16:14:40 +0100 From: "De Lara Guarch, Pablo" To: "Richardson, Bruce" Thread-Topic: [dpdk-dev] [PATCH] test/hash: improve hash unit tests Thread-Index: AQHQuX/IGJozpdODYUueu1VXpduK/J3RmvWAgAAR49A= Date: Wed, 8 Jul 2015 15:14:39 +0000 Message-ID: References: <1436361126-22178-1-git-send-email-pablo.de.lara.guarch@intel.com> <1436361126-22178-2-git-send-email-pablo.de.lara.guarch@intel.com> <20150708150417.GB2676@bricha3-MOBL3> In-Reply-To: <20150708150417.GB2676@bricha3-MOBL3> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [163.33.239.180] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 Cc: "dev@dpdk.org" Subject: Re: [dpdk-dev] [PATCH] test/hash: improve hash unit tests X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 08 Jul 2015 15:18:00 -0000 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 >=20 > 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 >=20 > Few more comments on the change to test_hash.c >=20 > /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 entrie= s) > > + * 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 =3D 0; > > + double added_keys_until_no_space =3D 0; > > + int ret; > > + > > + ut_params.entries =3D 1 << 20; > > + ut_params.name =3D "test_average_utilization"; > > + ut_params.hash_func =3D rte_jhash; > > + handle =3D rte_hash_create(&ut_params); > > + RETURN_IF_ERROR(handle =3D=3D NULL, "hash creation failed"); > > + > > + simple_key =3D rte_zmalloc(NULL, ut_params.key_len, 0); > > + > > + for (j =3D 0; j < ITERATIONS; j++) { > > + while (!no_space) { > > + for (i =3D 0; i < ut_params.key_len; i++) > > + ((uint8_t *) simple_key)[i] =3D rte_rand() % > 255; > > + > > + ret =3D rte_hash_add_key(handle, simple_key); > > + print_key_info("Add", simple_key, ret); > > + > > + if (ret =3D=3D -ENOSPC) { > > + if (rte_hash_lookup(handle, simple_key) !=3D - > 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 hav= e > the loop and test as: >=20 > do { > /*set up simple key */ > } while ((ret =3D rte_hash_add_key(...)) >=3D 0); > if (ret !=3D -ENOSPC) { > /* print error */ > return -1; > } >=20 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 =3D 1; > > + } else { > > + if (ret < 0) > > + rte_free(simple_key); >=20 > Rather than using malloc free, why not just make simple_key a local array= of > size MAX_KEY_SIZE. Will do. >=20 > > + RETURN_IF_ERROR(ret < 0, > > + "failed to add key (ret=3D%d)", > ret); > > + added_keys_until_no_space++; > > + } > > + } > > + no_space =3D 0; > > + > > + /* Reset the table */ > > + rte_hash_free(handle); > > + handle =3D rte_hash_create(&ut_params); > > + RETURN_IF_ERROR(handle =3D=3D NULL, "hash creation failed"); >=20 > 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. >=20 > > + } > > + > > + const unsigned average_keys_added =3D added_keys_until_no_space > / ITERATIONS; > > + > > + printf("Average table utilization =3D %.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] =3D {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(); > >