Re: [dpdk-dev] [PATCH v14 9/9] Add unit tests for thread API

[Dmitry Kozlyuk <dmitry.kozliuk@gmail.com>]

2021-08-19 14:31 (UTC-0700), Narcisa Ana Maria Vasile:
> From: Narcisa Vasile <navasile@microsoft.com>
> 
> As a new API for threading is introduced,
> a set of unit tests have been added to test the new interface.

There are three substantial issues with this tests:

1. They use pthread API to verify the results, but the whole point of
introducing this new API is to abstract pthread or Win32 interfaces
and eventually to remove pthread shim from Windows EAL. Furthermore, new new
API actually has functions to do the checks, e.g. rte_thread_get_affinity().

2. They don't really check the contracts:

2.1. Mutex test could pass even if it used no locking at all.
2.2. Barrier test does not verify that threads are blocked/unblocked.
2.3. No test for static initialization.

See also comments inline.

[...]
> +#define TEST_THREADS_LOG(func) \
> +		printf("Error at line %d. %s failed!\n", __LINE__, func)

rte_test.h contains some useful macros like this one.

[...]
> +static int
> +test_thread_self(void)
> +{
> +	rte_thread_t threads_ids[THREADS_COUNT];
> +	rte_thread_t self_ids[THREADS_COUNT] = {};
> +	size_t i;
> +	size_t j;
> +	int ret = 0;
> +
> +	for (i = 0; i < THREADS_COUNT; ++i) {
> +		if (rte_thread_create(&threads_ids[i], NULL, thread_loop_self,
> +				&self_ids[i]) != 0) {
> +			printf("Error, Only %zu threads created.\n", i);

Typo: "only" (in other copies too).
This test could use RTE_LOG_REGISTER() and log levels if needed.

> +			break;
> +		}
> +	}
> +
> +	for (j = 0; j < i; ++j) {
> +		ret = rte_thread_join(threads_ids[j], NULL);
> +		if (ret != 0) {
> +			TEST_THREADS_LOG("rte_thread_join()");
> +			return -1;
> +		}
> +
> +		if (rte_thread_equal(threads_ids[j], self_ids[j]) == 0)
> +			ret = -1;

Tests should indicate what exactly went wrong.
Suggesting RTE_TEST_ASSERT_EQUAL() here.

> +	}
> +
> +	return ret;
> +}
> +
> +struct thread_context {
> +	rte_thread_barrier *barrier;
> +	size_t *thread_count;
> +};
> +
> +static void *
> +thread_loop_barrier(void *arg)
> +{
> +

Unnecessary empty line.

> +	struct thread_context *ctx = arg;
> +
> +	(void)__atomic_add_fetch(ctx->thread_count, 1, __ATOMIC_RELAXED);
> +
> +	if (rte_thread_barrier_wait(ctx->barrier) > 0)
> +		TEST_THREADS_LOG("rte_thread_barrier_wait()");
> +
> +	return NULL;
> +}
[...]