On 25 May 2016 at 09:55, Yi He <[email protected]> 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 <[email protected]>
> ---
> 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
>
>
_______________________________________________
lng-odp mailing list
[email protected]
https://lists.linaro.org/mailman/listinfo/lng-odp
