[v2] helper: linux: odp thread cpu affinity APIs

Provide APIs to set and get cpu affinity in ODP threads,
and set affinity to the 1st available control cpu for all
odp test/validation programs in odp_cunit_common library.

Signed-off-by: Yi He <yi.he@linaro.org>
---
 helper/include/odp/helper/linux.h           | 25 +++++++-
 helper/linux.c                              | 58 +++++++++++++++++
 helper/test/odpthreads.c                    | 96 ++++++++++++++++++++++++++---
 platform/linux-generic/test/ring/.gitignore |  2 +-
 test/validation/common/odp_cunit_common.c   | 21 ++++++-
 5 files changed, 187 insertions(+), 15 deletions(-)

On 25 May 2016 at 09:55, Yi He <yi.he@linaro.org> wrote:

> Provide APIs to set and get cpu affinity in ODP threads,
> and set affinity to the 1st available control cpu for all
> odp test/validation programs in odp_cunit_common library.
>
> Signed-off-by: Yi He <yi.he@linaro.org>
> ---
>  helper/include/odp/helper/linux.h           | 25 +++++++-
>  helper/linux.c                              | 58 +++++++++++++++++
>  helper/test/odpthreads.c                    | 96
> ++++++++++++++++++++++++++---
>  platform/linux-generic/test/ring/.gitignore |  2 +-
>  test/validation/common/odp_cunit_common.c   | 21 ++++++-
>  5 files changed, 187 insertions(+), 15 deletions(-)
>
> diff --git a/helper/include/odp/helper/linux.h
> b/helper/include/odp/helper/linux.h
> index 2e89833..db27514 100644
> --- a/helper/include/odp/helper/linux.h
> +++ b/helper/include/odp/helper/linux.h
> @@ -122,7 +122,6 @@ int odph_linux_pthread_create(odph_linux_pthread_t
> *pthread_tbl,
>   */
>  void odph_linux_pthread_join(odph_linux_pthread_t *thread_tbl, int num);
>
> -
>  /**
>   * Fork a process
>   *
> @@ -200,6 +199,30 @@ int odph_odpthreads_create(odph_odpthread_t
> *thread_tbl,
>  int odph_odpthreads_join(odph_odpthread_t *thread_tbl);
>
>  /**
> + * Set CPU affinity of the current odp thread
> + *
> + * CPU affinity determines the CPU core on which the thread is
> + * eligible to run.
> + *
> + * @param cpu           The affinity CPU core
> + *
> + * @return 0 on success, -1 on failure
> + */
> +int odph_odpthread_setaffinity(const int cpu);
> +
> +/**
> + * Get CPU affinity of the current odp thread
> + *
> + * CPU affinity determines the CPU core on which the thread is
> + * eligible to run.
> + *
> + * @param cpu[out]      The affinity CPU core
> + *
> + * @return 0 on success, -1 on failure
> + */
>

Any reason not to return the cpu directely as many functions do?
int odph_odpthread_getaffinity();
the returned value would be positive or negative on error.

+int odph_odpthread_getaffinity(int *cpu);
> +
> +/**
>   * Merge getopt options
>   *
>   * Given two sets of getopt options (each containing possibly both short
> diff --git a/helper/linux.c b/helper/linux.c
> index 6366694..aad03cf 100644
> --- a/helper/linux.c
> +++ b/helper/linux.c
> @@ -12,6 +12,7 @@
>  #include <sys/types.h>
>  #include <sys/wait.h>
>  #include <sys/prctl.h>
> +#include <sys/syscall.h>
>
>  #include <stdlib.h>
>  #include <string.h>
> @@ -492,6 +493,63 @@ int odph_odpthreads_join(odph_odpthread_t *thread_tbl)
>         return (retval < 0) ? retval : terminated;
>  }
>
> +/* man gettid() notes:
> + * Glibc does not provide a wrapper for this system call;
> + */
> +static inline pid_t __gettid(void)
> +{
> +       return (pid_t)syscall(SYS_gettid);
> +}
> +
> +int odph_odpthread_setaffinity(const int cpu)
> +{
> +       cpu_set_t cpuset;
> +
> +       CPU_ZERO(&cpuset);
> +       CPU_SET(cpu, &cpuset);
> +
> +       /* determine main process or pthread based on
> +        * equality of thread and thread group IDs.
> +        * refer to question: http://stackoverflow.com/questions/6372102/
> +        */
>

I don't think links to start overflow are welcome in the code. I guess
mostly because links become obsolete with time.
(I did the same and it was rejected :-) )


> +       if (__gettid() == getpid()) {
> +               return sched_setaffinity(
> +                       0, /* pid zero means calling process */
> +                       sizeof(cpu_set_t), &cpuset);
> +       }
> +
> +       /* on error, they return a nonzero error number. */
> +       return (0 == pthread_setaffinity_np(
> +               pthread_self(), sizeof(cpu_set_t), &cpuset)) ? 0 : -1;
> +}
> +
> +int odph_odpthread_getaffinity(int *cpu)
> +{
> +       int _return = 0;
>

why that underscore? the usage of underscores is quite obscure within the
odp code, but function  local variables have no reason for it, If they do,
then you cpu_set should be underscored as well


> +       cpu_set_t cpuset;
> +
> +       CPU_ZERO(&cpuset);
> +       if (__gettid() == getpid()) {
> +               _return = sched_getaffinity(
> +                       0, sizeof(cpu_set_t), &cpuset);
> +       } else {
> +               _return = pthread_getaffinity_np(
> +                       pthread_self(), sizeof(cpu_set_t), &cpuset);
> +       }
> +
> +       /* ODP thread mean to run on single CPU core */
> +       if ((_return == 0) && (CPU_COUNT(&cpuset) == 1)) {
> +               for (int _cpu = 0; _cpu < CPU_SETSIZE; _cpu++) {
>

I think it was said that variable declaration should be at function
begining. no underscore there either.


> +                       if (CPU_ISSET(_cpu, &cpuset)) {
> +                               if (cpu != NULL)
> +                                       *cpu = _cpu;
> +                               return 0;
> +                       }
> +               }
> +       }
> +       return -1;
> +}
> +
>  /*
>   * return the number of elements in an array of getopt options, excluding
> the
>   * terminating {0,0,0,0}
> diff --git a/helper/test/odpthreads.c b/helper/test/odpthreads.c
> index bba4fa5..b05a93a 100644
> --- a/helper/test/odpthreads.c
> +++ b/helper/test/odpthreads.c
> @@ -10,47 +10,112 @@
>   * the option passed to the program (--odph_proc, --odph_thread or both)
>   */
>
> +#include <unistd.h>
> +#include <stdlib.h>
> +
>  #include <test_debug.h>
>  #include <odp_api.h>
>  #include <odp/helper/linux.h>
>
>  #define NUMBER_WORKERS 16
> +
> +/* register odp_term_local/global() calls atexit() */
> +static void main_exit(void);
> +
> +/* delayed assertion after threads collection */
> +static int worker_results[NUMBER_WORKERS];
> +
>  static int worker_fn(void *arg TEST_UNUSED)
>  {
> +       odp_cpumask_t workers;
> +       int cpu;
> +       int *result = NULL;
> +
> +       /* save the thread result for delayed assertion */
> +       result = &worker_results[odp_cpu_id() % NUMBER_WORKERS];
>

Is this correct? If your worker happen to be cpu 4,8,12,and 16 and
 NUMBER_WORKERS is 4,
they will all overwrite the same array value. I am aware that the worker
will probably get contiguous cpu numbers,
but the test shouldn't really assume that, right?

See  also my following note (marked (1)), later

+
>         /* depend on the odp helper to call odp_init_local */
>
>         printf("Worker thread on CPU %d\n", odp_cpu_id());
>
> +       odp_cpumask_zero(&workers);
> +       odp_cpumask_default_worker(&workers, NUMBER_WORKERS);
> +
> +       /* verify CPU affinity was already set */
> +       if ((odph_odpthread_getaffinity(&cpu) != 0) ||
> +           !odp_cpumask_isset(&workers, cpu)) {
> +               *result = -1;
> +               printf("Worker thread(%d)'s CPU "
> +                      "affinity was invalid.\n", odp_cpu_id());
> +       }
> +
> +       /* verify API is workable by re-configure the same */
>

maybe you should write "helper API" rather than just API in this comment.
API often reffers to the ODP API. I won't hang you on that :-)


> +       if (odph_odpthread_setaffinity(cpu) != 0) {
> +               *result = -1;
> +               printf("Re-configure worker thread(%d)'s "
> +                      "CPU affinity failed.\n", odp_cpu_id());
> +       }
> +
>         /* depend on the odp helper to call odp_term_local */
>
>         return 0;
>  }
>
> +static odp_instance_t odp_instance;
>

I would have placed this line at top of file. but odp_instance is a much
better name than instance :-)


>  /* Create additional dataplane opdthreads */
>  int main(int argc, char *argv[])
>  {
> -       odp_instance_t instance;
>         odph_odpthread_params_t thr_params;
>         odph_odpthread_t thread_tbl[NUMBER_WORKERS];
>         odp_cpumask_t cpu_mask;
>         int num_workers;
> -       int cpu;
> +       int cpu, affinity;
>         int ret;
>         char cpumaskstr[ODP_CPUMASK_STR_SIZE];
>
>         /* let helper collect its own arguments (e.g. --odph_proc) */
>         odph_parse_options(argc, argv, NULL, NULL);
>
> -       if (odp_init_global(&instance, NULL, NULL)) {
> +       if (odp_init_global(&odp_instance, NULL, NULL)) {
>                 LOG_ERR("Error: ODP global init failed.\n");
>                 exit(EXIT_FAILURE);
>         }
>
> -       if (odp_init_local(instance, ODP_THREAD_CONTROL)) {
> +       if (odp_init_local(odp_instance, ODP_THREAD_CONTROL)) {
>                 LOG_ERR("Error: ODP local init failed.\n");
>                 exit(EXIT_FAILURE);
>         }
>
> +       /* register termination callback */
> +       atexit(main_exit);
> +
> +       /* reset all worker thread results to success */
> +       memset(worker_results, 0, sizeof(worker_results));
> +
> +       odp_cpumask_zero(&cpu_mask);
> +       /* allocate the 1st available control cpu to main process */
> +       if (odp_cpumask_default_control(&cpu_mask, 1) != 1) {
> +               LOG_ERR("Allocate main process CPU core failed.\n");
> +               exit(EXIT_FAILURE);
> +       }
> +
> +       cpu = odp_cpumask_first(&cpu_mask);
> +       if (odph_odpthread_setaffinity(cpu) != 0) {
> +               LOG_ERR("Set main process affinify to cpu(%d) failed.\n",
> cpu);
> +               exit(EXIT_FAILURE);
> +       }
> +
> +       /* read back affinity to verify */
> +       if ((odph_odpthread_getaffinity(&affinity) != 0) ||
> +           (cpu != affinity)) {
> +               LOG_ERR("Verify main process affinity failed: "
> +                       "set(%d) read(%d).\n", cpu, affinity);
> +               exit(EXIT_FAILURE);
> +       }
> +       cpu = 0;
> +       affinity = 0; /* reset variables */
>

useless comment


> +       odp_cpumask_zero(&cpu_mask);
> +
>         /* discover how many opdthreads this system can support */
>         num_workers = odp_cpumask_default_worker(&cpu_mask,
> NUMBER_WORKERS);
>         if (num_workers < NUMBER_WORKERS) {
> @@ -78,7 +143,7 @@ int main(int argc, char *argv[])
>         thr_params.start    = worker_fn;
>         thr_params.arg      = NULL;
>         thr_params.thr_type = ODP_THREAD_WORKER;
> -       thr_params.instance = instance;
> +       thr_params.instance = odp_instance;
>
>         odph_odpthreads_create(&thread_tbl[0], &cpu_mask, &thr_params);
>
> @@ -86,15 +151,26 @@ int main(int argc, char *argv[])
>         if (ret < 0)
>                 exit(EXIT_FAILURE);
>
> +       /* assert all worker thread results */
> +       for (cpu = 0; cpu < num_workers; cpu++) {
> +               if (worker_results[cpu] < 0) {
> +                       LOG_ERR("Worker thread %d failed.\n", cpu);
> +                       exit(EXIT_FAILURE);
> +               }
> +       }
>

(1) Am I missing something? why are you doing all this? If worker_fn()
returned a non-zero value (e.g. -1),
odph_odpthread_join() should also return a failing code. All the stuff
with worker_results[] can be removed, as well as the above code.

+
> +       return 0;
> +}
> +
> +static void main_exit(void)
> +{
>         if (odp_term_local()) {
>                 LOG_ERR("Error: ODP local term failed.\n");
> -               exit(EXIT_FAILURE);
> +               _exit(EXIT_FAILURE);
>         }
>
> -       if (odp_term_global(instance)) {
> +       if (odp_term_global(odp_instance)) {
>                 LOG_ERR("Error: ODP global term failed.\n");
> -               exit(EXIT_FAILURE);
> +               _exit(EXIT_FAILURE);
>         }
> -
> -       return 0;
>  }
> diff --git a/platform/linux-generic/test/ring/.gitignore
> b/platform/linux-generic/test/ring/.gitignore
> index 4767f5e..7341a34 100644
> --- a/platform/linux-generic/test/ring/.gitignore
> +++ b/platform/linux-generic/test/ring/.gitignore
> @@ -1 +1 @@
> -ringtest
> +ring_main
>

This is a good fix but has nothing to do here. should be its own patch


> diff --git a/test/validation/common/odp_cunit_common.c
> b/test/validation/common/odp_cunit_common.c
> index 7df9aa6..2337c92 100644
> --- a/test/validation/common/odp_cunit_common.c
> +++ b/test/validation/common/odp_cunit_common.c
> @@ -333,9 +333,24 @@ int odp_cunit_update(odp_suiteinfo_t testsuites[])
>  int odp_cunit_register(odp_suiteinfo_t testsuites[])
>  {
>         /* call test executable init hook, if any */
> -       if (global_init_term.global_init_ptr &&
> -           ((*global_init_term.global_init_ptr)(&instance) != 0))
> -               return -1;
> +       if (global_init_term.global_init_ptr) {
> +               if ((*global_init_term.global_init_ptr)(&instance) == 0) {
> +                       /* After ODP initialization, set main thread's
> +                        * CPU affinity to the 1st available control CPU
> core
> +                        */
> +                       int cpu = 0;
> +                       odp_cpumask_t cpuset;
> +
> +                       odp_cpumask_zero(&cpuset);
> +                       if (odp_cpumask_default_control(&cpuset, 1) == 1) {
> +                               cpu = odp_cpumask_first(&cpuset);
> +                               odph_odpthread_setaffinity(cpu);
> +                       }
> +               } else {
> +                       /* ODP initialization failed */
> +                       return -1;
> +               }
> +       }
>
>         CU_set_error_action(CUEA_ABORT);
>

Hope these comments helps. Good luck :-)

Christophe.


> --
> 2.7.4
>
>