| Message ID | 20160428151835.GA26130@perric |
|---|---|
| State | New |
| Headers | show |
On 04/28 17:18:36, Christophe Milard wrote: > Since v3: > -fixed rebase error (Christophe) > -rebased Thanks for the rebase. test_in_process_mode_v4 merged cleanly into origin/master and build and tests PASS. I've given this a look, and it appears we're headed in the right direction. > diff --git a/example/classifier/odp_classifier.c b/example/classifier/odp_classifier.c > index a477e23..4057457 100644 > --- a/example/classifier/odp_classifier.c > +++ b/example/classifier/odp_classifier.c > @@ -815,10 +802,16 @@ static void parse_args(int argc, char *argv[], appl_args_t *appl_args) > {NULL, 0, NULL, 0} > }; > > + static const char *shortopts = "+c:t:i:p:m:t:h"; > + > + /* let helper collect its own arguments (e.g. --odph_proc) */ > + odph_parse_options(argc, argv, shortopts, longopts); > + > + opterr = 0; /* do not issue errors on helper options */ Please use the default behavior of opterr _or_ add the case statement for '?' when appropriate. > while (1) { > - opt = getopt_long(argc, argv, "+c:t:i:p:m:t:h", > - longopts, &long_index); > + opt = getopt_long(argc, argv, shortopts, > + longopts, &long_index); > > if (opt == -1) > break; /* No more options */ > diff --git a/example/l2fwd_simple/odp_l2fwd_simple.c b/example/l2fwd_simple/odp_l2fwd_simple.c > index 45bb9b1..7b67705 100644 > --- a/example/l2fwd_simple/odp_l2fwd_simple.c > +++ b/example/l2fwd_simple/odp_l2fwd_simple.c > @@ -116,13 +117,33 @@ int main(int argc, char **argv) > odp_pool_t pool; > odp_pool_param_t params; > odp_cpumask_t cpumask; > - odph_linux_pthread_t thd; > + odph_odpthread_t thd; > odp_instance_t instance; > - odph_linux_thr_params_t thr_params; > + odph_odpthread_params_t thr_params; > + int rc = 0; > + int opt; > + int long_index; > + > + static const struct option longopts[] = { {NULL, 0, NULL, 0} }; > + static const char *shortopts = ""; > + > + /* let helper collect its own arguments (e.g. --odph_proc) */ > + odph_parse_options(argc, argv, shortopts, longopts); > + > + /* > + * parse own options: currentely none, but this will move optind > + * to the first non-option argument. (in case there where helprt args) > + */ I suppose this is OK given the pre-existing arg handling. > + opterr = 0; /* do not issue errors on helper options */ > + while (!rc) { Please use a simpler loop, e.g. for (;;) or while (1) > + opt = getopt_long(argc, argv, shortopts, longopts, &long_index); > + if (-1 == opt) > + break; /* No more options */ > + } > > - if (argc != 5 || > - odph_eth_addr_parse(&global.dst, argv[3]) != 0 || > - odph_eth_addr_parse(&global.src, argv[4]) != 0) { > + if (argc != optind + 4 || > + odph_eth_addr_parse(&global.dst, argv[optind + 2]) != 0 || > + odph_eth_addr_parse(&global.src, argv[optind + 3]) != 0) { > printf("Usage: odp_l2fwd_simple eth0 eth1 01:02:03:04:05:06" > " 07:08:09:0a:0b:0c\n"); > printf("Where eth0 and eth1 are the used interfaces" > diff --git a/helper/include/odp/helper/linux.h b/helper/include/odp/helper/linux.h > index 7a6504f..a9ec90a 100644 > --- a/helper/include/odp/helper/linux.h > +++ b/helper/include/odp/helper/linux.h > @@ -25,111 +25,146 @@ extern "C" { > #include <odp_api.h> > > #include <pthread.h> > +#include <getopt.h> Please consider migrating CLI parsing via C library to a more appropriate location. > #include <sys/types.h> > > -/** Thread parameter for Linux pthreads and processes */ > +/** odpthread linux type: whether an ODP thread is a linux thread or process */ > +typedef enum odph_odpthread_linuxtype_e { > + NOT_STARTED = 0, > + PROCESS, > + PTHREAD > +} odph_odpthread_linuxtype_t; Please use more descriptive identifiers in enums and consider a MAX if used for iteration. > +/** odpthread parameters for odp threads (pthreads and processes) */ > typedef struct { > - void *(*start)(void *); /**< Thread entry point function */ > + int (*start)(void *); /**< Thread entry point function */ > void *arg; /**< Argument for the function */ > odp_thread_type_t thr_type; /**< ODP thread type */ > odp_instance_t instance; /**< ODP instance handle */ > -} odph_linux_thr_params_t; > +} odph_odpthread_params_t; > > -/** Linux pthread state information */ > +/** The odpthread starting arguments, used both in process or thread mode */ > typedef struct { > - pthread_t thread; /**< Pthread ID */ > - pthread_attr_t attr; /**< Pthread attributes */ > - int cpu; /**< CPU ID */ > - /** Copy of thread params */ > - odph_linux_thr_params_t thr_params; > -} odph_linux_pthread_t; > - > + odph_odpthread_linuxtype_t linuxtype; > + odph_odpthread_params_t thr_params; /*copy of thread start parameter*/ > +} odph_odpthread_start_args_t; > > -/** Linux process state information */ > +/** Linux odpthread state information, used both in process or thread mode */ > typedef struct { > - pid_t pid; /**< Process ID */ > - int cpu; /**< CPU ID */ > - int status; /**< Process state change status */ > -} odph_linux_process_t; > + odph_odpthread_start_args_t start_args; > + int cpu; /**< CPU ID */ > + int last; /**< true if last table entry */ > + union { > + struct { /* for thread implementation */ > + pthread_t thread; /**< Pthread ID */ thread_id > + pthread_attr_t attr; /**< Pthread attributes */ If threading affinity is determined at creation time, and never changes, please consider reducing the lifetime of this variable. > + }; > + struct { /* for process implementation */ > + pid_t pid; /**< Process ID */ > + int status; /**< Process state chge status*/ > + }; > + }; Please consider avoiding the use of anonymous structures within an anonymous union as it makes the code harder to read. > +} odph_odpthread_t; > > /** > - * Creates and launches pthreads > + * Creates and launches odpthreads (as linux threads or processes) > * > * Creates, pins and launches threads to separate CPU's based on the cpumask. > * > - * @param[out] pthread_tbl Table of pthread state information records. Table > - * must have at least as many entries as there are > - * CPUs in the CPU mask. > - * @param mask CPU mask > - * @param thr_params Linux helper thread parameters > + * @param thread_tbl Thread table > + * @param mask CPU mask > + * @param start_routine Thread start function > + * @param arg Thread argument > + * @param thr_type Thread type Please document thr_params and remove start_routine, arg, thr_type > * > * @return Number of threads created > */ > -int odph_linux_pthread_create(odph_linux_pthread_t *pthread_tbl, > - const odp_cpumask_t *mask, > - const odph_linux_thr_params_t *thr_params); > +int odph_odpthreads_create(odph_odpthread_t *thread_tbl, > + const odp_cpumask_t *mask, > + const odph_odpthread_params_t *thr_params); > > /** > - * Waits pthreads to exit > + * Waits odpthreads (as linux threads or processes) to exit. > * > - * Returns when all threads have been exit. > + * Returns when all odpthreads have terminated. > * > * @param thread_tbl Thread table > - * @param num Number of threads to create > - * > - */ > -void odph_linux_pthread_join(odph_linux_pthread_t *thread_tbl, int num); > - > - > -/** > - * Fork a process > - * > - * Forks and sets CPU affinity for the child process. Ignores 'start' and 'arg' > - * thread parameters. > - * > - * @param[out] proc Pointer to process state info (for output) > - * @param cpu Destination CPU for the child process > - * @param thr_params Linux helper thread parameters > + * @return The number of joined threads or -1 on error. > + * (error occurs if any of the start_routine return non-zero or if > + * the thread join/process wait itself failed -e.g. as the result of a kill) > > diff --git a/helper/linux.c b/helper/linux.c > index 24e243b..dd6ab8b 100644 > --- a/helper/linux.c > +++ b/helper/linux.c > @@ -1,4 +1,4 @@ > -/* Copyright (c) 2013, Linaro Limited > +/* Copyright (c) 2013, Linaro Limited Please revert this change > * All rights reserved. > * > * SPDX-License-Identifier: BSD-3-Clause > @@ -21,218 +21,368 @@ > #include <odp/helper/linux.h> > #include "odph_debug.h" > > -static void *odp_run_start_routine(void *arg) > +static struct { > + int proc; /* true when process mode is required */ > + int thrd; /* true when thread mode is required */ > +} helper_options; This is a boolean. Please avoid the use of global state for code like this. > +/* > + * wrapper for odpthreads, either implemented as linux threads or processes. > + * (in process mode, if start_routine returns NULL, the process return 1. > + */ > +static void *odpthread_run_start_routine(void *arg) > { > - odph_linux_thr_params_t *thr_params = arg; > + int status; > + int ret; > + odph_odpthread_params_t *thr_params; > + > + odph_odpthread_start_args_t *start_args = arg; > + > + thr_params = &start_args->thr_params; > > /* ODP thread local init */ > if (odp_init_local(thr_params->instance, thr_params->thr_type)) { > ODPH_ERR("Local init failed\n"); > + if (start_args->linuxtype == PROCESS) > + exit(-1); > return NULL; > } > > - void *ret_ptr = thr_params->start(thr_params->arg); > - int ret = odp_term_local(); > + /* debug: */ > + printf("helper: ODP %s thread started as linux %s. (pid=%d)\n", > + thr_params->thr_type == ODP_THREAD_WORKER ? "worker" : "control", > + (start_args->linuxtype == PTHREAD) ? "pthread" : "process", > + (int)getpid()); Please use appropriate logging facilities instead of printf(). > + status = thr_params->start(thr_params->arg); > + ret = odp_term_local(); > > if (ret < 0) > ODPH_ERR("Local term failed\n"); > else if (ret == 0 && odp_term_global(thr_params->instance)) > ODPH_ERR("Global term failed\n"); > > - return ret_ptr; > + /* for process implementation of odp threads, just return status... */ > + if (start_args->linuxtype == PROCESS) > + exit(status); > + > + /* threads implementation return void* pointers: cast status to that. */ Please avoid explicit cast. > + return (void *)(long)status; > } This review is 1% finished.
On 29 April 2016 at 00:48, Brian Brooks <brian.brooks@linaro.org> wrote: > On 04/28 17:18:36, Christophe Milard wrote: > > Since v3: > > -fixed rebase error (Christophe) > > -rebased > > Thanks for the rebase. test_in_process_mode_v4 merged cleanly into > origin/master and build and tests PASS. > > I've given this a look, and it appears we're headed in the right direction. > > > diff --git a/example/classifier/odp_classifier.c > b/example/classifier/odp_classifier.c > > index a477e23..4057457 100644 > > --- a/example/classifier/odp_classifier.c > > +++ b/example/classifier/odp_classifier.c > > @@ -815,10 +802,16 @@ static void parse_args(int argc, char *argv[], > appl_args_t *appl_args) > > {NULL, 0, NULL, 0} > > }; > > > > + static const char *shortopts = "+c:t:i:p:m:t:h"; > > + > > + /* let helper collect its own arguments (e.g. --odph_proc) */ > > + odph_parse_options(argc, argv, shortopts, longopts); > > + > > + opterr = 0; /* do not issue errors on helper options */ > > Please use the default behavior of opterr _or_ add the case statement for > '?' > when appropriate. > I do think we need opterr=0, sadly: when the test or example (such as the classifier here) parses it options, the command line options being parsed still contains all options, i.e. both the option meant for the classifier itself, and the option meant to the "other layers", (here the helper). If opt_err is left to 1, the classifier's call to get_opt() will issue errors when hitting the helper options, even if we have a "?" in the switch, as far as I understand. So the mechanism taken here is: the caller (e.g. classifier) passes its list of option to the helper which builds the complete list of options by merging it own (helper) option list to the caller options. Then the helper parse this complete list (with default opterr=1), i.e. reacting to unknown options (not being in the merged list) and also picking up its own option semantic, of course. After this the caller (here classifier) parse the options again, picking up its own options only. but should not react on the helper's otpions. I Actually looked at removing the options from argv in the helpers, but this turned up to be quite tricky as well. parse_args() seem to be better at spitting command line option in different owner, but is is not POSIX. not sure we want to insert that kind of dependency for so little. > > > while (1) { > > - opt = getopt_long(argc, argv, "+c:t:i:p:m:t:h", > > - longopts, &long_index); > > + opt = getopt_long(argc, argv, shortopts, > > + longopts, &long_index); > > > > if (opt == -1) > > break; /* No more options */ > > diff --git a/example/l2fwd_simple/odp_l2fwd_simple.c > b/example/l2fwd_simple/odp_l2fwd_simple.c > > index 45bb9b1..7b67705 100644 > > --- a/example/l2fwd_simple/odp_l2fwd_simple.c > > +++ b/example/l2fwd_simple/odp_l2fwd_simple.c > > @@ -116,13 +117,33 @@ int main(int argc, char **argv) > > odp_pool_t pool; > > odp_pool_param_t params; > > odp_cpumask_t cpumask; > > - odph_linux_pthread_t thd; > > + odph_odpthread_t thd; > > odp_instance_t instance; > > - odph_linux_thr_params_t thr_params; > > + odph_odpthread_params_t thr_params; > > + int rc = 0; > > + int opt; > > + int long_index; > > + > > + static const struct option longopts[] = { {NULL, 0, NULL, 0} }; > > + static const char *shortopts = ""; > > + > > + /* let helper collect its own arguments (e.g. --odph_proc) */ > > + odph_parse_options(argc, argv, shortopts, longopts); > > + > > + /* > > + * parse own options: currentely none, but this will move optind > > + * to the first non-option argument. (in case there where helprt > args) > > + */ > > I suppose this is OK given the pre-existing arg handling. > > > + opterr = 0; /* do not issue errors on helper options */ > > + while (!rc) { > > Please use a simpler loop, e.g. for (;;) or while (1) > Of course, there is not much left for rc there! will be addressed in v5 > > > + opt = getopt_long(argc, argv, shortopts, longopts, > &long_index); > > + if (-1 == opt) > > + break; /* No more options */ > > + } > > > > - if (argc != 5 || > > - odph_eth_addr_parse(&global.dst, argv[3]) != 0 || > > - odph_eth_addr_parse(&global.src, argv[4]) != 0) { > > + if (argc != optind + 4 || > > + odph_eth_addr_parse(&global.dst, argv[optind + 2]) != 0 || > > + odph_eth_addr_parse(&global.src, argv[optind + 3]) != 0) { > > printf("Usage: odp_l2fwd_simple eth0 eth1 > 01:02:03:04:05:06" > > " 07:08:09:0a:0b:0c\n"); > > printf("Where eth0 and eth1 are the used interfaces" > > diff --git a/helper/include/odp/helper/linux.h > b/helper/include/odp/helper/linux.h > > index 7a6504f..a9ec90a 100644 > > --- a/helper/include/odp/helper/linux.h > > +++ b/helper/include/odp/helper/linux.h > > @@ -25,111 +25,146 @@ extern "C" { > > #include <odp_api.h> > > > > #include <pthread.h> > > +#include <getopt.h> > > Please consider migrating CLI parsing via C library to a more appropriate > location. > Not sure I understand this: The goal is to have the helpers parsing their own options here. Maybe my comment above makes it clearer. otherwise please explain what you meant. > > > #include <sys/types.h> > > > > -/** Thread parameter for Linux pthreads and processes */ > > +/** odpthread linux type: whether an ODP thread is a linux thread or > process */ > > +typedef enum odph_odpthread_linuxtype_e { > > + NOT_STARTED = 0, > > + PROCESS, > > + PTHREAD > > +} odph_odpthread_linuxtype_t; > > Please use more descriptive identifiers in enums and consider a MAX if > used for iteration. > Any suggestion to any better names? What is meant here is: either the odpthread has never been started (in which case it is neither a linux process or a linux thread), or it is started as a linux process, or it is started as a linux pthread. There are not used in iteration. Maybe the confusion comes from what an odp thread is. I am not very keen on the naming, but despite its name, an ODP thread is just a concurrent execution task. On linux these can be implemented as pthreads or processes. The name "odp tasks" would in my eyes be better (as not as correlated to any specific implementation), but that was not accepted. > > > +/** odpthread parameters for odp threads (pthreads and processes) */ > > typedef struct { > > - void *(*start)(void *); /**< Thread entry point function */ > > + int (*start)(void *); /**< Thread entry point function */ > > void *arg; /**< Argument for the function */ > > odp_thread_type_t thr_type; /**< ODP thread type */ > > odp_instance_t instance; /**< ODP instance handle */ > > -} odph_linux_thr_params_t; > > +} odph_odpthread_params_t; > > > > -/** Linux pthread state information */ > > +/** The odpthread starting arguments, used both in process or thread > mode */ > > typedef struct { > > - pthread_t thread; /**< Pthread ID */ > > - pthread_attr_t attr; /**< Pthread attributes */ > > - int cpu; /**< CPU ID */ > > - /** Copy of thread params */ > > - odph_linux_thr_params_t thr_params; > > -} odph_linux_pthread_t; > > - > > + odph_odpthread_linuxtype_t linuxtype; > > + odph_odpthread_params_t thr_params; /*copy of thread start > parameter*/ > > +} odph_odpthread_start_args_t; > > > > -/** Linux process state information */ > > +/** Linux odpthread state information, used both in process or thread > mode */ > > typedef struct { > > - pid_t pid; /**< Process ID */ > > - int cpu; /**< CPU ID */ > > - int status; /**< Process state change status */ > > -} odph_linux_process_t; > > + odph_odpthread_start_args_t start_args; > > + int cpu; /**< CPU ID */ > > + int last; /**< true if last table > entry */ > > + union { > > + struct { /* for thread implementation */ > > + pthread_t thread; /**< Pthread ID */ > > thread_id > Ok. will take that in v5. > > > + pthread_attr_t attr; /**< Pthread attributes */ > > If threading affinity is determined at creation time, and never changes, > please > consider reducing the lifetime of this variable. > I am not sure what to do here: I don't really understand why this particular member is special here. The attribute themselves are created/destroyed at thread creation/destruction time.This is just a typedef, but indeed these structures (including all their members) are today staticaly allocated in tests. I have not changed that from the old code. I do not think this specific member is special and should be separated from this structure: it really belongs to it. What should be done, I think, is reducing the lifetime of the whole structure (allocating then at task/process creation time and freeing them at join time.). This can be addressed in another patch. > > > + }; > > + struct { /* for process implementation */ > > + pid_t pid; /**< Process ID */ > > + int status; /**< Process state chge > status*/ > > + }; > > + }; > > Please consider avoiding the use of anonymous structures within an > anonymous > union as it makes the code harder to read. > will be fixed in v5. (inherited from old code) > > > +} odph_odpthread_t; > > > > /** > > - * Creates and launches pthreads > > + * Creates and launches odpthreads (as linux threads or processes) > > * > > * Creates, pins and launches threads to separate CPU's based on the > cpumask. > > * > > - * @param[out] pthread_tbl Table of pthread state information records. > Table > > - * must have at least as many entries as there > are > > - * CPUs in the CPU mask. > > - * @param mask CPU mask > > - * @param thr_params Linux helper thread parameters > > + * @param thread_tbl Thread table > > + * @param mask CPU mask > > + * @param start_routine Thread start function > > + * @param arg Thread argument > > + * @param thr_type Thread type > > Please document thr_params and remove start_routine, arg, thr_type > Ooops! That passed through the rebase. Will fix in V5. Thx! > > > * > > * @return Number of threads created > > */ > > -int odph_linux_pthread_create(odph_linux_pthread_t *pthread_tbl, > > - const odp_cpumask_t *mask, > > - const odph_linux_thr_params_t *thr_params); > > +int odph_odpthreads_create(odph_odpthread_t *thread_tbl, > > + const odp_cpumask_t *mask, > > + const odph_odpthread_params_t *thr_params); > > > > /** > > - * Waits pthreads to exit > > + * Waits odpthreads (as linux threads or processes) to exit. > > * > > - * Returns when all threads have been exit. > > + * Returns when all odpthreads have terminated. > > * > > * @param thread_tbl Thread table > > - * @param num Number of threads to create > > - * > > - */ > > -void odph_linux_pthread_join(odph_linux_pthread_t *thread_tbl, int num); > > - > > - > > -/** > > - * Fork a process > > - * > > - * Forks and sets CPU affinity for the child process. Ignores 'start' > and 'arg' > > - * thread parameters. > > - * > > - * @param[out] proc Pointer to process state info (for output) > > - * @param cpu Destination CPU for the child process > > - * @param thr_params Linux helper thread parameters > > + * @return The number of joined threads or -1 on error. > > + * (error occurs if any of the start_routine return non-zero or if > > + * the thread join/process wait itself failed -e.g. as the result of a > kill) > > > > diff --git a/helper/linux.c b/helper/linux.c > > index 24e243b..dd6ab8b 100644 > > --- a/helper/linux.c > > +++ b/helper/linux.c > > @@ -1,4 +1,4 @@ > > -/* Copyright (c) 2013, Linaro Limited > > +/* Copyright (c) 2013, Linaro Limited > > Please revert this change > Wonder how I did that. fixed for V5. > > > * All rights reserved. > > * > > * SPDX-License-Identifier: BSD-3-Clause > > @@ -21,218 +21,368 @@ > > #include <odp/helper/linux.h> > > #include "odph_debug.h" > > > > -static void *odp_run_start_routine(void *arg) > > +static struct { > > + int proc; /* true when process mode is required */ > > + int thrd; /* true when thread mode is required */ > > +} helper_options; > > This is a boolean. Please avoid the use of global state for code like this. > bool: bool is C99 only and the usage of int as bool seems well spread within the code, but there spots with bool as well, I have to admit. But Getopt() expects an int* as third member of its option structure... do you really think I should use bool and then cast it to int for Getopt()?. My feeling is it is as good with int straight away global: isn't this the best solution to avoid polluting the caller space? (as main belongs to another space and you want the options to be visible for all the helpers. Any better approach is welcome > > > +/* > > + * wrapper for odpthreads, either implemented as linux threads or > processes. > > + * (in process mode, if start_routine returns NULL, the process return > 1. > > + */ > > +static void *odpthread_run_start_routine(void *arg) > > { > > - odph_linux_thr_params_t *thr_params = arg; > > + int status; > > + int ret; > > + odph_odpthread_params_t *thr_params; > > + > > + odph_odpthread_start_args_t *start_args = arg; > > + > > + thr_params = &start_args->thr_params; > > > > /* ODP thread local init */ > > if (odp_init_local(thr_params->instance, thr_params->thr_type)) { > > ODPH_ERR("Local init failed\n"); > > + if (start_args->linuxtype == PROCESS) > > + exit(-1); > > return NULL; > > } > > > > - void *ret_ptr = thr_params->start(thr_params->arg); > > - int ret = odp_term_local(); > > + /* debug: */ > > + printf("helper: ODP %s thread started as linux %s. (pid=%d)\n", > > + thr_params->thr_type == ODP_THREAD_WORKER ? "worker" : > "control", > > + (start_args->linuxtype == PTHREAD) ? "pthread" : "process", > > + (int)getpid()); > > Please use appropriate logging facilities instead of printf(). > fixed in V5 > > > + status = thr_params->start(thr_params->arg); > > + ret = odp_term_local(); > > > > if (ret < 0) > > ODPH_ERR("Local term failed\n"); > > else if (ret == 0 && odp_term_global(thr_params->instance)) > > ODPH_ERR("Global term failed\n"); > > > > - return ret_ptr; > > + /* for process implementation of odp threads, just return > status... */ > > + if (start_args->linuxtype == PROCESS) > > + exit(status); > > + > > + /* threads implementation return void* pointers: cast status to > that. */ > > Please avoid explicit cast. > Any other way to transform an int to a pointer which is what we need to do to be able to run the same code for processes (returning an int status) and a thread (returning a void*) ? > > > + return (void *)(long)status; > > } > > This review is 1% finished. > Thanks for looking at it anyway: I guess I should wait for V5...? I have fixed those things I said V5 on. other topics I discussed have not been changed, so either confirm that you understand and agree or come with suggestions. Once again many hx /Christophe
On 04/29 13:29:03, Christophe Milard wrote: > On 29 April 2016 at 00:48, Brian Brooks <brian.brooks@linaro.org> wrote: > > > On 04/28 17:18:36, Christophe Milard wrote: > > > Since v3: > > > -fixed rebase error (Christophe) > > > -rebased > > > > Thanks for the rebase. test_in_process_mode_v4 merged cleanly into > > origin/master and build and tests PASS. > > > > I've given this a look, and it appears we're headed in the right direction. > > > > > diff --git a/example/classifier/odp_classifier.c > > b/example/classifier/odp_classifier.c > > > index a477e23..4057457 100644 > > > --- a/example/classifier/odp_classifier.c > > > +++ b/example/classifier/odp_classifier.c > > > @@ -815,10 +802,16 @@ static void parse_args(int argc, char *argv[], > > appl_args_t *appl_args) > > > {NULL, 0, NULL, 0} > > > }; > > > > > > + static const char *shortopts = "+c:t:i:p:m:t:h"; > > > + > > > + /* let helper collect its own arguments (e.g. --odph_proc) */ > > > + odph_parse_options(argc, argv, shortopts, longopts); > > > + > > > + opterr = 0; /* do not issue errors on helper options */ > > > > Please use the default behavior of opterr _or_ add the case statement for > > '?' > > when appropriate. > > > > I do think we need opterr=0, sadly: when the test or example (such as the > classifier here) parses it options, the command line options being parsed > still contains all options, i.e. both the option meant for the classifier > itself, and the option meant to the "other layers", (here the helper). If > opt_err is left to 1, the classifier's call to get_opt() will issue errors > when hitting the helper options, even if we have a "?" in the switch, as > far as I understand. > So the mechanism taken here is: the caller (e.g. classifier) passes its > list of option to the helper which builds the complete list of options by > merging it own (helper) option list to the caller options. Then the helper > parse this complete list (with default opterr=1), i.e. reacting to unknown > options (not being in the merged list) and also picking up its own option > semantic, of course. After this the caller (here classifier) parse the > options again, picking up its own options only. but should not react on the > helper's otpions. > I Actually looked at removing the options from argv in the helpers, but > this turned up to be quite tricky as well. > parse_args() seem to be better at spitting command line option in different > owner, but is is not POSIX. not sure we want to insert that kind of > dependency for so little. OK, this should not block a contribution. > > > diff --git a/helper/include/odp/helper/linux.h > > b/helper/include/odp/helper/linux.h > > > index 7a6504f..a9ec90a 100644 > > > --- a/helper/include/odp/helper/linux.h > > > +++ b/helper/include/odp/helper/linux.h > > > @@ -25,111 +25,146 @@ extern "C" { > > > #include <odp_api.h> > > > > > > #include <pthread.h> > > > +#include <getopt.h> > > > > Please consider migrating CLI parsing via C library to a more appropriate > > location. > > > > Not sure I understand this: The goal is to have the helpers parsing their > own options here. Maybe my comment above makes it clearer. otherwise please > explain what you meant. I see. I'll attempt to explain further. Helper linux.[ch] files contain code for both threading and arg parsing. First, because threading and arg parsing are separate functional pieces of code they _should_ belong in separate files. Second, the linux.[ch] filename is misleading. Two simple examples: lib/thread.h lib/thread.c lib/thread_posix.c lib/thread_linux.c include/thread.h lib/thread.c lib/posix/thread.c Many ways, but the point is that common platform-independent code contained in one place, and abstraction occurs 'outward' or 'deeper' in the filename or directory structure. The filename and directory path corresponds to the code inside. helper/include/odp/helper/linux.h <- CLI code goes here. Too subjective? > > > > > #include <sys/types.h> > > > > > > -/** Thread parameter for Linux pthreads and processes */ > > > +/** odpthread linux type: whether an ODP thread is a linux thread or > > process */ > > > +typedef enum odph_odpthread_linuxtype_e { > > > + NOT_STARTED = 0, > > > + PROCESS, > > > + PTHREAD > > > +} odph_odpthread_linuxtype_t; > > > > Please use more descriptive identifiers in enums and consider a MAX if > > used for iteration. > > > > Any suggestion to any better names? What is meant here is: either the > odpthread has never been started (in which case it is neither a linux > process or a linux thread), or it is started as a linux process, or it is > started as a linux pthread. There are not used in iteration. > Maybe the confusion comes from what an odp thread is. I am not very keen on > the naming, but despite its name, an ODP thread is just a concurrent > execution task. On linux these can be implemented as pthreads or processes. > The name "odp tasks" would in my eyes be better (as not as correlated to > any specific implementation), but that was not accepted. First, this enum conflates ODP thread type & runtime state. Second, if an ODP thread is a concurrent execution task, why not name it that? I support this naming. ODPH_ODPTHREAD_PROCESS <- what is 'thread process'? ODPH_ODPTHREAD_THREAD <- what is 'thread thread'? <- don't forget 'thread zombie' Excruciatingly dumb, yet simple, example.. _not_ to be confused with real code! enum { ODP_TASK_TYPE_THREAD, ODP_TASK_TYPE_PROCESS }; enum { ODP_TASK_STATE_INIT, ODP_TASK_STATE_RUNNING, ODP_TASK_STATE_IDLE, ODP_TASK_STATE_TERMINATED }; struct odp_task { union { pid_t pid; pthread_t thread_id; }; int type; int state; }; Actually, please disregard this subjective example. > > > -/** Linux process state information */ > > > +/** Linux odpthread state information, used both in process or thread > > mode */ > > > typedef struct { > > > - pid_t pid; /**< Process ID */ > > > - int cpu; /**< CPU ID */ > > > - int status; /**< Process state change status */ > > > -} odph_linux_process_t; > > > + odph_odpthread_start_args_t start_args; > > > + int cpu; /**< CPU ID */ > > > + int last; /**< true if last table > > entry */ > > > + union { > > > + struct { /* for thread implementation */ > > > + pthread_t thread; /**< Pthread ID */ > > > > thread_id > > > > Ok. will take that in v5. > > > > > > > + pthread_attr_t attr; /**< Pthread attributes */ > > > > If threading affinity is determined at creation time, and never changes, > > please > > consider reducing the lifetime of this variable. > > > > I am not sure what to do here: I don't really understand why this > particular member is special here. The attribute themselves are > created/destroyed at thread creation/destruction time.This is just a > typedef, but indeed these structures (including all their members) are > today staticaly allocated in tests. I have not changed that from the old > code. I do not think this specific member is special and should be > separated from this structure: it really belongs to it. > > What should be done, I think, is reducing the lifetime of the whole > structure (allocating then at task/process creation time and freeing them > at join time.). > This can be addressed in another patch. OK > > > @@ -21,218 +21,368 @@ > > > #include <odp/helper/linux.h> > > > #include "odph_debug.h" > > > > > > -static void *odp_run_start_routine(void *arg) > > > +static struct { > > > + int proc; /* true when process mode is required */ > > > + int thrd; /* true when thread mode is required */ > > > +} helper_options; > > > > This is a boolean. Please avoid the use of global state for code like this. > > > > bool: bool is C99 only and the usage of int as bool seems well spread > within the code, but there spots with bool as well, I have to admit. But > Getopt() expects an int* as third member of its option structure... do you > really think I should use bool and then cast it to int for Getopt()?. My > feeling is it is as good with int straight away > global: isn't this the best solution to avoid polluting the caller space? > (as main belongs to another space and you want the options to be visible > for all the helpers. Any better approach is welcome What I mean to say is that if an ODP thread is either a thread or a process, this state does not need to be represented by two integers. And, this state should not be global if it is directly tied to the lifetime of an ODP thread. Further down in your patch series, there is a change to flip between spawning a thread or a process (every other ...) if CLI sets both thread and process mode. This is _not_ subjective, but if it lays the foundation for further work, then please carry on. > > > > > +/* > > > + * wrapper for odpthreads, either implemented as linux threads or > > processes. > > > + * (in process mode, if start_routine returns NULL, the process return > > 1. > > > + */ > > > +static void *odpthread_run_start_routine(void *arg) > > > { > > > - odph_linux_thr_params_t *thr_params = arg; > > > + int status; > > > + int ret; > > > + odph_odpthread_params_t *thr_params; > > > + > > > + odph_odpthread_start_args_t *start_args = arg; > > > + > > > + thr_params = &start_args->thr_params; > > > > > > /* ODP thread local init */ > > > if (odp_init_local(thr_params->instance, thr_params->thr_type)) { > > > ODPH_ERR("Local init failed\n"); > > > + if (start_args->linuxtype == PROCESS) > > > + exit(-1); > > > return NULL; > > > } > > > > > > - void *ret_ptr = thr_params->start(thr_params->arg); > > > - int ret = odp_term_local(); > > > + /* debug: */ > > > + printf("helper: ODP %s thread started as linux %s. (pid=%d)\n", > > > + thr_params->thr_type == ODP_THREAD_WORKER ? "worker" : > > "control", > > > + (start_args->linuxtype == PTHREAD) ? "pthread" : "process", > > > + (int)getpid()); > > > > Please use appropriate logging facilities instead of printf(). > > > > fixed in V5 > > > > > > > + status = thr_params->start(thr_params->arg); > > > + ret = odp_term_local(); > > > > > > if (ret < 0) > > > ODPH_ERR("Local term failed\n"); > > > else if (ret == 0 && odp_term_global(thr_params->instance)) > > > ODPH_ERR("Global term failed\n"); > > > > > > - return ret_ptr; > > > + /* for process implementation of odp threads, just return > > status... */ > > > + if (start_args->linuxtype == PROCESS) > > > + exit(status); > > > + > > > + /* threads implementation return void* pointers: cast status to > > that. */ > > > > Please avoid explicit cast. > > > > Any other way to transform an int to a pointer which is what we need to do > to be able to run the same code for processes (returning an int status) and > a thread (returning a void*) ? It is possible. But, it is faster to do the code. So, too subjective for this review. Please disregard. > > > > > + return (void *)(long)status; > > > } > > > > This review is 1% finished. > > > > Thanks for looking at it anyway: I guess I should wait for V5...? > I have fixed those things I said V5 on. other topics I discussed have not > been changed, so either confirm that you understand and agree or come with > suggestions. > Once again many hx > > /Christophe
Hi Brian I have commented below, but it looks we need to talk. would you have time after the linaro sync call today, for a HO? On 29 April 2016 at 23:49, Brian Brooks <brian.brooks@linaro.org> wrote: > On 04/29 13:29:03, Christophe Milard wrote: > > On 29 April 2016 at 00:48, Brian Brooks <brian.brooks@linaro.org> wrote: > > > > > On 04/28 17:18:36, Christophe Milard wrote: > > > > Since v3: > > > > -fixed rebase error (Christophe) > > > > -rebased > > > > > > Thanks for the rebase. test_in_process_mode_v4 merged cleanly into > > > origin/master and build and tests PASS. > > > > > > I've given this a look, and it appears we're headed in the right > direction. > > > > > > > diff --git a/example/classifier/odp_classifier.c > > > b/example/classifier/odp_classifier.c > > > > index a477e23..4057457 100644 > > > > --- a/example/classifier/odp_classifier.c > > > > +++ b/example/classifier/odp_classifier.c > > > > @@ -815,10 +802,16 @@ static void parse_args(int argc, char *argv[], > > > appl_args_t *appl_args) > > > > {NULL, 0, NULL, 0} > > > > }; > > > > > > > > + static const char *shortopts = "+c:t:i:p:m:t:h"; > > > > + > > > > + /* let helper collect its own arguments (e.g. --odph_proc) */ > > > > + odph_parse_options(argc, argv, shortopts, longopts); > > > > + > > > > + opterr = 0; /* do not issue errors on helper options */ > > > > > > Please use the default behavior of opterr _or_ add the case statement > for > > > '?' > > > when appropriate. > > > > > > > I do think we need opterr=0, sadly: when the test or example (such as the > > classifier here) parses it options, the command line options being parsed > > still contains all options, i.e. both the option meant for the classifier > > itself, and the option meant to the "other layers", (here the helper). If > > opt_err is left to 1, the classifier's call to get_opt() will issue > errors > > when hitting the helper options, even if we have a "?" in the switch, as > > far as I understand. > > So the mechanism taken here is: the caller (e.g. classifier) passes its > > list of option to the helper which builds the complete list of options by > > merging it own (helper) option list to the caller options. Then the > helper > > parse this complete list (with default opterr=1), i.e. reacting to > unknown > > options (not being in the merged list) and also picking up its own option > > semantic, of course. After this the caller (here classifier) parse the > > options again, picking up its own options only. but should not react on > the > > helper's otpions. > > I Actually looked at removing the options from argv in the helpers, but > > this turned up to be quite tricky as well. > > parse_args() seem to be better at spitting command line option in > different > > owner, but is is not POSIX. not sure we want to insert that kind of > > dependency for so little. > > OK, this should not block a contribution. > > > > > diff --git a/helper/include/odp/helper/linux.h > > > b/helper/include/odp/helper/linux.h > > > > index 7a6504f..a9ec90a 100644 > > > > --- a/helper/include/odp/helper/linux.h > > > > +++ b/helper/include/odp/helper/linux.h > > > > @@ -25,111 +25,146 @@ extern "C" { > > > > #include <odp_api.h> > > > > > > > > #include <pthread.h> > > > > +#include <getopt.h> > > > > > > Please consider migrating CLI parsing via C library to a more > appropriate > > > location. > > > > > > > Not sure I understand this: The goal is to have the helpers parsing their > > own options here. Maybe my comment above makes it clearer. otherwise > please > > explain what you meant. > > I see. I'll attempt to explain further. > > Helper linux.[ch] files contain code for both threading and arg parsing. > First, > because threading and arg parsing are separate functional pieces of code > they > _should_ belong in separate files. Second, the linux.[ch] filename is > misleading. Two simple examples: > > lib/thread.h > lib/thread.c > lib/thread_posix.c > lib/thread_linux.c > > include/thread.h > lib/thread.c > lib/posix/thread.c > > Many ways, but the point is that common platform-independent code contained > in one place, and abstraction occurs 'outward' or 'deeper' in the filename > or directory structure. > > The filename and directory path corresponds to the code inside. > > helper/include/odp/helper/linux.h <- CLI code goes here. Too subjective? > hmm, To start with, I will keep using the term "ODP thread", even if I don't like it more than you, because that is the accepted term here. It is important we talk the same language, all of us. You are welcome to alias it to "ODPtask" on your head, or argue for a name change (which I alreay tried !), but we should be using the same accepted term everywhere, and currently, it is "ODPthread". I think our disagreement mostly comes from the fact that "helpers" is not a very well defined thing. And interestingly, your comments match the feelings I had when I started... There must be some good in them :-) ODP is a strange thing when it comes to "ODPthreads": The problem being that ODP tries not to redefine what the OS provides, but somehow needs to: ODP provides mechanism to block odpthreads (e.g. barriers, scheduler), to wake them up (scheduler, on event reception), but ODP tries not to define how an ODP thread is implemented (which is supposed to be an OS matter). This situation gives me a head-hake too! I have personally the feeling that the odpthread creation will eventually have to be accepted as fully part of ODP and that ODP will do whatever is needed towards the underlying OS. But today the odpthread creation ends up either directly in the application (calling the OS) or in the helpers (for those things most application will keep doing). So the distinction thread vs process makes sense within linux. For the helper (for the time being), we are just creating ODP threads, so I do not think this should be splitted into 2 files, even if the odpthread creation itself may end-up calling two different OS system calls at the end. This patch series tries to address a single issue: We have claimed that ODP threads, on linux, could be either linux threads or linux processes. But the process case has mostly been ignored and is very broken. Before this patch series, the helper provided 2 sets of functions: one for creating odpthreads as linux threads, and the other for creating the odpthreads as linux processes. The latter set was in practical not used. so the "process mode" that we claim to support was not tested. This patch series just factorizes these 2 sets in one, letting the helper decide whether odpthreads should be linux processes or linux threads (based on options), hence giving the chance to all test/examples that only previously ran odpthreads as linux threads to now run as linux threads as well. (and mostly fails). I personnaly feel that this will change in the future and that ODP threads creation might well become a part of ODP. I think we need a HO call :-) Separating the command line parsing from the rest of the help makes more sense to me. But as the only option being parsed are the two being introduced here, I am not sure such a split makes sense now. Any other opinions? > > > > > > > > #include <sys/types.h> > > > > > > > > -/** Thread parameter for Linux pthreads and processes */ > > > > +/** odpthread linux type: whether an ODP thread is a linux thread or > > > process */ > > > > +typedef enum odph_odpthread_linuxtype_e { > > > > + NOT_STARTED = 0, > > > > + PROCESS, > > > > + PTHREAD > > > > +} odph_odpthread_linuxtype_t; > > > > > > Please use more descriptive identifiers in enums and consider a MAX if > > > used for iteration. > > > > > > > Any suggestion to any better names? What is meant here is: either the > > odpthread has never been started (in which case it is neither a linux > > process or a linux thread), or it is started as a linux process, or it is > > started as a linux pthread. There are not used in iteration. > > Maybe the confusion comes from what an odp thread is. I am not very keen > on > > the naming, but despite its name, an ODP thread is just a concurrent > > execution task. On linux these can be implemented as pthreads or > processes. > > The name "odp tasks" would in my eyes be better (as not as correlated to > > any specific implementation), but that was not accepted. > > First, this enum conflates ODP thread type & runtime state. Second, if an > ODP thread is a concurrent execution task, why not name it that? I support > this naming. > Yes and no: your are right in the sense that the name conflates 2 things: the type,and the thread existance (not its state). Maybe it should be: NONE, PROCESS and THREADS, "NONE" being for the odpthreads not being started, and that are hence neither processes nor threads. Note that those odpthreads not being started, are not going to be started either: This just tells that this entry in the table corresponds to a odpthread whose creation failed, and therefore no attempt to join/wait for the underlying thread/process should be made. We are not talking about the usual process state (running/wait/... state) > ODPH_ODPTHREAD_PROCESS <- what is 'thread process'? > ODPH_ODPTHREAD_THREAD <- what is 'thread thread'? > <- don't forget 'thread zombie' > The ODPH prefix should be reserved for things belonging to the helper interface. You will argue with reason that these are defined in the interface definition file (linux.h). I have kept the same structure as previously in this patch, keeping the table struct definition in "linux.h". I think the table definition should actually be opaque to the user and defined in linux.c (as it is local to it). I actually have a patch doing that ready. So rather than prefixing private things with ODPH, they are moved in linux.c. But as said, even if this code looks new, it is actually a merge of two of sructures (one for linux and one for threads) defined just above in the same file. I can howerver can it to ODPTHREAD_PROCESS and ODPTHREAD_PROCESS if yu prefer. > > Excruciatingly dumb, yet simple, example.. _not_ to be confused with real > code! > > enum { > ODP_TASK_TYPE_THREAD, > ODP_TASK_TYPE_PROCESS > }; > enum { > ODP_TASK_STATE_INIT, > ODP_TASK_STATE_RUNNING, > ODP_TASK_STATE_IDLE, > ODP_TASK_STATE_TERMINATED > }; > No: these are states for the OS. (and possibly for ODP in some future ?). but not for the helpers. > struct odp_task { > union { > pid_t pid; > pthread_t thread_id; > }; > int type; > int state; > }; > > Actually, please disregard this subjective example. > > > > > -/** Linux process state information */ > > > > +/** Linux odpthread state information, used both in process or > thread > > > mode */ > > > > typedef struct { > > > > - pid_t pid; /**< Process ID */ > > > > - int cpu; /**< CPU ID */ > > > > - int status; /**< Process state change status */ > > > > -} odph_linux_process_t; > > > > + odph_odpthread_start_args_t start_args; > > > > + int cpu; /**< CPU ID */ > > > > + int last; /**< true if last table > > > entry */ > > > > + union { > > > > + struct { /* for thread implementation */ > > > > + pthread_t thread; /**< Pthread ID */ > > > > > > thread_id > > > > > > > Ok. will take that in v5. > > > > > > > > > > > + pthread_attr_t attr; /**< Pthread > attributes */ > > > > > > If threading affinity is determined at creation time, and never > changes, > > > please > > > consider reducing the lifetime of this variable. > > > > > > > I am not sure what to do here: I don't really understand why this > > particular member is special here. The attribute themselves are > > created/destroyed at thread creation/destruction time.This is just a > > typedef, but indeed these structures (including all their members) are > > today staticaly allocated in tests. I have not changed that from the old > > code. I do not think this specific member is special and should be > > separated from this structure: it really belongs to it. > > > > What should be done, I think, is reducing the lifetime of the whole > > structure (allocating then at task/process creation time and freeing them > > at join time.). > > This can be addressed in another patch. > > OK > > > > > @@ -21,218 +21,368 @@ > > > > #include <odp/helper/linux.h> > > > > #include "odph_debug.h" > > > > > > > > -static void *odp_run_start_routine(void *arg) > > > > +static struct { > > > > + int proc; /* true when process mode is required */ > > > > + int thrd; /* true when thread mode is required */ > > > > +} helper_options; > > > > > > This is a boolean. Please avoid the use of global state for code like > this. > > > > > > > bool: bool is C99 only and the usage of int as bool seems well spread > > within the code, but there spots with bool as well, I have to admit. But > > Getopt() expects an int* as third member of its option structure... do > you > > really think I should use bool and then cast it to int for Getopt()?. My > > feeling is it is as good with int straight away > > global: isn't this the best solution to avoid polluting the caller space? > > (as main belongs to another space and you want the options to be visible > > for all the helpers. Any better approach is welcome > > What I mean to say is that if an ODP thread is either a thread or a > process, > this state does not need to be represented by two integers. And, this state > should not be global if it is directly tied to the lifetime of an ODP > thread. > I agree this 2 ints really carries bool info, but as said getopt() expects int*, not bool*. And this structure should live as long as the application lives (its lifetime is not connected to the odpthread lifetime): These flags tells whether any odpthread beeing started -at any time- should be started as linux process thread/process) > Further down in your patch series, there is a change to flip between > spawning > a thread or a process (every other ...) if CLI sets both thread and process > mode. This is _not_ subjective, but if it lays the foundation for further > work, > then please carry on. > Not sure what you mean here: we need to talk ! :-) > > > > > > > > +/* > > > > + * wrapper for odpthreads, either implemented as linux threads or > > > processes. > > > > + * (in process mode, if start_routine returns NULL, the process > return > > > 1. > > > > + */ > > > > +static void *odpthread_run_start_routine(void *arg) > > > > { > > > > - odph_linux_thr_params_t *thr_params = arg; > > > > + int status; > > > > + int ret; > > > > + odph_odpthread_params_t *thr_params; > > > > + > > > > + odph_odpthread_start_args_t *start_args = arg; > > > > + > > > > + thr_params = &start_args->thr_params; > > > > > > > > /* ODP thread local init */ > > > > if (odp_init_local(thr_params->instance, > thr_params->thr_type)) { > > > > ODPH_ERR("Local init failed\n"); > > > > + if (start_args->linuxtype == PROCESS) > > > > + exit(-1); > > > > return NULL; > > > > } > > > > > > > > - void *ret_ptr = thr_params->start(thr_params->arg); > > > > - int ret = odp_term_local(); > > > > + /* debug: */ > > > > + printf("helper: ODP %s thread started as linux %s. (pid=%d)\n", > > > > + thr_params->thr_type == ODP_THREAD_WORKER ? "worker" : > > > "control", > > > > + (start_args->linuxtype == PTHREAD) ? "pthread" : > "process", > > > > + (int)getpid()); > > > > > > Please use appropriate logging facilities instead of printf(). > > > > > > > fixed in V5 > > > > > > > > > > > + status = thr_params->start(thr_params->arg); > > > > + ret = odp_term_local(); > > > > > > > > if (ret < 0) > > > > ODPH_ERR("Local term failed\n"); > > > > else if (ret == 0 && odp_term_global(thr_params->instance)) > > > > ODPH_ERR("Global term failed\n"); > > > > > > > > - return ret_ptr; > > > > + /* for process implementation of odp threads, just return > > > status... */ > > > > + if (start_args->linuxtype == PROCESS) > > > > + exit(status); > > > > + > > > > + /* threads implementation return void* pointers: cast status to > > > that. */ > > > > > > Please avoid explicit cast. > > > > > > > Any other way to transform an int to a pointer which is what we need to > do > > to be able to run the same code for processes (returning an int status) > and > > a thread (returning a void*) ? > > It is possible. But, it is faster to do the code. So, too subjective for > this > review. Please disregard. > > > > > > > > + return (void *)(long)status; > > > > } > > > > > > This review is 1% finished. > > > > > > > Thanks for looking at it anyway: I guess I should wait for V5...? > > I have fixed those things I said V5 on. other topics I discussed have not > > been changed, so either confirm that you understand and agree or come > with > > suggestions. > > Once again many hx > > > > /Christophe > /Christophe.
On 05/02 11:18:19, Christophe Milard wrote: > Hi Brian > > I have commented below, but it looks we need to talk. would you have time > after the linaro sync call today, for a HO? > Hi Christophe, Remaining comments from 'helper_abstraction_odp_thread' following our discussion. Brian > diff --git a/helper/linux.c b/helper/linux.c > index 24e243b..2714e51 100644 > --- a/helper/linux.c > +++ b/helper/linux.c > +/* > + * wrapper for odpthreads, either implemented as linux threads or processes. > + * (in process mode, if start_routine returns NULL, the process return 1. > + */ > +static void *odpthread_run_start_routine(void *arg) > { > - odph_linux_thr_params_t *thr_params = arg; > + int status; > + int ret; > + odph_odpthread_params_t *thr_params; > + > + _odph_odpthread_start_args_t *start_args = arg; > + > + thr_params = &start_args->thr_params; > > /* ODP thread local init */ > if (odp_init_local(thr_params->instance, thr_params->thr_type)) { > ODPH_ERR("Local init failed\n"); > + if (start_args->linuxtype == PROCESS) > + exit(-1); We discussed -1 return value. However, _exit() vs exit() is relevant but beyond the scope of this patch series. > return NULL; > } > > - void *ret_ptr = thr_params->start(thr_params->arg); > - int ret = odp_term_local(); > + ODPH_DBG("helper: ODP %s thread started as linux %s. (pid=%d)\n", > + thr_params->thr_type == ODP_THREAD_WORKER ? > + "worker" : "control", > + (start_args->linuxtype == PTHREAD) ? > + "pthread" : "process", > + (int)getpid()); for extra sanity, perhaps: (start_args->linuxtype == PTHREAD) ? (int)pthread_self() : (int)getpid() A potential RFC to logging facilities (beyond scope of this series) would include pid and thread_id in log format alongside timestamp. > + status = thr_params->start(thr_params->arg); > + ret = odp_term_local(); > > if (ret < 0) > ODPH_ERR("Local term failed\n"); > else if (ret == 0 && odp_term_global(thr_params->instance)) > ODPH_ERR("Global term failed\n"); > > - return ret_ptr; > + /* for process implementation of odp threads, just return status... */ > + if (start_args->linuxtype == PROCESS) > + exit(status); > + > + /* threads implementation return void* pointers: cast status to that. */ > + return (void *)(long)status; Seems the cast to long is unnecessary? > } > > -int odph_linux_pthread_create(odph_linux_pthread_t *pthread_tbl, > - const odp_cpumask_t *mask, > - const odph_linux_thr_params_t *thr_params) > +/* > + * Create a single ODPthread as a linux process > + */ > +static int odph_linux_process_create(_odph_odpthread_t *thread_tbl, > + int cpu, > + const odph_odpthread_params_t *thr_params) > { > - int i; > - int num; > - int cpu_count; > - int cpu; > int ret; > + cpu_set_t cpu_set; > + pid_t pid; > > - num = odp_cpumask_count(mask); > + CPU_ZERO(&cpu_set); > + CPU_SET(cpu, &cpu_set); > > - memset(pthread_tbl, 0, num * sizeof(odph_linux_pthread_t)); > + thread_tbl->start_args.thr_params = *thr_params; /* copy */ > + thread_tbl->start_args.linuxtype = PROCESS; > + thread_tbl->cpu = cpu; > > - cpu_count = odp_cpu_count(); > + pid = fork(); > + if (pid < 0) { > + ODPH_ERR("fork() failed\n"); > + thread_tbl->start_args.linuxtype = NOT_STARTED; > + return -1; > + } > > - if (num < 1 || num > cpu_count) { > - ODPH_ERR("Invalid number of threads:%d (%d cores available)\n", > - num, cpu_count); > + /* Parent continues to fork */ > + if (pid > 0) { > + thread_tbl->proc.pid = pid; > return 0; > } > > - cpu = odp_cpumask_first(mask); > - for (i = 0; i < num; i++) { > - cpu_set_t cpu_set; > + /* Child process */ > > - CPU_ZERO(&cpu_set); > - CPU_SET(cpu, &cpu_set); > + /* Request SIGTERM if parent dies */ > + prctl(PR_SET_PDEATHSIG, SIGTERM); OK. Seems only TM example doing basic sig handling. > + /* Parent died already? */ > + if (getppid() == 1) > + kill(getpid(), SIGTERM); OK. > - pthread_attr_init(&pthread_tbl[i].attr); > + if (sched_setaffinity(0, sizeof(cpu_set_t), &cpu_set)) { > + ODPH_ERR("sched_setaffinity() failed\n"); > + return -2; OK. > + } > > - pthread_tbl[i].cpu = cpu; > + odpthread_run_start_routine(&thread_tbl->start_args); > > - pthread_attr_setaffinity_np(&pthread_tbl[i].attr, > - sizeof(cpu_set_t), &cpu_set); > + return 0; /* never reached */ > +} > > - pthread_tbl[i].thr_params.start = thr_params->start; > - pthread_tbl[i].thr_params.arg = thr_params->arg; > - pthread_tbl[i].thr_params.thr_type = thr_params->thr_type; > - pthread_tbl[i].thr_params.instance = thr_params->instance; > +/* > + * Create a single ODPthread as a linux thread > + */ > +static int odph_linux_thread_create(_odph_odpthread_t *thread_tbl, > + int cpu, > + const odph_odpthread_params_t *thr_params) > +{ > + int ret; > + cpu_set_t cpu_set; > > - ret = pthread_create(&pthread_tbl[i].thread, > - &pthread_tbl[i].attr, > - odp_run_start_routine, > - &pthread_tbl[i].thr_params); > - if (ret != 0) { > - ODPH_ERR("Failed to start thread on cpu #%d\n", cpu); > - break; > - } > + CPU_ZERO(&cpu_set); > + CPU_SET(cpu, &cpu_set); > > - cpu = odp_cpumask_next(mask, cpu); > - } > + pthread_attr_init(&thread_tbl->thread.attr); > > - return i; > -} > + thread_tbl->cpu = cpu; > > -void odph_linux_pthread_join(odph_linux_pthread_t *thread_tbl, int num) > -{ > - int i; > - int ret; > + pthread_attr_setaffinity_np(&thread_tbl->thread.attr, > + sizeof(cpu_set_t), &cpu_set); > > - for (i = 0; i < num; i++) { > - /* Wait thread to exit */ > - ret = pthread_join(thread_tbl[i].thread, NULL); > - if (ret != 0) { > - ODPH_ERR("Failed to join thread from cpu #%d\n", > - thread_tbl[i].cpu); > - } > - pthread_attr_destroy(&thread_tbl[i].attr); > + thread_tbl->start_args.thr_params = *thr_params; /* copy */ > + thread_tbl->start_args.linuxtype = PTHREAD; > + > + ret = pthread_create(&thread_tbl->thread.thread_id, > + &thread_tbl->thread.attr, > + odpthread_run_start_routine, > + &thread_tbl->start_args); > + if (ret != 0) { > + ODPH_ERR("Failed to start thread on cpu #%d\n", cpu); > + thread_tbl->start_args.linuxtype = NOT_STARTED; > + return ret; > } > + > + return 0; > } > > -int odph_linux_process_fork_n(odph_linux_process_t *proc_tbl, > - const odp_cpumask_t *mask, > - const odph_linux_thr_params_t *thr_params) > +/* > + * create an odpthread set (as linux processes or linux threads or both) > + */ > +int odph_odpthreads_create(odph_odpthread_tbl_t *thread_tbl_ptr, > + const odp_cpumask_t *mask, > + const odph_odpthread_params_t *thr_params) > { > - pid_t pid; > + int i; > int num; > int cpu_count; > int cpu; > - int i; > + _odph_odpthread_t *thread_tbl; > > num = odp_cpumask_count(mask); > > - memset(proc_tbl, 0, num * sizeof(odph_linux_process_t)); > + thread_tbl = malloc(num * sizeof(_odph_odpthread_t)); > + *thread_tbl_ptr = (void *)thread_tbl; > + > + memset(thread_tbl, 0, num * sizeof(_odph_odpthread_t)); > > cpu_count = odp_cpu_count(); > > if (num < 1 || num > cpu_count) { > - ODPH_ERR("Bad num\n"); > + ODPH_ERR("Invalid number of odpthreads:%d" > + " (%d cores available)\n", > + num, cpu_count); > return -1; > } > > cpu = odp_cpumask_first(mask); > for (i = 0; i < num; i++) { > - cpu_set_t cpu_set; > + /* > + * Thread mode by default, or if both thread and proc mode > + * are required each second odpthread is a linux thread. > + */ > + if ((!helper_options.proc) || > + (helper_options.proc && helper_options.thrd && (i & 1))) { > + if (odph_linux_thread_create(&thread_tbl[i], > + cpu, > + thr_params)) > + break; > + } else { > + if (odph_linux_process_create(&thread_tbl[i], > + cpu, > + thr_params)) > + break; > + } > > - CPU_ZERO(&cpu_set); > - CPU_SET(cpu, &cpu_set); > + cpu = odp_cpumask_next(mask, cpu); > + } > + thread_tbl[num - 1].last = 1; > - pid = fork(); > + return i; > +} > > - if (pid < 0) { > - ODPH_ERR("fork() failed\n"); > - return -1; > - } > +/* > + * wait for the odpthreads terminaison (linux processes and threads) parlez-vous francais? > + */ > +int odph_odpthreads_join(odph_odpthread_tbl_t *thread_tbl_ptr) > +{ > + _odph_odpthread_t *thread_tbl; > + pid_t pid; > + int i = 0; > + int status = 0; /* child process return code (!=0 is error) */ > + void *thread_ret; /* "child" thread return code (NULL is error) */ > + int ret; > + int retval = 0; > > - /* Parent continues to fork */ > - if (pid > 0) { > - proc_tbl[i].pid = pid; > - proc_tbl[i].cpu = cpu; > + thread_tbl = (_odph_odpthread_t *)*thread_tbl_ptr; > + > + if (!thread_tbl) { > + ODPH_ERR("Attempt to join thread from invalid table\n"); > + return -1; > + } > > - cpu = odp_cpumask_next(mask, cpu); > + /* joins linux threads or wait for processes */ > + do { Since 'last' is set for the last element in the array, it is possible to run across some elements which are NOT_STARTED. Perhaps you could switch() against the 'linuxtype' field and handle NOT_STARTED by doing nothing and logging a warning? > + /* pthreads: */ > + if (thread_tbl[i].start_args.linuxtype == PTHREAD) { Another subjective comment beyond the scope of this patch series is to have a thread checker utility for asserting that create/join calls are amde from same or master pid/thread, otherwise ... > + /* Wait thread to exit */ > + ret = pthread_join(thread_tbl[i].thread.thread_id, > + &thread_ret); > + if (ret != 0) { > + ODPH_ERR("Failed to join thread from cpu #%d\n", > + thread_tbl[i].cpu); > + retval = -1; > + } else { > + if (thread_ret != NULL) > + retval = -1; > + } > + pthread_attr_destroy(&thread_tbl[i].thread.attr); > continue; > } > > - /* Child process */ > + /* processes: */ > + pid = waitpid(thread_tbl[i].proc.pid, &status, 0); > > - /* Request SIGTERM if parent dies */ > - prctl(PR_SET_PDEATHSIG, SIGTERM); > - /* Parent died already? */ > - if (getppid() == 1) > - kill(getpid(), SIGTERM); > + if (pid < 0) { > + ODPH_ERR("wait() failed\n"); waitpid() failed > + retval = -1; > + } > > - if (sched_setaffinity(0, sizeof(cpu_set_t), &cpu_set)) { > - ODPH_ERR("sched_setaffinity() failed\n"); > - return -2; > + /* Examine the child process' termination status */ > + if (WIFEXITED(status) && WEXITSTATUS(status) != EXIT_SUCCESS) { We're assuming that an ODP thread 'start' routine will return 0 / EXIT_SUCCESS _and_ will return in the first place--no killing in this join. > + ODPH_ERR("Child exit status:%d (pid:%d)\n", > + WEXITSTATUS(status), (int)pid); > + retval = -1; > } > + if (WIFSIGNALED(status)) { > + int signo = WTERMSIG(status); > > - if (odp_init_local(thr_params->instance, > - thr_params->thr_type)) { > - ODPH_ERR("Local init failed\n"); > - return -2; > + ODPH_ERR("Child term signo:%d - %s (pid:%d)\n", > + signo, strsignal(signo), (int)pid); > + retval = -1; OK. Nevermind. :) > } > > - return 0; > - } > + } while (!thread_tbl[i++].last); > > - return 1; > + /* free the allocated table: */ > + free(*thread_tbl_ptr); > + > + return (retval < 0) ? retval : i; > }
Since v3: -fixed rebase error (Christophe) -rebased Since v2: -serious rebase following clash with dba05a28 (api: init: add instance handle) -squashing the validation changes. I am not very keen on squashing small isolated modifs to larger patches. don't really see the gain. Squashing more will put even more unrelated things together. Since v1: -variable declaration gathered in function 's head (Petri) -linux prefix removed from helper's types and function names (Mike, Christophe) Hi, This patch series adds the ability to run tests/ exemples / perf-test in "process mode" (i.e letting OPD threads being linux processes) It it hence tackling ODP-171. This is achieved in 2 main steps: A] The 2 pairs of helper functions: odph_linux_pthread_create(), odph_linux_pthread_join() and odph_linux_process_fork_n(), odph_linux_process_wait_n() are replaced by: odph_linux_odpthreads_create() and odph_linux_odpthreads_join() The latter's callers are unaware of the actual implementation of the ODP thread (making test truly platform agnostic). The helper functions decide at run time whether an odp thread is to be a linux process or pthread based on the command line argument. B] each test/example now calls a new helper function, odph_linux_parse_options(), so that the helper can get its own arguments out of the command line. Currentely supported args are: --odph_proc, --odph_thread. Defaults assumes thread. specifying both options runs in mixed mode. The changed are first done on the shmem tests, and thereafter propagated to other helper users. Note that this patch series enable the option but does not affect make check at this time: make check still calls the tests with no options which default to thread mode. This patch series nicely splits it two groups (you can split your review there): 1) up to "validation: pktio: adding command line argument parsing", the new helper functions are introduced, and used in the validation tests. 2) from "helper: adding a function to merge getopt parameters" the ability to parse command line arguments in subset in added and applied to the example and performance tests. Hope this makes sence for you too! ---- The following changes since commit 4834b40e7840fd23b8481b04dfff50d8fd0e1699: configure: fix broken conditional example execution (2016-04-27 16:51:38 +0300) are available in the git repository at: https://git.linaro.org/people/christophe.milard/odp.git test_in_process_mode_v4 for you to fetch changes up to d833f72880758a128777785775847a6bda558253: helper: removing dead code (2016-04-28 17:04:14 +0200) ---------------------------------------------------------------- Christophe Milard (36): helpers: adding command line argument parsing validation: common: adding command line argument parsing validation: shmem: adding command line argument parsing helpers: linux: creating common entry for process and thread helpers: linux: creating functions to handle odpthreads helper: test: adding odpthread functions tests validation: using implementation agnostic function for ODP threads validation: most tests: adding command line argument parsing validation: init: adding command line argument parsing validation: pktio: adding command line argument parsing helper: adding a function to merge getopt parameters helper: parsing the complete set of options performance: odp_scheduling: proc mode done by helper performance: odp_pktio_perf: using agnostic function for ODP threads performance: odp_pktio_perf: adding helper cmd line parsing performance: odp_l2fwd: using agnostic function for ODP threads performance: odp_l2fwd: adding helper cmd line parsing performance: crypto: using agnostic function for ODP threads performance: crypto: adding helper cmd line parsing example: classifier: using agnostic function for ODP threads example: classifier: adding helper cmd line parsing example: generator: using agnostic function for ODP threads example: generator: adding helper cmd line parsing example: ipsec: using agnostic function for ODP threads example: ipsec: adding helper cmd line parsing example: l2fwd_simple: using agnostic function for ODP threads example: l2fwd_simple: adding helper cmd line parsing example: pktio: using agnostic function for ODP threads example: pktio: adding helper cmd line parsing example: time: using agnostic function for ODP threads example: time: adding helper cmd line parsing example: timer: using agnostic function for ODP threads example: timer: adding helper cmd line parsing example: switch: using agnostic function for ODP threads example: switch: adding helper cmd line parsing helper: removing dead code example/classifier/odp_classifier.c | 39 +- example/generator/odp_generator.c | 45 ++- example/ipsec/odp_ipsec.c | 27 +- example/l2fwd_simple/odp_l2fwd_simple.c | 48 ++- example/packet/odp_pktio.c | 45 ++- example/switch/odp_switch.c | 26 +- example/time/time_global_test.c | 17 +- example/timer/odp_timer_test.c | 26 +- helper/include/odp/helper/linux.h | 183 +++++---- helper/linux.c | 414 ++++++++++++++------- helper/test/.gitignore | 3 +- helper/test/Makefile.am | 14 +- helper/test/{thread.c => odpthreads.c} | 33 +- helper/test/odpthreads_as_mixed | 15 + helper/test/odpthreads_as_processes | 15 + helper/test/odpthreads_as_pthreads | 15 + helper/test/process.c | 92 ----- platform/linux-generic/test/pktio/pktio_run | 21 +- platform/linux-generic/test/pktio/pktio_run_dpdk | 17 +- platform/linux-generic/test/pktio/pktio_run_netmap | 5 +- platform/linux-generic/test/pktio/pktio_run_pcap | 5 +- platform/linux-generic/test/pktio/pktio_run_tap | 6 +- test/performance/odp_crypto.c | 24 +- test/performance/odp_l2fwd.c | 37 +- test/performance/odp_pktio_perf.c | 34 +- test/performance/odp_scheduling.c | 91 ++--- test/validation/atomic/atomic.c | 40 +- test/validation/atomic/atomic.h | 2 +- test/validation/atomic/atomic_main.c | 4 +- test/validation/barrier/barrier.c | 14 +- test/validation/barrier/barrier.h | 2 +- test/validation/barrier/barrier_main.c | 4 +- test/validation/buffer/buffer.c | 10 +- test/validation/buffer/buffer.h | 2 +- test/validation/buffer/buffer_main.c | 4 +- test/validation/classification/classification.c | 10 +- test/validation/classification/classification.h | 2 +- .../classification/classification_main.c | 4 +- test/validation/common/odp_cunit_common.c | 24 +- test/validation/common/odp_cunit_common.h | 6 +- test/validation/config/config.c | 10 +- test/validation/config/config.h | 2 +- test/validation/config/config_main.c | 4 +- test/validation/cpumask/cpumask.c | 10 +- test/validation/cpumask/cpumask.h | 2 +- test/validation/cpumask/cpumask_main.c | 4 +- test/validation/crypto/crypto.c | 6 +- test/validation/crypto/crypto.h | 2 +- test/validation/crypto/crypto_main.c | 4 +- test/validation/errno/errno.c | 10 +- test/validation/errno/errno.h | 2 +- test/validation/errno/errno_main.c | 4 +- test/validation/hash/hash.c | 10 +- test/validation/hash/hash.h | 2 +- test/validation/hash/hash_main.c | 4 +- test/validation/init/init.c | 18 +- test/validation/init/init.h | 6 +- test/validation/init/init_main_abort.c | 4 +- test/validation/init/init_main_log.c | 4 +- test/validation/init/init_main_ok.c | 4 +- test/validation/lock/lock.c | 50 +-- test/validation/lock/lock.h | 2 +- test/validation/lock/lock_main.c | 4 +- test/validation/packet/packet.c | 10 +- test/validation/packet/packet.h | 2 +- test/validation/packet/packet_main.c | 4 +- test/validation/pktio/pktio.c | 10 +- test/validation/pktio/pktio.h | 2 +- test/validation/pktio/pktio_main.c | 4 +- test/validation/pool/pool.c | 10 +- test/validation/pool/pool.h | 2 +- test/validation/pool/pool_main.c | 4 +- test/validation/queue/queue.c | 10 +- test/validation/queue/queue.h | 2 +- test/validation/queue/queue_main.c | 4 +- test/validation/random/random.c | 10 +- test/validation/random/random.h | 2 +- test/validation/random/random_main.c | 4 +- test/validation/scheduler/scheduler.c | 18 +- test/validation/scheduler/scheduler.h | 2 +- test/validation/scheduler/scheduler_main.c | 4 +- test/validation/shmem/shmem.c | 16 +- test/validation/shmem/shmem.h | 2 +- test/validation/shmem/shmem_main.c | 4 +- test/validation/std_clib/std_clib.c | 10 +- test/validation/std_clib/std_clib.h | 2 +- test/validation/std_clib/std_clib_main.c | 4 +- test/validation/system/system.c | 10 +- test/validation/system/system.h | 2 +- test/validation/system/system_main.c | 4 +- test/validation/thread/thread.c | 14 +- test/validation/thread/thread.h | 2 +- test/validation/thread/thread_main.c | 4 +- test/validation/time/time.c | 10 +- test/validation/time/time.h | 2 +- test/validation/time/time_main.c | 4 +- test/validation/timer/timer.c | 10 +- test/validation/timer/timer.h | 2 +- test/validation/timer/timer_main.c | 4 +- test/validation/traffic_mngr/traffic_mngr.c | 6 +- test/validation/traffic_mngr/traffic_mngr.h | 2 +- test/validation/traffic_mngr/traffic_mngr_main.c | 4 +- 102 files changed, 1097 insertions(+), 718 deletions(-) rename helper/test/{thread.c => odpthreads.c} (71%) create mode 100755 helper/test/odpthreads_as_mixed create mode 100755 helper/test/odpthreads_as_processes create mode 100755 helper/test/odpthreads_as_pthreads delete mode 100644 helper/test/process.c