Message ID | 20200203194912.4669-4-honnappa.nagarahalli@arm.com |
---|---|
State | New |
Headers | show |
Series | test/meson: fix hash readwrite timeout failure | expand |
On Mon, Feb 3, 2020 at 8:49 PM Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> wrote: > > Add lock-free reader writer concurrency functional tests. > These tests will provide the same coverage that non lock-free > APIs have. > > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> > --- > app/test/test_hash_readwrite.c | 58 +++++++++++++++++++++------------- > 1 file changed, 36 insertions(+), 22 deletions(-) > > diff --git a/app/test/test_hash_readwrite.c b/app/test/test_hash_readwrite.c > index 635ed5a9f..a9429091c 100644 > --- a/app/test/test_hash_readwrite.c > +++ b/app/test/test_hash_readwrite.c > @@ -121,7 +121,7 @@ test_hash_readwrite_worker(__attribute__((unused)) void *arg) > } > > static int > -init_params(int use_ext, int use_htm, int use_jhash) > +init_params(int use_ext, int use_htm, int rw_lf, int use_jhash) > { > unsigned int i; > > @@ -140,15 +140,16 @@ init_params(int use_ext, int use_htm, int use_jhash) > else > hash_params.hash_func = rte_hash_crc; > > + hash_params.extra_flag = RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD; > if (use_htm) > - hash_params.extra_flag = > - RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT | > - RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY | > - RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD; > + hash_params.extra_flag |= > + RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT; > + if (rw_lf) > + hash_params.extra_flag |= > + RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF; > else > - hash_params.extra_flag = > - RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY | > - RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD; > + hash_params.extra_flag |= > + RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY; > > if (use_ext) > hash_params.extra_flag |= > @@ -195,7 +196,7 @@ init_params(int use_ext, int use_htm, int use_jhash) > } > > static int > -test_hash_readwrite_functional(int use_ext, int use_htm) > +test_hash_readwrite_functional(int use_htm, int use_rw_lf, int use_ext) This is a bit hard to read, please keep the same order than init_params. > { > unsigned int i; > const void *next_key; > @@ -214,7 +215,7 @@ test_hash_readwrite_functional(int use_ext, int use_htm) > rte_atomic64_init(&ginsertions); > rte_atomic64_clear(&ginsertions); > > - if (init_params(use_ext, use_htm, use_jhash) != 0) > + if (init_params(use_ext, use_htm, use_rw_lf, use_jhash) != 0) > goto err; > > if (use_ext) > @@ -229,6 +230,8 @@ test_hash_readwrite_functional(int use_ext, int use_htm) > tbl_rw_test_param.num_insert > * slave_cnt; > > + printf("\nHTM = %d, RW-LF = %d, EXT-Table = %d\n", > + use_htm, use_rw_lf, use_ext); > printf("++++++++Start function tests:+++++++++\n"); > > /* Fire all threads. */ > @@ -379,7 +382,7 @@ test_hash_readwrite_perf(struct perf *perf_results, int use_htm, > rte_atomic64_init(&gwrite_cycles); > rte_atomic64_clear(&gwrite_cycles); > > - if (init_params(0, use_htm, use_jhash) != 0) > + if (init_params(0, use_htm, 0, use_jhash) != 0) > goto err; > > /* > @@ -700,7 +703,6 @@ test_hash_rw_func_main(void) > * than writer threads. This is to timing either reader threads or > * writer threads for performance numbers. > */ > - int use_htm, use_ext; The comments block just before is out of sync. > unsigned int i = 0, core_id = 0; > > if (rte_lcore_count() < 3) { > @@ -721,29 +723,41 @@ test_hash_rw_func_main(void) > > printf("Test read-write with Hardware transactional memory\n"); > > - use_htm = 1; > - use_ext = 0; > + /* htm = 1, rw_lf = 0, ext = 0 */ I didn't like those local variables. But comments tend to get out of sync fairly easily, please remove too. > + if (test_hash_readwrite_functional(1, 0, 0) < 0) > + return -1; > > - if (test_hash_readwrite_functional(use_ext, use_htm) < 0) > + /* htm = 1, rw_lf = 1, ext = 0 */ > + if (test_hash_readwrite_functional(1, 1, 0) < 0) > return -1; > > - use_ext = 1; > - if (test_hash_readwrite_functional(use_ext, use_htm) < 0) > + /* htm = 1, rw_lf = 0, ext = 1 */ > + if (test_hash_readwrite_functional(1, 0, 1) < 0) > return -1; > > + /* htm = 1, rw_lf = 1, ext = 1 */ > + if (test_hash_readwrite_functional(1, 1, 1) < 0) > + return -1; > } else { > printf("Hardware transactional memory (lock elision) " > "is NOT supported\n"); > } > > printf("Test read-write without Hardware transactional memory\n"); > - use_htm = 0; > - use_ext = 0; > - if (test_hash_readwrite_functional(use_ext, use_htm) < 0) > + /* htm = 0, rw_lf = 0, ext = 0 */ > + if (test_hash_readwrite_functional(0, 0, 0) < 0) > + return -1; > + > + /* htm = 0, rw_lf = 1, ext = 0 */ > + if (test_hash_readwrite_functional(0, 1, 0) < 0) > + return -1; > + > + /* htm = 0, rw_lf = 0, ext = 1 */ > + if (test_hash_readwrite_functional(0, 0, 1) < 0) > return -1; > > - use_ext = 1; > - if (test_hash_readwrite_functional(use_ext, use_htm) < 0) > + /* htm = 0, rw_lf = 1, ext = 1 */ > + if (test_hash_readwrite_functional(0, 1, 1) < 0) > return -1; > > return 0; > -- > 2.17.1 > -- David Marchand
> > On Mon, Feb 3, 2020 at 8:49 PM Honnappa Nagarahalli > <honnappa.nagarahalli@arm.com> wrote: > > > > Add lock-free reader writer concurrency functional tests. > > These tests will provide the same coverage that non lock-free APIs > > have. > > > > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> > > --- > > app/test/test_hash_readwrite.c | 58 > > +++++++++++++++++++++------------- > > 1 file changed, 36 insertions(+), 22 deletions(-) > > > > diff --git a/app/test/test_hash_readwrite.c > > b/app/test/test_hash_readwrite.c index 635ed5a9f..a9429091c 100644 > > --- a/app/test/test_hash_readwrite.c > > +++ b/app/test/test_hash_readwrite.c > > @@ -121,7 +121,7 @@ > test_hash_readwrite_worker(__attribute__((unused)) > > void *arg) } > > > > static int > > -init_params(int use_ext, int use_htm, int use_jhash) > > +init_params(int use_ext, int use_htm, int rw_lf, int use_jhash) > > { > > unsigned int i; > > > > @@ -140,15 +140,16 @@ init_params(int use_ext, int use_htm, int > use_jhash) > > else > > hash_params.hash_func = rte_hash_crc; > > > > + hash_params.extra_flag = > > + RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD; > > if (use_htm) > > - hash_params.extra_flag = > > - RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT | > > - RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY | > > - RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD; > > + hash_params.extra_flag |= > > + RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT; > > + if (rw_lf) > > + hash_params.extra_flag |= > > + RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF; > > else > > - hash_params.extra_flag = > > - RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY | > > - RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD; > > + hash_params.extra_flag |= > > + RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY; > > > > if (use_ext) > > hash_params.extra_flag |= @@ -195,7 +196,7 @@ > > init_params(int use_ext, int use_htm, int use_jhash) } > > > > static int > > -test_hash_readwrite_functional(int use_ext, int use_htm) > > +test_hash_readwrite_functional(int use_htm, int use_rw_lf, int > > +use_ext) > > This is a bit hard to read, please keep the same order than init_params. It looks like it is better to change the init_params. Otherwise, the code in test_hash_rw_func_main becomes hard to read. See the comment below. > > > > { > > unsigned int i; > > const void *next_key; > > @@ -214,7 +215,7 @@ test_hash_readwrite_functional(int use_ext, int > use_htm) > > rte_atomic64_init(&ginsertions); > > rte_atomic64_clear(&ginsertions); > > > > - if (init_params(use_ext, use_htm, use_jhash) != 0) > > + if (init_params(use_ext, use_htm, use_rw_lf, use_jhash) != 0) > > goto err; > > > > if (use_ext) > > @@ -229,6 +230,8 @@ test_hash_readwrite_functional(int use_ext, int > use_htm) > > tbl_rw_test_param.num_insert > > * slave_cnt; > > > > + printf("\nHTM = %d, RW-LF = %d, EXT-Table = %d\n", > > + use_htm, use_rw_lf, use_ext); > > printf("++++++++Start function tests:+++++++++\n"); > > > > /* Fire all threads. */ > > @@ -379,7 +382,7 @@ test_hash_readwrite_perf(struct perf *perf_results, > int use_htm, > > rte_atomic64_init(&gwrite_cycles); > > rte_atomic64_clear(&gwrite_cycles); > > > > - if (init_params(0, use_htm, use_jhash) != 0) > > + if (init_params(0, use_htm, 0, use_jhash) != 0) > > goto err; > > > > /* > > @@ -700,7 +703,6 @@ test_hash_rw_func_main(void) > > * than writer threads. This is to timing either reader threads or > > * writer threads for performance numbers. > > */ > > - int use_htm, use_ext; > > The comments block just before is out of sync. > > > > unsigned int i = 0, core_id = 0; > > > > if (rte_lcore_count() < 3) { > > @@ -721,29 +723,41 @@ test_hash_rw_func_main(void) > > > > printf("Test read-write with Hardware transactional > > memory\n"); > > > > - use_htm = 1; > > - use_ext = 0; > > + /* htm = 1, rw_lf = 0, ext = 0 */ > > I didn't like those local variables. > But comments tend to get out of sync fairly easily, please remove too. > > > > + if (test_hash_readwrite_functional(1, 0, 0) < 0) > > + return -1; > > > > - if (test_hash_readwrite_functional(use_ext, use_htm) < 0) > > + /* htm = 1, rw_lf = 1, ext = 0 */ > > + if (test_hash_readwrite_functional(1, 1, 0) < 0) > > return -1; > > > > - use_ext = 1; > > - if (test_hash_readwrite_functional(use_ext, use_htm) < 0) > > + /* htm = 1, rw_lf = 0, ext = 1 */ > > + if (test_hash_readwrite_functional(1, 0, 1) < 0) > > return -1; > > > > + /* htm = 1, rw_lf = 1, ext = 1 */ > > + if (test_hash_readwrite_functional(1, 1, 1) < 0) > > + return -1; > > } else { > > printf("Hardware transactional memory (lock elision) " > > "is NOT supported\n"); > > } > > > > printf("Test read-write without Hardware transactional memory\n"); > > - use_htm = 0; > > - use_ext = 0; > > - if (test_hash_readwrite_functional(use_ext, use_htm) < 0) > > + /* htm = 0, rw_lf = 0, ext = 0 */ > > + if (test_hash_readwrite_functional(0, 0, 0) < 0) > > + return -1; > > + > > + /* htm = 0, rw_lf = 1, ext = 0 */ > > + if (test_hash_readwrite_functional(0, 1, 0) < 0) > > + return -1; > > + > > + /* htm = 0, rw_lf = 0, ext = 1 */ > > + if (test_hash_readwrite_functional(0, 0, 1) < 0) > > return -1; > > > > - use_ext = 1; > > - if (test_hash_readwrite_functional(use_ext, use_htm) < 0) > > + /* htm = 0, rw_lf = 1, ext = 1 */ > > + if (test_hash_readwrite_functional(0, 1, 1) < 0) > > return -1; The ordering of bits (0-0-0, 0-1-0, 0-0-1, 0-1-1) looks better here. > > > > return 0; > > -- > > 2.17.1 > > > > > -- > David Marchand
On Wed, Feb 5, 2020 at 5:22 PM Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com> wrote: > > > > > On Mon, Feb 3, 2020 at 8:49 PM Honnappa Nagarahalli > > <honnappa.nagarahalli@arm.com> wrote: > > > > > > Add lock-free reader writer concurrency functional tests. > > > These tests will provide the same coverage that non lock-free APIs > > > have. > > > > > > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> > > > --- > > > app/test/test_hash_readwrite.c | 58 > > > +++++++++++++++++++++------------- > > > 1 file changed, 36 insertions(+), 22 deletions(-) > > > > > > diff --git a/app/test/test_hash_readwrite.c > > > b/app/test/test_hash_readwrite.c index 635ed5a9f..a9429091c 100644 > > > --- a/app/test/test_hash_readwrite.c > > > +++ b/app/test/test_hash_readwrite.c > > > @@ -121,7 +121,7 @@ > > test_hash_readwrite_worker(__attribute__((unused)) > > > void *arg) } > > > > > > static int > > > -init_params(int use_ext, int use_htm, int use_jhash) > > > +init_params(int use_ext, int use_htm, int rw_lf, int use_jhash) > > > { > > > unsigned int i; > > > > > > @@ -140,15 +140,16 @@ init_params(int use_ext, int use_htm, int > > use_jhash) > > > else > > > hash_params.hash_func = rte_hash_crc; > > > > > > + hash_params.extra_flag = > > > + RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD; > > > if (use_htm) > > > - hash_params.extra_flag = > > > - RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT | > > > - RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY | > > > - RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD; > > > + hash_params.extra_flag |= > > > + RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT; > > > + if (rw_lf) > > > + hash_params.extra_flag |= > > > + RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF; > > > else > > > - hash_params.extra_flag = > > > - RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY | > > > - RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD; > > > + hash_params.extra_flag |= > > > + RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY; > > > > > > if (use_ext) > > > hash_params.extra_flag |= @@ -195,7 +196,7 @@ > > > init_params(int use_ext, int use_htm, int use_jhash) } > > > > > > static int > > > -test_hash_readwrite_functional(int use_ext, int use_htm) > > > +test_hash_readwrite_functional(int use_htm, int use_rw_lf, int > > > +use_ext) > > > > This is a bit hard to read, please keep the same order than init_params. > It looks like it is better to change the init_params. Otherwise, the code in test_hash_rw_func_main becomes hard to read. See the comment below. > > > > > > > > { > > > unsigned int i; > > > const void *next_key; > > > @@ -214,7 +215,7 @@ test_hash_readwrite_functional(int use_ext, int > > use_htm) > > > rte_atomic64_init(&ginsertions); > > > rte_atomic64_clear(&ginsertions); > > > > > > - if (init_params(use_ext, use_htm, use_jhash) != 0) > > > + if (init_params(use_ext, use_htm, use_rw_lf, use_jhash) != 0) > > > goto err; > > > > > > if (use_ext) > > > @@ -229,6 +230,8 @@ test_hash_readwrite_functional(int use_ext, int > > use_htm) > > > tbl_rw_test_param.num_insert > > > * slave_cnt; > > > > > > + printf("\nHTM = %d, RW-LF = %d, EXT-Table = %d\n", > > > + use_htm, use_rw_lf, use_ext); > > > printf("++++++++Start function tests:+++++++++\n"); > > > > > > /* Fire all threads. */ > > > @@ -379,7 +382,7 @@ test_hash_readwrite_perf(struct perf *perf_results, > > int use_htm, > > > rte_atomic64_init(&gwrite_cycles); > > > rte_atomic64_clear(&gwrite_cycles); > > > > > > - if (init_params(0, use_htm, use_jhash) != 0) > > > + if (init_params(0, use_htm, 0, use_jhash) != 0) > > > goto err; > > > > > > /* > > > @@ -700,7 +703,6 @@ test_hash_rw_func_main(void) > > > * than writer threads. This is to timing either reader threads or > > > * writer threads for performance numbers. > > > */ > > > - int use_htm, use_ext; > > > > The comments block just before is out of sync. > > > > > > > unsigned int i = 0, core_id = 0; > > > > > > if (rte_lcore_count() < 3) { > > > @@ -721,29 +723,41 @@ test_hash_rw_func_main(void) > > > > > > printf("Test read-write with Hardware transactional > > > memory\n"); > > > > > > - use_htm = 1; > > > - use_ext = 0; > > > + /* htm = 1, rw_lf = 0, ext = 0 */ > > > > I didn't like those local variables. > > But comments tend to get out of sync fairly easily, please remove too. > > > > > > > + if (test_hash_readwrite_functional(1, 0, 0) < 0) > > > + return -1; > > > > > > - if (test_hash_readwrite_functional(use_ext, use_htm) < 0) > > > + /* htm = 1, rw_lf = 1, ext = 0 */ > > > + if (test_hash_readwrite_functional(1, 1, 0) < 0) > > > return -1; > > > > > > - use_ext = 1; > > > - if (test_hash_readwrite_functional(use_ext, use_htm) < 0) > > > + /* htm = 1, rw_lf = 0, ext = 1 */ > > > + if (test_hash_readwrite_functional(1, 0, 1) < 0) > > > return -1; > > > > > > + /* htm = 1, rw_lf = 1, ext = 1 */ > > > + if (test_hash_readwrite_functional(1, 1, 1) < 0) > > > + return -1; > > > } else { > > > printf("Hardware transactional memory (lock elision) " > > > "is NOT supported\n"); > > > } > > > > > > printf("Test read-write without Hardware transactional memory\n"); > > > - use_htm = 0; > > > - use_ext = 0; > > > - if (test_hash_readwrite_functional(use_ext, use_htm) < 0) > > > + /* htm = 0, rw_lf = 0, ext = 0 */ > > > + if (test_hash_readwrite_functional(0, 0, 0) < 0) > > > + return -1; > > > + > > > + /* htm = 0, rw_lf = 1, ext = 0 */ > > > + if (test_hash_readwrite_functional(0, 1, 0) < 0) > > > + return -1; > > > + > > > + /* htm = 0, rw_lf = 0, ext = 1 */ > > > + if (test_hash_readwrite_functional(0, 0, 1) < 0) > > > return -1; > > > > > > - use_ext = 1; > > > - if (test_hash_readwrite_functional(use_ext, use_htm) < 0) > > > + /* htm = 0, rw_lf = 1, ext = 1 */ > > > + if (test_hash_readwrite_functional(0, 1, 1) < 0) > > > return -1; > The ordering of bits (0-0-0, 0-1-0, 0-0-1, 0-1-1) looks better here. Ok, forget my comment. I just want to get rid of this series and we stop getting random timeout in the CI. I will take it as is and cleanup if I find some time later. -- David Marchand
>-----Original Message----- >From: David Marchand [mailto:david.marchand@redhat.com] >Sent: Wednesday, February 5, 2020 8:42 AM >To: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com> >Cc: Amit Gupta <agupta3@marvell.com>; Wang, Yipeng1 <yipeng1.wang@intel.com>; Gobriel, Sameh <sameh.gobriel@intel.com>; >thomas@monjalon.net; dev <dev@dpdk.org>; nd <nd@arm.com> >Subject: Re: [PATCH v2 3/5] test/hash: add lock free reader writer functional tests > >On Wed, Feb 5, 2020 at 5:22 PM Honnappa Nagarahalli ><Honnappa.Nagarahalli@arm.com> wrote: >> >> > >> > On Mon, Feb 3, 2020 at 8:49 PM Honnappa Nagarahalli >> > <honnappa.nagarahalli@arm.com> wrote: >> > > >> > > Add lock-free reader writer concurrency functional tests. >> > > These tests will provide the same coverage that non lock-free APIs >> > > have. >> > > >> > > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> >> > > --- >> > > app/test/test_hash_readwrite.c | 58 >> > > +++++++++++++++++++++------------- >> > > 1 file changed, 36 insertions(+), 22 deletions(-) >> > > >> > > diff --git a/app/test/test_hash_readwrite.c >> > > b/app/test/test_hash_readwrite.c index 635ed5a9f..a9429091c 100644 >> > > --- a/app/test/test_hash_readwrite.c >> > > +++ b/app/test/test_hash_readwrite.c >> > > @@ -121,7 +121,7 @@ >> > test_hash_readwrite_worker(__attribute__((unused)) >> > > void *arg) } >> > > >> > > static int >> > > -init_params(int use_ext, int use_htm, int use_jhash) >> > > +init_params(int use_ext, int use_htm, int rw_lf, int use_jhash) >> > > { >> > > unsigned int i; >> > > >> > > @@ -140,15 +140,16 @@ init_params(int use_ext, int use_htm, int >> > use_jhash) >> > > else >> > > hash_params.hash_func = rte_hash_crc; >> > > >> > > + hash_params.extra_flag = >> > > + RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD; >> > > if (use_htm) >> > > - hash_params.extra_flag = >> > > - RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT | >> > > - RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY | >> > > - RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD; >> > > + hash_params.extra_flag |= >> > > + RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT; [Wang, Yipeng] Thanks for the patch Honnappa. Here I think we still need the RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY Flag even with HTM. Other commits in this series look good to me and seems David already applied. Thanks!
<snip> > >> > > > >> > > Add lock-free reader writer concurrency functional tests. > >> > > These tests will provide the same coverage that non lock-free > >> > > APIs have. > >> > > > >> > > Signed-off-by: Honnappa Nagarahalli > >> > > <honnappa.nagarahalli@arm.com> > >> > > --- > >> > > app/test/test_hash_readwrite.c | 58 > >> > > +++++++++++++++++++++------------- > >> > > 1 file changed, 36 insertions(+), 22 deletions(-) > >> > > > >> > > diff --git a/app/test/test_hash_readwrite.c > >> > > b/app/test/test_hash_readwrite.c index 635ed5a9f..a9429091c > >> > > 100644 > >> > > --- a/app/test/test_hash_readwrite.c > >> > > +++ b/app/test/test_hash_readwrite.c > >> > > @@ -121,7 +121,7 @@ > >> > test_hash_readwrite_worker(__attribute__((unused)) > >> > > void *arg) } > >> > > > >> > > static int > >> > > -init_params(int use_ext, int use_htm, int use_jhash) > >> > > +init_params(int use_ext, int use_htm, int rw_lf, int use_jhash) > >> > > { > >> > > unsigned int i; > >> > > > >> > > @@ -140,15 +140,16 @@ init_params(int use_ext, int use_htm, int > >> > use_jhash) > >> > > else > >> > > hash_params.hash_func = rte_hash_crc; > >> > > > >> > > + hash_params.extra_flag = > >> > > + RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD; > >> > > if (use_htm) > >> > > - hash_params.extra_flag = > >> > > - RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT | > >> > > - RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY | > >> > > - RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD; > >> > > + hash_params.extra_flag |= > >> > > + RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT; > > [Wang, Yipeng] Thanks for the patch Honnappa. Here I think we still need the > RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY > Flag even with HTM. I have made RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY depend on 'rw_lf' flag. The test case HTM + RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY will still run when 'rw_lf' is set to 0. > > Other commits in this series look good to me and seems David already > applied. > > Thanks!
>-----Original Message----- >From: Honnappa Nagarahalli [mailto:Honnappa.Nagarahalli@arm.com] >Sent: Wednesday, February 5, 2020 11:52 AM >To: Wang, Yipeng1 <yipeng1.wang@intel.com>; David Marchand <david.marchand@redhat.com> >Cc: Amit Gupta <agupta3@marvell.com>; Gobriel, Sameh <sameh.gobriel@intel.com>; thomas@monjalon.net; dev <dev@dpdk.org>; >nd <nd@arm.com>; Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; nd <nd@arm.com> >Subject: RE: [PATCH v2 3/5] test/hash: add lock free reader writer functional tests > ><snip> > >> >> > > >> >> > > Add lock-free reader writer concurrency functional tests. >> >> > > These tests will provide the same coverage that non lock-free >> >> > > APIs have. >> >> > > >> >> > > Signed-off-by: Honnappa Nagarahalli >> >> > > <honnappa.nagarahalli@arm.com> >> >> > > --- >> >> > > app/test/test_hash_readwrite.c | 58 >> >> > > +++++++++++++++++++++------------- >> >> > > 1 file changed, 36 insertions(+), 22 deletions(-) >> >> > > >> >> > > diff --git a/app/test/test_hash_readwrite.c >> >> > > b/app/test/test_hash_readwrite.c index 635ed5a9f..a9429091c >> >> > > 100644 >> >> > > --- a/app/test/test_hash_readwrite.c >> >> > > +++ b/app/test/test_hash_readwrite.c >> >> > > @@ -121,7 +121,7 @@ >> >> > test_hash_readwrite_worker(__attribute__((unused)) >> >> > > void *arg) } >> >> > > >> >> > > static int >> >> > > -init_params(int use_ext, int use_htm, int use_jhash) >> >> > > +init_params(int use_ext, int use_htm, int rw_lf, int use_jhash) >> >> > > { >> >> > > unsigned int i; >> >> > > >> >> > > @@ -140,15 +140,16 @@ init_params(int use_ext, int use_htm, int >> >> > use_jhash) >> >> > > else >> >> > > hash_params.hash_func = rte_hash_crc; >> >> > > >> >> > > + hash_params.extra_flag = >> >> > > + RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD; >> >> > > if (use_htm) >> >> > > - hash_params.extra_flag = >> >> > > - RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT | >> >> > > - RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY | >> >> > > - RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD; >> >> > > + hash_params.extra_flag |= >> >> > > + RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT; >> >> [Wang, Yipeng] Thanks for the patch Honnappa. Here I think we still need the >> RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY >> Flag even with HTM. >I have made RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY depend on 'rw_lf' flag. The test case HTM + >RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY will still run when 'rw_lf' is set to 0. > [Wang, Yipeng] I see, thought was an "else if". It is correct then, Thanks!
diff --git a/app/test/test_hash_readwrite.c b/app/test/test_hash_readwrite.c index 635ed5a9f..a9429091c 100644 --- a/app/test/test_hash_readwrite.c +++ b/app/test/test_hash_readwrite.c @@ -121,7 +121,7 @@ test_hash_readwrite_worker(__attribute__((unused)) void *arg) } static int -init_params(int use_ext, int use_htm, int use_jhash) +init_params(int use_ext, int use_htm, int rw_lf, int use_jhash) { unsigned int i; @@ -140,15 +140,16 @@ init_params(int use_ext, int use_htm, int use_jhash) else hash_params.hash_func = rte_hash_crc; + hash_params.extra_flag = RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD; if (use_htm) - hash_params.extra_flag = - RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT | - RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY | - RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD; + hash_params.extra_flag |= + RTE_HASH_EXTRA_FLAGS_TRANS_MEM_SUPPORT; + if (rw_lf) + hash_params.extra_flag |= + RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY_LF; else - hash_params.extra_flag = - RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY | - RTE_HASH_EXTRA_FLAGS_MULTI_WRITER_ADD; + hash_params.extra_flag |= + RTE_HASH_EXTRA_FLAGS_RW_CONCURRENCY; if (use_ext) hash_params.extra_flag |= @@ -195,7 +196,7 @@ init_params(int use_ext, int use_htm, int use_jhash) } static int -test_hash_readwrite_functional(int use_ext, int use_htm) +test_hash_readwrite_functional(int use_htm, int use_rw_lf, int use_ext) { unsigned int i; const void *next_key; @@ -214,7 +215,7 @@ test_hash_readwrite_functional(int use_ext, int use_htm) rte_atomic64_init(&ginsertions); rte_atomic64_clear(&ginsertions); - if (init_params(use_ext, use_htm, use_jhash) != 0) + if (init_params(use_ext, use_htm, use_rw_lf, use_jhash) != 0) goto err; if (use_ext) @@ -229,6 +230,8 @@ test_hash_readwrite_functional(int use_ext, int use_htm) tbl_rw_test_param.num_insert * slave_cnt; + printf("\nHTM = %d, RW-LF = %d, EXT-Table = %d\n", + use_htm, use_rw_lf, use_ext); printf("++++++++Start function tests:+++++++++\n"); /* Fire all threads. */ @@ -379,7 +382,7 @@ test_hash_readwrite_perf(struct perf *perf_results, int use_htm, rte_atomic64_init(&gwrite_cycles); rte_atomic64_clear(&gwrite_cycles); - if (init_params(0, use_htm, use_jhash) != 0) + if (init_params(0, use_htm, 0, use_jhash) != 0) goto err; /* @@ -700,7 +703,6 @@ test_hash_rw_func_main(void) * than writer threads. This is to timing either reader threads or * writer threads for performance numbers. */ - int use_htm, use_ext; unsigned int i = 0, core_id = 0; if (rte_lcore_count() < 3) { @@ -721,29 +723,41 @@ test_hash_rw_func_main(void) printf("Test read-write with Hardware transactional memory\n"); - use_htm = 1; - use_ext = 0; + /* htm = 1, rw_lf = 0, ext = 0 */ + if (test_hash_readwrite_functional(1, 0, 0) < 0) + return -1; - if (test_hash_readwrite_functional(use_ext, use_htm) < 0) + /* htm = 1, rw_lf = 1, ext = 0 */ + if (test_hash_readwrite_functional(1, 1, 0) < 0) return -1; - use_ext = 1; - if (test_hash_readwrite_functional(use_ext, use_htm) < 0) + /* htm = 1, rw_lf = 0, ext = 1 */ + if (test_hash_readwrite_functional(1, 0, 1) < 0) return -1; + /* htm = 1, rw_lf = 1, ext = 1 */ + if (test_hash_readwrite_functional(1, 1, 1) < 0) + return -1; } else { printf("Hardware transactional memory (lock elision) " "is NOT supported\n"); } printf("Test read-write without Hardware transactional memory\n"); - use_htm = 0; - use_ext = 0; - if (test_hash_readwrite_functional(use_ext, use_htm) < 0) + /* htm = 0, rw_lf = 0, ext = 0 */ + if (test_hash_readwrite_functional(0, 0, 0) < 0) + return -1; + + /* htm = 0, rw_lf = 1, ext = 0 */ + if (test_hash_readwrite_functional(0, 1, 0) < 0) + return -1; + + /* htm = 0, rw_lf = 0, ext = 1 */ + if (test_hash_readwrite_functional(0, 0, 1) < 0) return -1; - use_ext = 1; - if (test_hash_readwrite_functional(use_ext, use_htm) < 0) + /* htm = 0, rw_lf = 1, ext = 1 */ + if (test_hash_readwrite_functional(0, 1, 1) < 0) return -1; return 0;
Add lock-free reader writer concurrency functional tests. These tests will provide the same coverage that non lock-free APIs have. Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com> --- app/test/test_hash_readwrite.c | 58 +++++++++++++++++++++------------- 1 file changed, 36 insertions(+), 22 deletions(-) -- 2.17.1