diff mbox series

[v2,3/5] test/hash: add lock free reader writer functional tests

Message ID 20200203194912.4669-4-honnappa.nagarahalli@arm.com
State New
Headers show
Series test/meson: fix hash readwrite timeout failure | expand

Commit Message

Honnappa Nagarahalli Feb. 3, 2020, 7:49 p.m. UTC 
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

Comments

David Marchand Feb. 5, 2020, 9:07 a.m. UTC | #1 
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
Honnappa Nagarahalli Feb. 5, 2020, 4:22 p.m. UTC | #2 
> 

> 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
David Marchand Feb. 5, 2020, 4:41 p.m. UTC | #3 
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
Wang, Yipeng1 Feb. 5, 2020, 7:34 p.m. UTC | #4 
>-----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!
Honnappa Nagarahalli Feb. 5, 2020, 7:52 p.m. UTC | #5 
<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!
Wang, Yipeng1 Feb. 5, 2020, 7:57 p.m. UTC | #6 
>-----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 mbox series

Patch

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;