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 <navas...@microsoft.com>
> > Signed-off-by: Tyler Retzlaff <roret...@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);
