From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 0EA8DA0093; Tue, 3 May 2022 11:38:06 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 9EA3940C35; Tue, 3 May 2022 11:38:05 +0200 (CEST) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id CBBC340691 for ; Tue, 3 May 2022 11:38:03 +0200 (CEST) Received: by linux.microsoft.com (Postfix, from userid 1086) id AE4FE20EAB77; Tue, 3 May 2022 02:38:02 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com AE4FE20EAB77 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1651570682; bh=cl4eVIlOJOSW0YthO0Sh2DZEv04KHolzWunYOQSVYVI=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=SKwkCnSBE6PMxYxAAyv/dQU8szRxmgW/LfQkGoEFe4b9fFxNQYKkRkfEEbgxXZE7V pwhtXpF2xMhzQSGQ9J+AagnVNGOG+6sbpKuuVr/RugEfeYtiYTyP9ZNL69YlWwUVi/ CZPjyyZZn4JdzG4MF8kxrOW+kcytYBCM82/kOzf4= Date: Tue, 3 May 2022 02:38:02 -0700 From: Tyler Retzlaff To: Dmitry Kozlyuk Cc: dev@dpdk.org, thomas@monjalon.net, anatoly.burakov@intel.com, Narcisa Vasile Subject: Re: [PATCH v4 3/3] test/threads: add unit test for thread API Message-ID: <20220503093802.GA5181@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1648819793-18948-1-git-send-email-roretzla@linux.microsoft.com> <1650959442-29330-1-git-send-email-roretzla@linux.microsoft.com> <1650959442-29330-4-git-send-email-roretzla@linux.microsoft.com> <20220501121836.439b883e@sovereign> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220501121836.439b883e@sovereign> User-Agent: Mutt/1.5.21 (2010-09-15) X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org 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 > > Signed-off-by: Tyler Retzlaff > > --- > > 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 > > +#include > > + > > +#include > > +#include > > + > > +#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);