diff mbox series

[v14,9/9] Add unit tests for thread API

Message ID 1629408694-31803-10-git-send-email-navasile@linux.microsoft.com (mailing list archive)
State Superseded
Delegated to: David Marchand
Headers show
Series eal: Add EAL API for threading | expand

Checks

Context Check Description
ci/intel-Testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-aarch64-compile-testing success Testing PASS
ci/iol-x86_64-compile-testing fail Testing issues
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/github-robot: build success github build: passed
ci/checkpatch success coding style OK

Commit Message

Narcisa Ana Maria Vasile Aug. 19, 2021, 9:31 p.m. UTC
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.

Signed-off-by: Narcisa Vasile <navasile@microsoft.com>
---
 app/test/meson.build    |   2 +
 app/test/test_threads.c | 419 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 421 insertions(+)
 create mode 100644 app/test/test_threads.c

Comments

Narcisa Ana Maria Vasile Aug. 20, 2021, 4:10 p.m. UTC | #1
On Thu, Aug 19, 2021 at 02:31:34PM -0700, Narcisa Ana Maria Vasile wrote:
> 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.
> 
> Signed-off-by: Narcisa Vasile <navasile@microsoft.com>
> ---
>  app/test/meson.build    |   2 +
>  app/test/test_threads.c | 419 ++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 421 insertions(+)
>  create mode 100644 app/test/test_threads.c
> 

There's a failure here on Alpine Linux:
"error: implicit declaration of function 'pthread_attr_setaffinity_np';
did you mean 'pthread_setaffinity_np'? [-Werror=implicit-function-declaration]"

It looks like "pthread_attr_setaffinity_np" is not available on Alpine Linux. However,
other affinity functions such as "pthread_setaffinity_np" are present. Is there a guard that
I can use here to check if the pthread_*_np functions are available, similar to RTE_HAS_CPUSET for cpuset?
Dmitry Kozlyuk Aug. 20, 2021, 4:54 p.m. UTC | #2
2021-08-20 09:10 (UTC-0700), Narcisa Ana Maria Vasile:
> On Thu, Aug 19, 2021 at 02:31:34PM -0700, Narcisa Ana Maria Vasile wrote:
> > 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.
> > 
> > Signed-off-by: Narcisa Vasile <navasile@microsoft.com>
> > ---
> >  app/test/meson.build    |   2 +
> >  app/test/test_threads.c | 419 ++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 421 insertions(+)
> >  create mode 100644 app/test/test_threads.c
> >   
> 
> There's a failure here on Alpine Linux:
> "error: implicit declaration of function 'pthread_attr_setaffinity_np';
> did you mean 'pthread_setaffinity_np'? [-Werror=implicit-function-declaration]"
> 
> It looks like "pthread_attr_setaffinity_np" is not available on Alpine Linux. However,
> other affinity functions such as "pthread_setaffinity_np" are present. Is there a guard that
> I can use here to check if the pthread_*_np functions are available, similar to RTE_HAS_CPUSET for cpuset?

Even if there is one, you still need to handle the Alpine case.
41b5a7a8494e ("vdpa/mlx5: replace pthread functions unavailable in musl")
is an example how to solve this particular case.
It's also what that rte_ctrl_thread_create() does.
Dmitry Kozlyuk Aug. 23, 2021, 8:25 p.m. UTC | #3
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;
> +}
[...]
diff mbox series

Patch

diff --git a/app/test/meson.build b/app/test/meson.build
index a7611686ad..57e61ce601 100644
--- a/app/test/meson.build
+++ b/app/test/meson.build
@@ -140,6 +140,7 @@  test_sources = files(
         'test_table_tables.c',
         'test_tailq.c',
         'test_thash.c',
+        'test_threads.c',
         'test_timer.c',
         'test_timer_perf.c',
         'test_timer_racecond.c',
@@ -276,6 +277,7 @@  fast_tests = [
         ['reorder_autotest', true],
         ['service_autotest', true],
         ['thash_autotest', true],
+        ['threads_autotest', true],
         ['trace_autotest', true],
 ]
 
diff --git a/app/test/test_threads.c b/app/test/test_threads.c
new file mode 100644
index 0000000000..beaa303506
--- /dev/null
+++ b/app/test/test_threads.c
@@ -0,0 +1,419 @@ 
+/* SPDX-License-Identifier: BSD-3-Clause
+ * Copyright (c) 2021 Microsoft.
+ */
+
+#include <pthread.h>
+
+#include <rte_thread.h>
+
+#include "test.h"
+
+#define THREADS_COUNT 20
+
+#define TEST_THREADS_LOG(func) \
+		printf("Error at line %d. %s failed!\n", __LINE__, func)
+
+static void *
+thread_loop_self(void *arg)
+{
+	rte_thread_t *id = arg;
+
+	*id = rte_thread_self();
+
+	return NULL;
+}
+
+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);
+			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;
+	}
+
+	return ret;
+}
+
+struct thread_context {
+	rte_thread_barrier *barrier;
+	size_t *thread_count;
+};
+
+static void *
+thread_loop_barrier(void *arg)
+{
+
+	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;
+}
+
+static int
+test_thread_barrier(void)
+{
+	rte_thread_t threads_ids[THREADS_COUNT];
+	struct thread_context ctx[THREADS_COUNT] = {};
+	rte_thread_barrier barrier;
+	size_t count = 0;
+	size_t i;
+	size_t j;
+	int ret = 0;
+
+	ret = rte_thread_barrier_init(&barrier, THREADS_COUNT + 1);
+	if (ret != 0) {
+		TEST_THREADS_LOG("rte_thread_barrier_init()");
+		return -1;
+	}
+
+	for (i = 0; i < THREADS_COUNT; ++i) {
+		ctx[i].thread_count = &count;
+		ctx[i].barrier = &barrier;
+		if (rte_thread_create(&threads_ids[i], NULL,
+				thread_loop_barrier, &ctx[i]) != 0) {
+			printf("Error, Only %zu threads created.\n", i);
+			ret = -1;
+			goto error;
+		}
+	}
+
+	ret = rte_thread_barrier_wait(ctx->barrier);
+	if (ret > 0) {
+		TEST_THREADS_LOG("rte_thread_barrier_wait()");
+		ret = -1;
+		goto error;
+	}
+
+	if (count != i) {
+		ret = -1;
+		printf("Error, expected thread count(%zu) to be equal "
+			"to the number of threads that wait at the barrier(%zu)\n",
+			count, i);
+		goto error;
+	}
+
+error:
+	for (j = 0; j < i; ++j) {
+		ret = rte_thread_join(threads_ids[j], NULL);
+		if (ret != 0) {
+			TEST_THREADS_LOG("rte_thread_join()");
+			ret = -1;
+			break;
+		}
+	}
+
+	ret = rte_thread_barrier_destroy(&barrier);
+	if (ret != 0) {
+		TEST_THREADS_LOG("rte_thread_barrier_destroy()");
+		ret = -1;
+	}
+
+	return ret;
+}
+
+static size_t val;
+
+static void *
+thread_loop_mutex(void *arg)
+{
+	rte_thread_mutex *mutex = arg;
+
+	rte_thread_mutex_lock(mutex);
+	val++;
+	rte_thread_mutex_unlock(mutex);
+
+	return NULL;
+}
+
+static int
+test_thread_mutex(void)
+{
+	rte_thread_t threads_ids[THREADS_COUNT];
+	rte_thread_mutex mutex;
+	size_t i;
+	size_t j;
+	int ret = 0;
+
+	/*
+	 * The value that each thread will increment while holding the mutex.
+	 */
+	val = 0;
+
+	ret = rte_thread_mutex_init(&mutex);
+	if (ret != 0) {
+		TEST_THREADS_LOG("rte_thread_mutex_init()");
+		return -1;
+	}
+
+	for (i = 0; i < THREADS_COUNT; ++i) {
+		if (rte_thread_create(&threads_ids[i], NULL,
+				thread_loop_mutex, &mutex) != 0) {
+			printf("Error, created only %zu threads\n", i);
+			ret = -1;
+			goto error;
+		}
+	}
+
+error:
+	for (j = 0; j < i; ++j) {
+		ret = rte_thread_join(threads_ids[j], NULL);
+		if (ret != 0) {
+			TEST_THREADS_LOG("rte_thread_join()");
+			ret = -1;
+		}
+	}
+
+	ret = rte_thread_mutex_destroy(&mutex);
+	if (ret != 0) {
+		TEST_THREADS_LOG("rte_thread_mutex_destroy()");
+		ret = -1;
+	}
+
+	if (i != val) {
+		printf("Unexpected value: %zu!. Expected %zu. "
+			"Each thread should increment the value once.\n",
+			val, i);
+		ret = -1;
+	}
+
+	return ret;
+}
+
+struct thread_affinity_ctx {
+	size_t idx;
+	unsigned int result;
+};
+
+static void *
+thread_loop_attributes_affinity(void *arg)
+{
+	struct thread_affinity_ctx *ctx = arg;
+	rte_cpuset_t cpuset;
+	size_t i;
+
+	ctx->result = 0;
+
+	CPU_ZERO(&cpuset);
+	if (pthread_getaffinity_np(pthread_self(), sizeof(cpuset),
+			&cpuset) != 0) {
+		ctx->result = 1;
+		TEST_THREADS_LOG("pthread_getaffinity_np()");
+		return NULL;
+	}
+
+	if (!CPU_ISSET(ctx->idx, &cpuset)) {
+		ctx->result = 1;
+		printf("CPU %zu should be set for thread %zu\n",
+			ctx->idx, ctx->idx);
+		return NULL;
+	}
+
+	for (i = 0; i < CPU_SETSIZE; ++i) {
+		if (i != ctx->idx && CPU_ISSET(i, &cpuset)) {
+			ctx->result = 1;
+			printf("CPU %zu should not be set for thread %zu\n",
+				i, ctx->idx);
+			return NULL;
+		}
+	}
+	return NULL;
+}
+
+static int
+test_thread_attributes_affinity(void)
+{
+	rte_thread_t threads_ids[THREADS_COUNT];
+	struct thread_affinity_ctx ctx[THREADS_COUNT] = {};
+	rte_thread_attr_t attr;
+	rte_cpuset_t cpuset;
+	size_t i;
+	size_t j;
+	int ret = 0;
+
+	ret = rte_thread_attr_init(&attr);
+	if (ret != 0) {
+		TEST_THREADS_LOG("rte_thread_attr_init()");
+		return -1;
+	}
+
+	for (i = 0; i < THREADS_COUNT; ++i) {
+		CPU_ZERO(&cpuset);
+		CPU_SET(i, &cpuset);
+
+		ret = rte_thread_attr_set_affinity(&attr, &cpuset);
+		if (ret != 0) {
+			ret = -1;
+			TEST_THREADS_LOG("rte_thread_attr_set_affinity()");
+			goto error;
+		}
+
+		ctx[i].idx = i;
+		if (rte_thread_create(&threads_ids[i], &attr,
+				thread_loop_attributes_affinity,
+				&ctx[i]) != 0) {
+			printf("Error, created only %zu threads\n", i);
+			ret = -1;
+			goto error;
+		}
+
+	}
+
+error:
+	for (j = 0; j < i; ++j) {
+		ret = rte_thread_join(threads_ids[j], NULL);
+		if (ret != 0) {
+			TEST_THREADS_LOG("rte_thread_join()");
+			ret = -1;
+			break;
+		}
+
+		if (ctx[j].result != 0)
+			ret = -1;
+	}
+
+	return ret;
+}
+
+static void *
+thread_loop_return(void *arg)
+{
+	RTE_SET_USED(arg);
+	return NULL;
+}
+
+static int
+test_thread_attributes_priority(void)
+{
+	rte_thread_t threads_ids[THREADS_COUNT];
+	rte_thread_attr_t attr;
+	size_t i;
+	size_t j;
+	int ret = 0;
+	int policy;
+	struct sched_param param;
+
+	ret = rte_thread_attr_init(&attr);
+	if (ret != 0) {
+		TEST_THREADS_LOG("rte_thread_attr_init()");
+		return -1;
+	}
+
+	ret = rte_thread_attr_set_priority(&attr, RTE_THREAD_PRIORITY_NORMAL);
+	if (ret != 0) {
+		TEST_THREADS_LOG("rte_thread_attr_set_priority()");
+		return -1;
+	}
+
+	for (i = 0; i < THREADS_COUNT; ++i) {
+		if (rte_thread_create(&threads_ids[i], &attr,
+				thread_loop_return, NULL) != 0) {
+			printf("Error, created only %zu threads\n", i);
+			ret = -1;
+			goto error;
+		}
+
+		ret = pthread_getschedparam(
+				(pthread_t)threads_ids[i].opaque_id,
+				&policy, &param);
+		if (ret != 0) {
+			ret = -1;
+			TEST_THREADS_LOG("pthread_getschedparam()");
+			goto error;
+		}
+
+		if (policy != SCHED_OTHER || param.sched_priority != 0) {
+			ret = -1;
+			printf("Unexpected priority: %d or policy: %d\n",
+					param.sched_priority, SCHED_OTHER);
+			goto error;
+		}
+
+	}
+
+error:
+	for (j = 0; j < i; ++j) {
+		ret = rte_thread_join(threads_ids[j], NULL);
+		if (ret != 0) {
+			TEST_THREADS_LOG("rte_thread_join()");
+			ret = -1;
+			break;
+		}
+	}
+
+	return ret;
+}
+
+static int
+test_thread_detach(void)
+{
+	rte_thread_t threads_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_return, NULL) != 0) {
+			printf("Error, Only %zu threads created.\n", i);
+			goto error;
+		}
+	}
+
+error:
+	for (j = 0; j < i; ++j) {
+		ret = rte_thread_detach(threads_ids[j]);
+		if (ret != 0) {
+			TEST_THREADS_LOG("rte_thread_detach()");
+			return -1;
+		}
+	}
+
+	return ret;
+}
+
+static struct unit_test_suite threads_test_suite = {
+	.suite_name = "threads autotest",
+	.setup = NULL,
+	.teardown = NULL,
+	.unit_test_cases = {
+			TEST_CASE(test_thread_self),
+			TEST_CASE(test_thread_barrier),
+			TEST_CASE(test_thread_mutex),
+			TEST_CASE(test_thread_attributes_affinity),
+			TEST_CASE(test_thread_attributes_priority),
+			TEST_CASE(test_thread_detach),
+			TEST_CASES_END()
+	}
+};
+
+static int
+test_threads(void)
+{
+	return unit_test_suite_runner(&threads_test_suite);
+}
+
+REGISTER_TEST_COMMAND(threads_autotest, test_threads);