Re: [dpdk-dev] [PATCH v7 03/17] test/ring: add functional tests for rte_ring_xxx_elem APIs

[Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>]

<snip>

> Subject: RE: [PATCH v7 03/17] test/ring: add functional tests for
> rte_ring_xxx_elem APIs
> 
> > > > Add basic infrastructure to test rte_ring_xxx_elem APIs. Add test
> > > > cases for testing burst and bulk tests.
> > > >
> > > > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli@arm.com>
> > > > Reviewed-by: Gavin Hu <gavin.hu@arm.com>
> > > > ---
<snip>

> > > > diff --git a/app/test/test_ring.h b/app/test/test_ring.h new file
> > > > mode
> > > > 100644 index 000000000..19ef1b399
> > > > --- /dev/null
> > > > +++ b/app/test/test_ring.h
> > > > @@ -0,0 +1,203 @@
> > > > +/* SPDX-License-Identifier: BSD-3-Clause
> > > > + * Copyright(c) 2019 Arm Limited
> > > > + */
> > > > +
> > > > +#include <rte_malloc.h>
> > > > +#include <rte_ring.h>
> > > > +#include <rte_ring_elem.h>
> > > > +
> > > > +/* API type to call
> > > > + * N - Calls default APIs
> > > > + * S - Calls SP or SC API
> > > > + * M - Calls MP or MC API
> > > > + */
> > > > +#define TEST_RING_N 1
> > > > +#define TEST_RING_S 2
> > > > +#define TEST_RING_M 4
> > > > +
> > > > +/* API type to call
> > > > + * SL - Calls single element APIs
> > > > + * BL - Calls bulk APIs
> > > > + * BR - Calls burst APIs
> > > > + */
> > > > +#define TEST_RING_SL 8
> > > > +#define TEST_RING_BL 16
> > > > +#define TEST_RING_BR 32
> > > > +
> > > > +#define TEST_RING_IGNORE_API_TYPE ~0U
> > > > +
> > > > +#define TEST_RING_INCP(obj, esize, n) do { \
> > > > +	/* Legacy queue APIs? */ \
> > > > +	if ((esize) == -1) \
> > > > +		obj = ((void **)obj) + n; \
> > > > +	else \
> > > > +		obj = (void **)(((uint32_t *)obj) + \
> > > > +					(n * esize / sizeof(uint32_t))); \ }
> > > while (0)
> > > > +
> > > > +#define TEST_RING_CREATE(name, esize, count, socket_id, flags, r) do
> { \
> > > > +	/* Legacy queue APIs? */ \
> > > > +	if ((esize) == -1) \
> > > > +		r = rte_ring_create((name), (count), (socket_id), (flags)); \
> > > > +	else \
> > > > +		r = rte_ring_create_elem((name), (esize), (count), \
> > > > +						(socket_id), (flags)); \
> > > > +} while (0)
> > > > +
> > > > +#define TEST_RING_ENQUEUE(r, obj, esize, n, ret, api_type) do { \
> > > > +	/* Legacy queue APIs? */ \
> > > > +	if ((esize) == -1) \
> > > > +		switch (api_type) { \
> > > > +		case (TEST_RING_N | TEST_RING_SL): \
> > > > +			ret = rte_ring_enqueue(r, obj); \
> > > > +			break; \
> > > > +		case (TEST_RING_S | TEST_RING_SL): \
> > > > +			ret = rte_ring_sp_enqueue(r, obj); \
> > > > +			break; \
> > > > +		case (TEST_RING_M | TEST_RING_SL): \
> > > > +			ret = rte_ring_mp_enqueue(r, obj); \
> > > > +			break; \
> > > > +		case (TEST_RING_N | TEST_RING_BL): \
> > > > +			ret = rte_ring_enqueue_bulk(r, obj, n, NULL); \
> > > > +			break; \
> > > > +		case (TEST_RING_S | TEST_RING_BL): \
> > > > +			ret = rte_ring_sp_enqueue_bulk(r, obj, n, NULL); \
> > > > +			break; \
> > > > +		case (TEST_RING_M | TEST_RING_BL): \
> > > > +			ret = rte_ring_mp_enqueue_bulk(r, obj, n, NULL); \
> > > > +			break; \
> > > > +		case (TEST_RING_N | TEST_RING_BR): \
> > > > +			ret = rte_ring_enqueue_burst(r, obj, n, NULL); \
> > > > +			break; \
> > > > +		case (TEST_RING_S | TEST_RING_BR): \
> > > > +			ret = rte_ring_sp_enqueue_burst(r, obj, n, NULL); \
> > > > +			break; \
> > > > +		case (TEST_RING_M | TEST_RING_BR): \
> > > > +			ret = rte_ring_mp_enqueue_burst(r, obj, n, NULL); \
> > > > +		} \
> > > > +	else \
> > > > +		switch (api_type) { \
> > > > +		case (TEST_RING_N | TEST_RING_SL): \
> > > > +			ret = rte_ring_enqueue_elem(r, obj, esize); \
> > > > +			break; \
> > > > +		case (TEST_RING_S | TEST_RING_SL): \
> > > > +			ret = rte_ring_sp_enqueue_elem(r, obj, esize); \
> > > > +			break; \
> > > > +		case (TEST_RING_M | TEST_RING_SL): \
> > > > +			ret = rte_ring_mp_enqueue_elem(r, obj, esize); \
> > > > +			break; \
> > > > +		case (TEST_RING_N | TEST_RING_BL): \
> > > > +			ret = rte_ring_enqueue_bulk_elem(r, obj, esize, n, \
> > > > +								NULL); \
> > > > +			break; \
> > > > +		case (TEST_RING_S | TEST_RING_BL): \
> > > > +			ret = rte_ring_sp_enqueue_bulk_elem(r, obj, esize, n,
> > > \
> > > > +								NULL); \
> > > > +			break; \
> > > > +		case (TEST_RING_M | TEST_RING_BL): \
> > > > +			ret = rte_ring_mp_enqueue_bulk_elem(r, obj, esize, n,
> > > \
> > > > +								NULL); \
> > > > +			break; \
> > > > +		case (TEST_RING_N | TEST_RING_BR): \
> > > > +			ret = rte_ring_enqueue_burst_elem(r, obj, esize, n, \
> > > > +								NULL); \
> > > > +			break; \
> > > > +		case (TEST_RING_S | TEST_RING_BR): \
> > > > +			ret = rte_ring_sp_enqueue_burst_elem(r, obj, esize, n,
> > > \
> > > > +								NULL); \
> > > > +			break; \
> > > > +		case (TEST_RING_M | TEST_RING_BR): \
> > > > +			ret = rte_ring_mp_enqueue_burst_elem(r, obj, esize,
> > > n, \
> > > > +								NULL); \
> > > > +		} \
> > > > +} while (0)
> > > > +
> > > > +#define TEST_RING_DEQUEUE(r, obj, esize, n, ret, api_type) do { \
> > > > +	/* Legacy queue APIs? */ \
> > > > +	if ((esize) == -1) \
> > > > +		switch (api_type) { \
> > > > +		case (TEST_RING_N | TEST_RING_SL): \
> > > > +			ret = rte_ring_dequeue(r, obj); \
> > > > +			break; \
> > > > +		case (TEST_RING_S | TEST_RING_SL): \
> > > > +			ret = rte_ring_sc_dequeue(r, obj); \
> > > > +			break; \
> > > > +		case (TEST_RING_M | TEST_RING_SL): \
> > > > +			ret = rte_ring_mc_dequeue(r, obj); \
> > > > +			break; \
> > > > +		case (TEST_RING_N | TEST_RING_BL): \
> > > > +			ret = rte_ring_dequeue_bulk(r, obj, n, NULL); \
> > > > +			break; \
> > > > +		case (TEST_RING_S | TEST_RING_BL): \
> > > > +			ret = rte_ring_sc_dequeue_bulk(r, obj, n, NULL); \
> > > > +			break; \
> > > > +		case (TEST_RING_M | TEST_RING_BL): \
> > > > +			ret = rte_ring_mc_dequeue_bulk(r, obj, n, NULL); \
> > > > +			break; \
> > > > +		case (TEST_RING_N | TEST_RING_BR): \
> > > > +			ret = rte_ring_dequeue_burst(r, obj, n, NULL); \
> > > > +			break; \
> > > > +		case (TEST_RING_S | TEST_RING_BR): \
> > > > +			ret = rte_ring_sc_dequeue_burst(r, obj, n, NULL); \
> > > > +			break; \
> > > > +		case (TEST_RING_M | TEST_RING_BR): \
> > > > +			ret = rte_ring_mc_dequeue_burst(r, obj, n, NULL); \
> > > > +		} \
> > > > +	else \
> > > > +		switch (api_type) { \
> > > > +		case (TEST_RING_N | TEST_RING_SL): \
> > > > +			ret = rte_ring_dequeue_elem(r, obj, esize); \
> > > > +			break; \
> > > > +		case (TEST_RING_S | TEST_RING_SL): \
> > > > +			ret = rte_ring_sc_dequeue_elem(r, obj, esize); \
> > > > +			break; \
> > > > +		case (TEST_RING_M | TEST_RING_SL): \
> > > > +			ret = rte_ring_mc_dequeue_elem(r, obj, esize); \
> > > > +			break; \
> > > > +		case (TEST_RING_N | TEST_RING_BL): \
> > > > +			ret = rte_ring_dequeue_bulk_elem(r, obj, esize, n, \
> > > > +								NULL); \
> > > > +			break; \
> > > > +		case (TEST_RING_S | TEST_RING_BL): \
> > > > +			ret = rte_ring_sc_dequeue_bulk_elem(r, obj, esize, n,
> > > \
> > > > +								NULL); \
> > > > +			break; \
> > > > +		case (TEST_RING_M | TEST_RING_BL): \
> > > > +			ret = rte_ring_mc_dequeue_bulk_elem(r, obj, esize, n,
> > > \
> > > > +								NULL); \
> > > > +			break; \
> > > > +		case (TEST_RING_N | TEST_RING_BR): \
> > > > +			ret = rte_ring_dequeue_burst_elem(r, obj, esize, n, \
> > > > +								NULL); \
> > > > +			break; \
> > > > +		case (TEST_RING_S | TEST_RING_BR): \
> > > > +			ret = rte_ring_sc_dequeue_burst_elem(r, obj, esize, n,
> > > \
> > > > +								NULL); \
> > > > +			break; \
> > > > +		case (TEST_RING_M | TEST_RING_BR): \
> > > > +			ret = rte_ring_mc_dequeue_burst_elem(r, obj, esize,
> > > n, \
> > > > +								NULL); \
> > > > +		} \
> > > > +} while (0)
> > >
> > >
> > > My thought to avoid test-code duplication was a bit different.
> > Yes, this can be done multiple ways. My implementation is not complicated
> either.
> >
> > > Instead of adding extra enums/parameters and then do switch on them,
> > > my
> > The switch statement should be removed by the compiler for the
> performance tests.
> 
> I am sure the compiler will do its job properly.
> My concern is that with all these extra flags, it is really hard to read and
> understand what exactly function we are calling and what we are trying to
> test.
There are just 2 flags - 1) representing single/bulk/burst 2) representing default/single/multiple threads. This is the way the rte_ring APIs are also organized (rte_ring_<sp/mp or sc/mc>_enqueue_<bulk/burst>).
If we want to keep the code flexible, we have to keep these 2 flags that can be varied.
Your proposal considers only the element size as a variable. It does not consider the above mentioned variables. This results in code duplication. This is visible in patch 10/17.

> Might be just me, but let say in original version for enqueue_bulk() we have:
> 
>         const uint64_t sp_start = rte_rdtsc();
>         for (i = 0; i < iterations; i++)
>                 while (rte_ring_sp_enqueue_bulk(r, burst, size, NULL) == 0)
>                         rte_pause();
>         const uint64_t sp_end = rte_rdtsc();
> 
>         const uint64_t mp_start = rte_rdtsc();
>         for (i = 0; i < iterations; i++)
>                 while (rte_ring_mp_enqueue_bulk(r, burst, size, NULL) == 0)
>                         rte_pause();
>         const uint64_t mp_end = rte_rdtsc();
> 
> Simple and easy to understand.
> Same code after the patch doesn't that straightforward anymore:
> 
>  const uint64_t sp_start = rte_rdtsc();
>         for (i = 0; i < iterations; i++)
>                 do {
>                         if (flag == 0)
>                                 TEST_RING_ENQUEUE(r, burst, esize, bsize, ret,
>                                                 TEST_RING_S | TEST_RING_BL);
>                         else if (flag == 1)
>                                 TEST_RING_DEQUEUE(r, burst, esize, bsize, ret,
>                                                 TEST_RING_S | TEST_RING_BL);
Would it help if the #define names are better?
May be convert

TEST_RING_SL to TEST_ELEM_SINGLE
TEST_RING_BL to TEST_ELEM_BULK
TEST_RING_BR to TEST_ELEM_BURST

and

TEST_RING_N to TEST_THREAD_DEFAULT
TEST_RING_S to TEST_THREAD_SPSC
TEST_RING_M to TEST_THREAD_MPMC

>                         if (ret == 0)
>                                 rte_pause();
>                 } while (!ret);
>  const uint64_t sp_end = rte_rdtsc();
> 
> Another thing - if tomorrow we'll want to add perf tests for elem_size==4/8,
> etc. - we'll need to do copy/paste for all test-case invocations, as you did for
> 16B (or some code reorg).
This is a mistake on my side. Looking at the code, 'test_ring_perf' can be simplified to avoid the copy/paste. 'test_ring_perf' can be changed to call another function (that contains the test cases) with different element sizes. I will make this change.
The only issue would be the wrappers 'dequeue_bulk', 'dequeue_bulk_16B' etc. However, the wrappers are simple enough to maintain.

> 
> >
> > > intention was something like that:
> > >
> > > 1. mv  test_ring_perf.c test_ring_perf.h 2. Inside test_ring_perf.h
> > > change rte_ring_ create/enqueue/dequeue function
> > >     calls to some not-defined function/macros invocations.
> > >    With similar name, same number of parameters, and same semantics.
> > >    Also change 'void *burst[..]' to 'RING_ELEM[...]'; 3. For each
> > > test configuration we want to have (default, 4B, 8B, 16B)
> > >     create a new .c file where we:
> > >     - define used in test_ring_perf.h macros(/function)
> > >    - include test_ring_perf.h
> > >    -  REGISTER_TEST_COMMAND(<test_name>, test_ring_perf);
> > >
> > > As an example:
> > > test_ring_perf.h:
> > > ...
> > > static int
> > > enqueue_bulk(void *p)
> > > {
> > >         ...
> > >         RING_ELEM burst[MAX_BURST];
> > >
> > >         memset(burst, 0, sizeof(burst));
> > >         ....
> > >         const uint64_t sp_start = rte_rdtsc();
> > >         for (i = 0; i < iterations; i++)
> > >                 while (RING_SP_ENQUEUE_BULK(r, burst, size, NULL) == 0)
> > >                         rte_pause();
> > >         const uint64_t sp_end = rte_rdtsc();
> > >
> > >         const uint64_t mp_start = rte_rdtsc();
> > >         for (i = 0; i < iterations; i++)
> > >                 while (RING_MP_ENQUEUE_BULK(r, burst, size, NULL) == 0)
> > >                         rte_pause();
> > >         const uint64_t mp_end = rte_rdtsc();
> > >         ....
> > >
> > > Then in test_ring_perf.c:
> > >
> > > ....
> > > #define RING_ELEM	void *
> > > ...
> > > #define RING_SP_ENQUEUE_BULK(ring, buf, size, spc)  \
> > >        rte_ring_sp_enqueue_bulk(ring, buf, size, spc) ....
> > >
> > > #include "test_ring_perf.h"
> > > REGISTER_TEST_COMMAND(ring_perf_autotest, test_ring_perf);
> > >
> > >
> > > In test_ring_elem16B_perf.c:
> > > ....
> > > #define RING_ELEM	__uint128_t
> > > #define RING_SP_ENQUEUE_BULK(ring, buf, size, spc)  \
> > > 	rte_ring_sp_enqueue_bulk_elem(ring, buf, sizeof(RING_ELEM), size,
> > > spc) ....
> > > #include "test_ring_perf.h"
> > > REGISTER_TEST_COMMAND(ring_perf_elem16B_autotest, test_ring_perf);
> > >
> > > In test_ring_elem4B_per.c:
> > >
> > > ....
> > > #define RING_ELEM	uint32_t
> > > #define RING_SP_ENQUEUE_BULK(ring, buf, size, spc)  \
> > > 	rte_ring_sp_enqueue_bulk_elem(ring, buf, sizeof(RING_ELEM), size,
> > > spc) ....
> > > #include "test_ring_perf.h"
> > > REGISTER_TEST_COMMAND(ring_perf_elem4B_autotest, test_ring_perf);
> > >
> > > And so on.
This will result in additional test files.

> > >
> > > > +
> > > > +/* This function is placed here as it is required for both
> > > > + * performance and functional tests.
> > > > + */
> > > > +static __rte_always_inline void * test_ring_calloc(unsigned int
> > > > +rsize, int esize) {
> > > > +	unsigned int sz;
> > > > +	void *p;
> > > > +
> > > > +	/* Legacy queue APIs? */
> > > > +	if (esize == -1)
> > > > +		sz = sizeof(void *);
> > > > +	else
> > > > +		sz = esize;
> > > > +
> > > > +	p = rte_zmalloc(NULL, rsize * sz, RTE_CACHE_LINE_SIZE);
> > > > +	if (p == NULL)
> > > > +		printf("Failed to allocate memory\n");
> > > > +
> > > > +	return p;
> > > > +}
> > > > --
> > > > 2.17.1