shortlog should say something like: test: correct worker count calculation On 2016-02-22 16:46, Gary S. Robertson wrote: > During the process of addressing Linaro BUG 2027 which relates to
Add a link to the bug in this changelog. > inaccurate reporting of available CPUs by ODP linux-generic when > running atop a kernel compiled with NO_HZ_FULL support, You should add a test that uses CGROUP (should be enough to expose the bug) that fails with the current code base and passes with this fix. > a number of instances were encountered in the validation and > performance test software where incorrect methods were used to > determine the proper (or maximum) number of worker threads to create. > > The use of odp_cpu_count() for this purpose is incorrect and > deprecated... The documentation doesn't say that you should not use this for this purpose. However, its not actually deprecated right? > odp_cpumask_default_worker() should always be used > to determine the set and number of CPUs available for creating > worker threads. > > The use of odp_cpu_count() for this purpose in conjunction with the > correct means of determining available CPUs resulted in some tests > hanging in calls to odp_barrier_wait() as the barriers were initialized > to expect threads on ALL CPUs rather than on worker CPUs alone... > and there were too few worker threads created to satisfy the barriers. > > The changes below correct all instances I could find of this depecated > technique and allowed all tests to complete successfully with the > BUG 2027 patch applied. (BTW they also run correctly without that patch > after the application of the modifications included here.) > > Signed-off-by: Gary S. Robertson <[email protected]> > --- > test/performance/odp_atomic.c | 9 ++++++--- > test/validation/cpumask/cpumask.c | 2 +- > test/validation/shmem/shmem.c | 5 ++++- > test/validation/timer/timer.c | 16 +++++++++------- > 4 files changed, 20 insertions(+), 12 deletions(-) > > diff --git a/test/performance/odp_atomic.c b/test/performance/odp_atomic.c > index 067329b..7e2e22f 100644 > --- a/test/performance/odp_atomic.c > +++ b/test/performance/odp_atomic.c > @@ -303,6 +303,8 @@ int odp_test_thread_exit(pthrd_arg *arg) > /** test init globals and call odp_init_global() */ > int odp_test_global_init(void) > { > + odp_cpumask_t unused; > + > memset(thread_tbl, 0, sizeof(thread_tbl)); > > if (odp_init_global(NULL, NULL)) { > @@ -310,7 +312,8 @@ int odp_test_global_init(void) > return -1; > } > > - num_workers = odp_cpu_count(); > + num_workers = odp_cpumask_default_worker(&unused, 0); > + > /* force to max CPU count */ > if (num_workers > MAX_WORKERS) > num_workers = MAX_WORKERS; > @@ -378,7 +381,7 @@ int main(int argc, char *argv[]) > goto err_exit; > } > if (test_type < TEST_MIX || test_type > TEST_MAX || > - pthrdnum > odp_cpu_count() || pthrdnum < 0) { > + pthrdnum > num_workers || pthrdnum < 0) { > usage(); > goto err_exit; > } > @@ -386,7 +389,7 @@ int main(int argc, char *argv[]) > } > > if (pthrdnum == 0) > - pthrdnum = odp_cpu_count(); > + pthrdnum = num_workers; > > test_atomic_init(); > test_atomic_store(); > diff --git a/test/validation/cpumask/cpumask.c > b/test/validation/cpumask/cpumask.c > index 2419f47..1bb8f8d 100644 > --- a/test/validation/cpumask/cpumask.c > +++ b/test/validation/cpumask/cpumask.c > @@ -67,7 +67,7 @@ void cpumask_test_odp_cpumask_def(void) > mask_count = odp_cpumask_count(&mask); > CU_ASSERT(mask_count == num_control); > > - CU_ASSERT(num_control == 1); > + CU_ASSERT(num_control >= 1); Good catch, belongs in its own patch. > CU_ASSERT(num_worker <= available_cpus); > CU_ASSERT(num_worker > 0); > } > diff --git a/test/validation/shmem/shmem.c b/test/validation/shmem/shmem.c > index 08425e6..be8bd6f 100644 > --- a/test/validation/shmem/shmem.c > +++ b/test/validation/shmem/shmem.c > @@ -6,6 +6,8 @@ > > #include <odp.h> > #include <odp_cunit_common.h> > +#include <odp/cpumask.h> > + Not needed. > #include "shmem.h" > > #define ALIGE_SIZE (128) > @@ -52,6 +54,7 @@ void shmem_test_odp_shm_sunnyday(void) > pthrd_arg thrdarg; > odp_shm_t shm; > test_shared_data_t *test_shared_data; > + odp_cpumask_t unused; > > shm = odp_shm_reserve(TESTNAME, > sizeof(test_shared_data_t), ALIGE_SIZE, 0); > @@ -70,7 +73,7 @@ void shmem_test_odp_shm_sunnyday(void) > test_shared_data->foo = TEST_SHARE_FOO; > test_shared_data->bar = TEST_SHARE_BAR; > > - thrdarg.numthrds = odp_cpu_count(); > + thrdarg.numthrds = odp_cpumask_default_worker(&unused, 0); > > if (thrdarg.numthrds > MAX_WORKERS) > thrdarg.numthrds = MAX_WORKERS; > diff --git a/test/validation/timer/timer.c b/test/validation/timer/timer.c > index 004670a..bee3e8d 100644 > --- a/test/validation/timer/timer.c > +++ b/test/validation/timer/timer.c > @@ -15,6 +15,7 @@ > > #include <time.h> > #include <odp.h> > +#include <odp/helper/linux.h> > #include "odp_cunit_common.h" > #include "test_debug.h" > #include "timer.h" > @@ -41,12 +42,6 @@ static odp_atomic_u32_t ndelivtoolate; > * caches may make this number lower than the capacity of the pool */ > static odp_atomic_u32_t timers_allocated; > > -/** @private min() function */ > -static int min(int a, int b) > -{ > - return a < b ? a : b; > -} > - > /* @private Timer helper structure */ > struct test_timer { > odp_timer_t tim; /* Timer handle */ > @@ -457,10 +452,17 @@ void timer_test_odp_timer_all(void) > int rc; > odp_pool_param_t params; > odp_timer_pool_param_t tparam; > + odp_cpumask_t unused; > + > /* Reserve at least one core for running other processes so the timer > * test hopefully can run undisturbed and thus get better timing > * results. */ > - int num_workers = min(odp_cpu_count() - 1, MAX_WORKERS); > + int num_workers = odp_cpumask_default_worker(&unused, 0); > + > + /* force to max CPU count */ > + if (num_workers > MAX_WORKERS) > + num_workers = MAX_WORKERS; > + Why do we need MAX_WORKERS when we asks for odp_cpumask_default_worker? Cheers, Anders _______________________________________________ lng-odp mailing list [email protected] https://lists.linaro.org/mailman/listinfo/lng-odp
