[dpdk-dev] [PATCH v9 3/6] test/ring: add functional tests for rte_ring_xxx_elem APIs

[Honnappa Nagarahalli]

<snip>

> On Wed, Jan 15, 2020 at 11:25:08PM -0600, Honnappa Nagarahalli wrote:
> > Add basic infrastructure to test rte_ring_xxx_elem APIs.
> > Adjust the existing test cases to test for various ring element sizes.
> >
> > Signed-off-by: Honnappa Nagarahalli <honnappa.nagarahalli at arm.com>
> > Reviewed-by: Gavin Hu <gavin.hu at arm.com>
> > ---
> >  app/test/test_ring.c | 1342
> > +++++++++++++++++++++---------------------
> >  app/test/test_ring.h |  187 ++++++
> >  2 files changed, 850 insertions(+), 679 deletions(-)  create mode
> > 100644 app/test/test_ring.h
> >
> > diff --git a/app/test/test_ring.c b/app/test/test_ring.c index
> > aaf1e70ad..c08500eca 100644
> > --- a/app/test/test_ring.c
> > +++ b/app/test/test_ring.c
> > @@ -23,11 +23,13 @@
> >  #include <rte_branch_prediction.h>
> >  #include <rte_malloc.h>
> >  #include <rte_ring.h>
> > +#include <rte_ring_elem.h>
> >  #include <rte_random.h>
> >  #include <rte_errno.h>
> >  #include <rte_hexdump.h>
> >
> >  #include "test.h"
> > +#include "test_ring.h"
> >
> >  /*
> >   * Ring
> 
> As you are changing a lot of things, maybe it's an opportunity to update or
> remove the comment at the beginning of the file.
I have removed specific comments. I have converted it into generic comments.

> 
> 
> > @@ -55,8 +57,6 @@
> >  #define RING_SIZE 4096
> >  #define MAX_BULK 32
> >
> > -static rte_atomic32_t synchro;
> > -
> >  #define	TEST_RING_VERIFY(exp)
> 		\
> >  	if (!(exp)) {							\
> >  		printf("error at %s:%d\tcondition " #exp " failed\n",	\
> > @@ -67,808 +67,792 @@ static rte_atomic32_t synchro;
> >
> >  #define	TEST_RING_FULL_EMTPY_ITER	8
> >
> > -/*
> > - * helper routine for test_ring_basic
> > - */
> > -static int
> > -test_ring_basic_full_empty(struct rte_ring *r, void * const src[],
> > void *dst[])
> > +static int esize[] = {-1, 4, 8, 16, 20};
> 
> it could be const
Yes

> 
> [...]
> 
> > +/*
> > + * Burst and bulk operations with sp/sc, mp/mc and default (during
> > +creation)
> > + * Random number of elements are enqueued and dequeued.
> > + */
> > +static int
> > +test_ring_burst_bulk_tests1(unsigned int api_type) {
> > +	struct rte_ring *r;
> > +	void **src = NULL, **cur_src = NULL, **dst = NULL, **cur_dst = NULL;
> > +	int ret;
> > +	unsigned int i, j;
> > +	int rand;
> > +	const unsigned int rsz = RING_SIZE - 1;
> >
> > -	/* check data */
> > -	if (memcmp(src, dst, cur_dst - dst)) {
> > -		rte_hexdump(stdout, "src", src, cur_src - src);
> > -		rte_hexdump(stdout, "dst", dst, cur_dst - dst);
> > -		printf("data after dequeue is not the same\n");
> > -		goto fail;
> > -	}
> > +	for (i = 0; i < RTE_DIM(esize); i++) {
> > +		test_ring_print_test_string("Test standard ring", api_type,
> > +						esize[i]);
> >
> > -	cur_src = src;
> > -	cur_dst = dst;
> > +		/* Create the ring */
> > +		r = test_ring_create("test_ring_burst_bulk_tests", esize[i],
> > +					RING_SIZE, SOCKET_ID_ANY, 0);
> >
> > -	ret = rte_ring_mp_enqueue(r, cur_src);
> > -	if (ret != 0)
> > -		goto fail;
> > +		/* alloc dummy object pointers */
> > +		src = test_ring_calloc(RING_SIZE * 2, esize[i]);
> > +		if (src == NULL)
> > +			goto fail;
> > +		test_ring_mem_init(src, RING_SIZE * 2, esize[i]);
> > +		cur_src = src;
> >
> > -	ret = rte_ring_mc_dequeue(r, cur_dst);
> > -	if (ret != 0)
> > -		goto fail;
> > +		/* alloc some room for copied objects */
> > +		dst = test_ring_calloc(RING_SIZE * 2, esize[i]);
> > +		if (dst == NULL)
> > +			goto fail;
> > +		cur_dst = dst;
> > +
> > +		printf("Random full/empty test\n");
> > +
> > +		for (j = 0; j != TEST_RING_FULL_EMTPY_ITER; j++) {
> > +			/* random shift in the ring */
> > +			rand = RTE_MAX(rte_rand() % RING_SIZE, 1UL);
> > +			printf("%s: iteration %u, random shift: %u;\n",
> > +			    __func__, i, rand);
> > +			ret = test_ring_enqueue(r, cur_src, esize[i], rand,
> > +							api_type);
> > +			TEST_RING_VERIFY(ret != 0);
> > +
> > +			ret = test_ring_dequeue(r, cur_dst, esize[i], rand,
> > +							api_type);
> > +			TEST_RING_VERIFY(ret == rand);
> > +
> > +			/* fill the ring */
> > +			ret = test_ring_enqueue(r, cur_src, esize[i], rsz,
> > +							api_type);
> > +			TEST_RING_VERIFY(ret != 0);
> > +
> > +			TEST_RING_VERIFY(rte_ring_free_count(r) == 0);
> > +			TEST_RING_VERIFY(rsz == rte_ring_count(r));
> > +			TEST_RING_VERIFY(rte_ring_full(r));
> > +			TEST_RING_VERIFY(rte_ring_empty(r) == 0);
> > +
> > +			/* empty the ring */
> > +			ret = test_ring_dequeue(r, cur_dst, esize[i], rsz,
> > +							api_type);
> > +			TEST_RING_VERIFY(ret == (int)rsz);
> > +			TEST_RING_VERIFY(rsz == rte_ring_free_count(r));
> > +			TEST_RING_VERIFY(rte_ring_count(r) == 0);
> > +			TEST_RING_VERIFY(rte_ring_full(r) == 0);
> > +			TEST_RING_VERIFY(rte_ring_empty(r));
> > +
> > +			/* check data */
> > +			TEST_RING_VERIFY(memcmp(src, dst, rsz) == 0);
> > +		}
> > +
> > +		/* Free memory before test completed */
> > +		rte_ring_free(r);
> > +		rte_free(src);
> > +		rte_free(dst);
> 
> I think they should be reset to NULL to avoid a double free if next iteration
> fails.
> 
> There are several places like this, I think it can be done even if not really
> needed.
Will change all of them

> 
> [...]
> 
> > diff --git a/app/test/test_ring.h b/app/test/test_ring.h new file mode
> > 100644 index 000000000..26716e4f8
> > --- /dev/null
> > +++ b/app/test/test_ring.h
> > @@ -0,0 +1,187 @@
> > +/* 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
> > + * rte_ring_<sp/mp or sc/mc>_enqueue_<bulk/burst>
> > + * TEST_RING_THREAD_DEF - Uses configured SPSC/MPMC calls
> > + * TEST_RING_THREAD_SPSC - Calls SP or SC API
> > + * TEST_RING_THREAD_MPMC - Calls MP or MC API  */ #define
> > +TEST_RING_THREAD_DEF 1 #define TEST_RING_THREAD_SPSC 2 #define
> > +TEST_RING_THREAD_MPMC 4
> > +
> > +/* API type to call
> > + * SL - Calls single element APIs
> > + * BL - Calls bulk APIs
> > + * BR - Calls burst APIs
> > + */
> 
> The comment was not updated according to macro name.
Will fix

>