Re: [PATCH v4 3/3] test/threads: add unit test for thread API

[Tyler Retzlaff <roretzla@linux.microsoft.com>]

On Sun, May 01, 2022 at 12:18:36PM +0300, Dmitry Kozlyuk wrote:
> 2022-04-26 00:50 (UTC-0700), Tyler Retzlaff:
> > Establish unit test for testing thread api. Initial unit tests
> > for rte_thread_{get,set}_affinity_by_id().
> > 
> > Signed-off-by: Narcisa Vasile <navasile@microsoft.com>
> > Signed-off-by: Tyler Retzlaff <roretzla@linux.microsoft.com>
> > ---
> >  app/test/meson.build    |  2 ++
> >  app/test/test_threads.c | 89 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 91 insertions(+)
> >  create mode 100644 app/test/test_threads.c
> > 
> > diff --git a/app/test/meson.build b/app/test/meson.build
> > index 5fc1dd1..5a9d69b 100644
> > --- a/app/test/meson.build
> > +++ b/app/test/meson.build
> > @@ -133,6 +133,7 @@ test_sources = files(
> >          'test_tailq.c',
> >          'test_thash.c',
> >          'test_thash_perf.c',
> > +        'test_threads.c',
> >          'test_timer.c',
> >          'test_timer_perf.c',
> >          'test_timer_racecond.c',
> > @@ -238,6 +239,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 0000000..0ca6745
> > --- /dev/null
> > +++ b/app/test/test_threads.c
> > @@ -0,0 +1,89 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright (C) 2022 Microsoft Corporation
> > + */
> > +
> > +#include <string.h>
> > +#include <pthread.h>
> > +
> > +#include <rte_thread.h>
> > +#include <rte_debug.h>
> > +
> > +#include "test.h"
> > +
> > +RTE_LOG_REGISTER(threads_logtype_test, test.threads, INFO);
> > +
> > +static uint32_t thread_id_ready;
> > +
> > +static void *
> > +thread_main(void *arg)
> > +{
> > +	*(rte_thread_t *)arg = rte_thread_self();
> > +	__atomic_store_n(&thread_id_ready, 1, __ATOMIC_RELEASE);
> > +
> > +	return NULL;
> > +}
> > +
> > +static int
> > +test_thread_affinity(void)
> > +{
> > +	pthread_t id;
> > +	rte_thread_t thread_id;
> > +
> > +	RTE_TEST_ASSERT(pthread_create(&id, NULL, thread_main, &thread_id) == 0,
> > +		"Failed to create thread");
> > +
> > +	while (__atomic_load_n(&thread_id_ready, __ATOMIC_ACQUIRE) == 0)
> > +		;
> > +
> > +	rte_cpuset_t cpuset0;
> 
> Variables should be declared at the beginning of the block.

just curious because we don't require c99 compiler or because it is
not compliant with dpdk style/convention?

regardless, will fix just curious.

> 
> > +	RTE_TEST_ASSERT(rte_thread_get_affinity_by_id(thread_id, &cpuset0) == 0,
> > +		"Failed to get thread affinity");
> > +
> > +	rte_cpuset_t cpuset1;
> > +	RTE_TEST_ASSERT(rte_thread_get_affinity_by_id(thread_id, &cpuset1) == 0,
> > +		"Failed to get thread affinity");
> > +	RTE_TEST_ASSERT(memcmp(&cpuset0, &cpuset1, sizeof(rte_cpuset_t)) == 0,
> > +		"Affinity should be stable");
> > +
> > +	RTE_TEST_ASSERT(rte_thread_set_affinity_by_id(thread_id, &cpuset1) == 0,
> > +		"Failed to set thread affinity");
> > +	RTE_TEST_ASSERT(rte_thread_get_affinity_by_id(thread_id, &cpuset0) == 0,
> > +		"Failed to get thread affinity");
> > +	RTE_TEST_ASSERT(memcmp(&cpuset0, &cpuset1, sizeof(rte_cpuset_t)) == 0,
> > +		"Affinity should be stable");
> > +
> > +	size_t i;
> > +	for (i = 1; i < CPU_SETSIZE; i++)
> > +		if (CPU_ISSET(i, &cpuset0)) {
> > +			CPU_ZERO(&cpuset0);
> > +			CPU_SET(i, &cpuset0);
> > +
> > +			break;
> > +		}
> > +	RTE_TEST_ASSERT(rte_thread_set_affinity_by_id(thread_id, &cpuset0) == 0,
> > +		"Failed to set thread affinity");
> > +	RTE_TEST_ASSERT(rte_thread_get_affinity_by_id(thread_id, &cpuset1) == 0,
> > +		"Failed to get thread affinity");
> > +	RTE_TEST_ASSERT(memcmp(&cpuset0, &cpuset1, sizeof(rte_cpuset_t)) == 0,
> > +		"Affinity should be stable");
> 
> The message is not really relevant to the check done.
> "Retrieved affinity differs from requested"?

will fix.

> 
> I think this is the only check worth keeping in this test.
> The fist one is speculative: if we expect that a wrong implementation may
> change affinity sporadically, the test can't prove it doesn't.

if you're saying it isn't deterministic i partially agree. regardless
many bugs are not deterministic. e.g. composite/split initialization bugs
as a class, there could be side-effects get/get since the function is
not pure.

enforcing a rule that tests only ever fail for deterministic reasons
would eliminate opportunity to hint at various classes of initialization
bugs. as an example right now pthread_create shim has one of these class
of bugs and there is no deterministic test that guarantees 100% failure
though a non-deterministic test enabled today would show a high rate of
failure warranting investigation before now.

the only downside is that if a test like this does fail it is possible
to be unrelated to the implementation of the feature being tested and
that can be misleading and i admit frustrating.

> The second one isn't stronger than this one.

i agree that the set/get/compare is duplicated i'll remove it the
first occurence but the get/get/compare i think should stay.

> 
> > +
> > +	return 0;
> > +}
> > +
> > +static struct unit_test_suite threads_test_suite = {
> > +	.suite_name = "threads autotest",
> > +	.setup = NULL,
> > +	.teardown = NULL,
> > +	.unit_test_cases = {
> > +		TEST_CASE(test_thread_affinity),
> > +		TEST_CASES_END()
> > +	}
> > +};
> > +
> > +static int
> > +test_threads(void)
> > +{
> > +	return unit_test_suite_runner(&threads_test_suite);
> > +}
> > +
> > +REGISTER_TEST_COMMAND(threads_autotest, test_threads);