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

[Olivier Matz <olivier.matz@6wind.com>]

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@arm.com>
> Reviewed-by: Gavin Hu <gavin.hu@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.


> @@ -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

[...]

> +/*
> + * 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.

[...]

> 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.