| Message ID | 1464181929-35564-1-git-send-email-christophe.milard@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
On 25 May 2016 at 21:12, Christophe Milard <christophe.milard@linaro.org> wrote: > The odph_odpthread_t (non opaque array of odpthread) is now replaced by a > single type odph_odpthread_tbl_t abstracted as a void*. > The table describing the odpthreads being created is now malloc'd and > freed by the helper function themselves. > > Signed-off-by: Christophe Milard <christophe.milard@linaro.org> > --- > example/classifier/odp_classifier.c | 9 ++--- > example/generator/odp_generator.c | 5 +-- > example/ipsec/odp_ipsec.c | 6 +-- > example/l2fwd_simple/odp_l2fwd_simple.c | 2 +- > example/packet/odp_pktio.c | 5 +-- > example/switch/odp_switch.c | 4 +- > example/time/time_global_test.c | 6 +-- > example/timer/odp_timer_test.c | 8 ++-- > helper/include/odp/helper/linux.h | 35 ++---------------- > helper/linux.c | 61 > +++++++++++++++++++++++++++---- > helper/test/odpthreads.c | 6 +-- > test/performance/odp_crypto.c | 8 ++-- > test/performance/odp_l2fwd.c | 4 +- > test/performance/odp_pktio_perf.c | 15 ++++---- > test/performance/odp_scheduling.c | 8 ++-- > test/validation/common/odp_cunit_common.c | 6 +-- > 16 files changed, 93 insertions(+), 95 deletions(-) > > diff --git a/example/classifier/odp_classifier.c > b/example/classifier/odp_classifier.c > index 20e64ec..7512ec6 100644 > --- a/example/classifier/odp_classifier.c > +++ b/example/classifier/odp_classifier.c > @@ -469,7 +469,7 @@ static void configure_cos(odp_cos_t default_cos, > appl_args_t *args) > */ int main(int argc, char *argv[]) > { > - odph_odpthread_t thread_tbl[MAX_WORKERS]; > + odph_odpthread_tbl_t thread_tbl; > odp_pool_t pool; > int num_workers; > int i; > @@ -562,21 +562,18 @@ int main(int argc, char *argv[]) > exit(EXIT_FAILURE); > } > > - /* Create and init worker threads */ > - memset(thread_tbl, 0, sizeof(thread_tbl)); > - > memset(&thr_params, 0, sizeof(thr_params)); > thr_params.start = pktio_receive_thread; > thr_params.arg = args; > thr_params.thr_type = ODP_THREAD_WORKER; > thr_params.instance = instance; > - odph_odpthreads_create(thread_tbl, &cpumask, &thr_params); > + odph_odpthreads_create(&thread_tbl, &cpumask, &thr_params); > > print_cls_statistics(args); > > odp_pktio_stop(pktio); > shutdown = 1; > - odph_odpthreads_join(thread_tbl); > + odph_odpthreads_join(&thread_tbl); > > for (i = 0; i < args->policy_count; i++) { > if ((i != args->policy_count - 1) && > diff --git a/example/generator/odp_generator.c > b/example/generator/odp_generator.c > index ccae907..e29b008 100644 > --- a/example/generator/odp_generator.c > +++ b/example/generator/odp_generator.c > @@ -621,7 +621,7 @@ static void print_global_stats(int num_workers) > */ > int main(int argc, char *argv[]) > { > - odph_odpthread_t thread_tbl[MAX_WORKERS]; > + odph_odpthread_tbl_t thread_tbl[MAX_WORKERS]; > understanding the overall goals: * to make odpthread data structure opaque to application developer * and dispel the duty of odpthread data structure allocation/free from application developer. review comments: is odph_odpthread_tbl_t a proper name? like in this example and several others, to create threads of different functions, it still invokes odph_odpthread_create() separately for each, thus makes the above name not intuitively. suggestions: instead of naming opaque odph_odpthread_tbl_t, define odph_odpthread_t as opaque void *; in examples which fan out identical workers, can code like: * odph_odpthread_t *thread_tbl = NULL;* * ....* * odph_odpthread_create(thread_tbl, cpuset (batch), ...);* in examples which spawn multiple different functional workers, can code like: * odph_odpthread_t thread_tbl[WORKERS_NUMBER];* * ....* * odph_odpthread_create(&thread_tbl[i], cpuset(each), ...);* odp_pool_t pool; > int num_workers; > int i; > @@ -746,9 +746,6 @@ int main(int argc, char *argv[]) > for (i = 0; i < args->appl.if_count; ++i) > pktio[i] = create_pktio(args->appl.if_names[i], pool); > > - /* Create and init worker threads */ > - memset(thread_tbl, 0, sizeof(thread_tbl)); > - > In this example program and several others, this seems still good practice, but not mandatory. > /* Init threads params */ > memset(&thr_params, 0, sizeof(thr_params)); > thr_params.thr_type = ODP_THREAD_WORKER; > diff --git a/example/ipsec/odp_ipsec.c b/example/ipsec/odp_ipsec.c > index fb4385f..95b907f 100644 > --- a/example/ipsec/odp_ipsec.c > +++ b/example/ipsec/odp_ipsec.c > @@ -1212,7 +1212,7 @@ int pktio_thread(void *arg EXAMPLE_UNUSED) > int > main(int argc, char *argv[]) > { > - odph_odpthread_t thread_tbl[MAX_WORKERS]; > + odph_odpthread_tbl_t thread_tbl; > int num_workers; > int i; > int stream_count; > @@ -1341,7 +1341,7 @@ main(int argc, char *argv[]) > thr_params.arg = NULL; > thr_params.thr_type = ODP_THREAD_WORKER; > thr_params.instance = instance; > - odph_odpthreads_create(thread_tbl, &cpumask, &thr_params); > + odph_odpthreads_create(&thread_tbl, &cpumask, &thr_params); > > /* > * If there are streams attempt to verify them else > @@ -1355,7 +1355,7 @@ main(int argc, char *argv[]) > } while (!done); > printf("All received\n"); > } else { > - odph_odpthreads_join(thread_tbl); > + odph_odpthreads_join(&thread_tbl); > } > > free(args->appl.if_names); > diff --git a/example/l2fwd_simple/odp_l2fwd_simple.c > b/example/l2fwd_simple/odp_l2fwd_simple.c > index daae038..8bd51e1 100644 > --- a/example/l2fwd_simple/odp_l2fwd_simple.c > +++ b/example/l2fwd_simple/odp_l2fwd_simple.c > @@ -119,7 +119,7 @@ int main(int argc, char **argv) > odp_pool_t pool; > odp_pool_param_t params; > odp_cpumask_t cpumask; > - odph_odpthread_t thd; > + odph_odpthread_tbl_t thd; > odp_instance_t instance; > odph_odpthread_params_t thr_params; > int opt; > diff --git a/example/packet/odp_pktio.c b/example/packet/odp_pktio.c > index 9028ab2..cab9da2 100644 > --- a/example/packet/odp_pktio.c > +++ b/example/packet/odp_pktio.c > @@ -342,7 +342,7 @@ static int pktio_ifburst_thread(void *arg) > */ > int main(int argc, char *argv[]) > { > - odph_odpthread_t thread_tbl[MAX_WORKERS]; > + odph_odpthread_tbl_t thread_tbl[MAX_WORKERS]; > odp_pool_t pool; > int num_workers; > int i; > @@ -409,9 +409,6 @@ int main(int argc, char *argv[]) > for (i = 0; i < args->appl.if_count; ++i) > create_pktio(args->appl.if_names[i], pool, > args->appl.mode); > > - /* Create and init worker threads */ > - memset(thread_tbl, 0, sizeof(thread_tbl)); > - > memset(&thr_params, 0, sizeof(thr_params)); > thr_params.thr_type = ODP_THREAD_WORKER; > thr_params.instance = instance; > diff --git a/example/switch/odp_switch.c b/example/switch/odp_switch.c > index 0c9b257..5cfc742 100644 > --- a/example/switch/odp_switch.c > +++ b/example/switch/odp_switch.c > @@ -884,7 +884,7 @@ static void gbl_args_init(args_t *args) > > int main(int argc, char **argv) > { > - odph_odpthread_t thread_tbl[MAX_WORKERS]; > + odph_odpthread_tbl_t thread_tbl[MAX_WORKERS]; > int i, j; > int cpu; > int num_workers; > @@ -986,8 +986,6 @@ int main(int argc, char **argv) > > print_port_mapping(); > > - memset(thread_tbl, 0, sizeof(thread_tbl)); > - > odp_barrier_init(&barrier, num_workers + 1); > > stats = gbl_args->stats; > diff --git a/example/time/time_global_test.c > b/example/time/time_global_test.c > index 372d96b..8f80149 100644 > --- a/example/time/time_global_test.c > +++ b/example/time/time_global_test.c > @@ -252,7 +252,7 @@ int main(int argc, char *argv[]) > odp_shm_t shm_glbls = ODP_SHM_INVALID; > odp_shm_t shm_log = ODP_SHM_INVALID; > int log_size, log_enries_num; > - odph_odpthread_t thread_tbl[MAX_WORKERS]; > + odph_odpthread_tbl_t thread_tbl; > odp_instance_t instance; > odph_odpthread_params_t thr_params; > > @@ -326,10 +326,10 @@ int main(int argc, char *argv[]) > thr_params.instance = instance; > > /* Create and launch worker threads */ > - odph_odpthreads_create(thread_tbl, &cpumask, &thr_params); > + odph_odpthreads_create(&thread_tbl, &cpumask, &thr_params); > > /* Wait for worker threads to exit */ > - odph_odpthreads_join(thread_tbl); > + odph_odpthreads_join(&thread_tbl); > > print_log(gbls); > > diff --git a/example/timer/odp_timer_test.c > b/example/timer/odp_timer_test.c > index cb58dfe..c594a9f 100644 > --- a/example/timer/odp_timer_test.c > +++ b/example/timer/odp_timer_test.c > @@ -330,7 +330,7 @@ static void parse_args(int argc, char *argv[], > test_args_t *args) > */ > int main(int argc, char *argv[]) > { > - odph_odpthread_t thread_tbl[MAX_WORKERS]; > + odph_odpthread_tbl_t thread_tbl; > int num_workers; > odp_queue_t queue; > uint64_t tick, ns; > @@ -393,8 +393,6 @@ int main(int argc, char *argv[]) > > parse_args(argc, argv, &gbls->args); > > - memset(thread_tbl, 0, sizeof(thread_tbl)); > - > /* Default to system CPU count unless user specified */ > num_workers = MAX_WORKERS; > if (gbls->args.cpu_count) > @@ -505,10 +503,10 @@ int main(int argc, char *argv[]) > thr_params.thr_type = ODP_THREAD_WORKER; > thr_params.instance = instance; > > - odph_odpthreads_create(thread_tbl, &cpumask, &thr_params); > + odph_odpthreads_create(&thread_tbl, &cpumask, &thr_params); > > /* Wait for worker threads to exit */ > - odph_odpthreads_join(thread_tbl); > + odph_odpthreads_join(&thread_tbl); > > /* free resources */ > if (odp_queue_destroy(queue)) > diff --git a/helper/include/odp/helper/linux.h > b/helper/include/odp/helper/linux.h > index 2e89833..2085768 100644 > --- a/helper/include/odp/helper/linux.h > +++ b/helper/include/odp/helper/linux.h > @@ -56,13 +56,6 @@ typedef struct { > int status; /**< Process state change status */ > } odph_linux_process_t; > > -/** odpthread linux type: whether an ODP thread is a linux thread or > process */ > -typedef enum odph_odpthread_linuxtype_e { > - ODPTHREAD_NOT_STARTED = 0, > - ODPTHREAD_PROCESS, > - ODPTHREAD_PTHREAD > -} odph_odpthread_linuxtype_t; > - > /** odpthread parameters for odp threads (pthreads and processes) */ > typedef struct { > int (*start)(void *); /**< Thread entry point function */ > @@ -71,28 +64,8 @@ typedef struct { > odp_instance_t instance; /**< ODP instance handle */ > } odph_odpthread_params_t; > > -/** The odpthread starting arguments, used both in process or thread mode > */ > -typedef struct { > - odph_odpthread_linuxtype_t linuxtype; /**< process or pthread */ > - odph_odpthread_params_t thr_params; /**< odpthread start > parameters */ > -} odph_odpthread_start_args_t; > - > -/** Linux odpthread state information, used both in process or thread > mode */ > -typedef struct { > - odph_odpthread_start_args_t start_args; /**< start arguments */ > - int cpu; /**< CPU ID */ > - int last; /**< true if last table > entry */ > - union { > - struct { /* for thread implementation */ > - pthread_t thread_id; /**< Pthread ID */ > - pthread_attr_t attr; /**< Pthread attributes */ > - } thread; > - struct { /* for process implementation */ > - pid_t pid; /**< Process ID */ > - int status; /**< Process state chge > status*/ > - } proc; > - }; > -} odph_odpthread_t; > +/** abstract type for odpthread arrays:*/ > +typedef void *odph_odpthread_tbl_t; > > Yes, understand, to expose only opaque odpthread representation for user interface. > /** > * Creates and launches pthreads > @@ -182,7 +155,7 @@ int odph_linux_process_wait_n(odph_linux_process_t > *proc_tbl, int num); > * > * @return Number of threads created > */ > -int odph_odpthreads_create(odph_odpthread_t *thread_tbl, > +int odph_odpthreads_create(odph_odpthread_tbl_t *thread_tbl_ptr, > const odp_cpumask_t *mask, > const odph_odpthread_params_t *thr_params); > > @@ -197,7 +170,7 @@ int odph_odpthreads_create(odph_odpthread_t > *thread_tbl, > * the thread join/process wait itself failed -e.g. as the result of a > kill) > * > */ > -int odph_odpthreads_join(odph_odpthread_t *thread_tbl); > +int odph_odpthreads_join(odph_odpthread_tbl_t *thread_tbl_ptr); > > /** > * Merge getopt options > diff --git a/helper/linux.c b/helper/linux.c > index 6366694..57cd8a7 100644 > --- a/helper/linux.c > +++ b/helper/linux.c > @@ -21,6 +21,36 @@ > #include <odp/helper/linux.h> > #include "odph_debug.h" > > +/** odpthread linux type: whether an ODP thread is a linux thread or > process */ > +typedef enum _odph_odpthread_linuxtype_e { > + ODPTHREAD_NOT_STARTED = 0, > + ODPTHREAD_PROCESS, > + ODPTHREAD_PTHREAD > +} _odph_odpthread_linuxtype_t; > + > +/** The odpthread starting arguments, used both in process or thread mode > */ > +typedef struct { > + _odph_odpthread_linuxtype_t linuxtype; > + odph_odpthread_params_t thr_params; /*copy of thread start > parameter*/ > +} _odph_odpthread_start_args_t; > + > +/** Linux odpthread state information, used both in process or thread > mode */ > +typedef struct { > + _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_id; /**< Pthread ID */ > + pthread_attr_t attr; /**< Pthread attributes */ > + } thread; > + struct { /* for process implementation */ > + pid_t pid; /**< Process ID */ > + int status; /**< Process state chge > status*/ > + } proc; > + }; > +} _odph_odpthread_t; > + > Can name like _odph_odpthread_impl_t, only a suggestion for your consideration :). > static struct { > int proc; /* true when process mode is required, false otherwise */ > } helper_options; > @@ -251,7 +281,7 @@ static void *odpthread_run_start_routine(void *arg) > int ret; > odph_odpthread_params_t *thr_params; > > - odph_odpthread_start_args_t *start_args = arg; > + _odph_odpthread_start_args_t *start_args = arg; > > thr_params = &start_args->thr_params; > > @@ -289,7 +319,7 @@ static void *odpthread_run_start_routine(void *arg) > /* > * Create a single ODPthread as a linux process > */ > -static int odph_linux_process_create(odph_odpthread_t *thread_tbl, > +static int odph_linux_process_create(_odph_odpthread_t *thread_tbl, > int cpu, > const odph_odpthread_params_t > *thr_params) > { > @@ -338,7 +368,7 @@ static int odph_linux_process_create(odph_odpthread_t > *thread_tbl, > /* > * Create a single ODPthread as a linux thread > */ > -static int odph_linux_thread_create(odph_odpthread_t *thread_tbl, > +static int odph_linux_thread_create(_odph_odpthread_t *thread_tbl, > int cpu, > const odph_odpthread_params_t > *thr_params) > { > @@ -374,7 +404,7 @@ static int odph_linux_thread_create(odph_odpthread_t > *thread_tbl, > /* > * create an odpthread set (as linux processes or linux threads or both) > */ > -int odph_odpthreads_create(odph_odpthread_t *thread_tbl, > +int odph_odpthreads_create(odph_odpthread_tbl_t *thread_tbl_ptr, > const odp_cpumask_t *mask, > const odph_odpthread_params_t *thr_params) > { > @@ -382,11 +412,10 @@ int odph_odpthreads_create(odph_odpthread_t > *thread_tbl, > int num; > int cpu_count; > int cpu; > + _odph_odpthread_t *thread_tbl; > > num = odp_cpumask_count(mask); > > - memset(thread_tbl, 0, num * sizeof(odph_odpthread_t)); > - > cpu_count = odp_cpu_count(); > > if (num < 1 || num > cpu_count) { > @@ -396,6 +425,11 @@ int odph_odpthreads_create(odph_odpthread_t > *thread_tbl, > return -1; > } > > + thread_tbl = malloc(num * sizeof(_odph_odpthread_t)); > + *thread_tbl_ptr = (void *)thread_tbl; > + > + memset(thread_tbl, 0, num * sizeof(_odph_odpthread_t)); > + > Need a check for potential malloc failures. > cpu = odp_cpumask_first(mask); > for (i = 0; i < num; i++) { > if (!helper_options.proc) { > @@ -420,8 +454,9 @@ int odph_odpthreads_create(odph_odpthread_t > *thread_tbl, > /* > * wait for the odpthreads termination (linux processes and threads) > */ > -int odph_odpthreads_join(odph_odpthread_t *thread_tbl) > +int odph_odpthreads_join(odph_odpthread_tbl_t *thread_tbl_ptr) > { > + _odph_odpthread_t *thread_tbl; > pid_t pid; > int i = 0; > int terminated = 0; > @@ -430,6 +465,13 @@ int odph_odpthreads_join(odph_odpthread_t *thread_tbl) > int ret; > int retval = 0; > > + thread_tbl = (_odph_odpthread_t *)*thread_tbl_ptr; > + > + if (!thread_tbl) { > + ODPH_ERR("Attempt to join thread from invalid table\n"); > + return -1; > + } > + > /* joins linux threads or wait for processes */ > do { > /* pthreads: */ > @@ -489,7 +531,10 @@ int odph_odpthreads_join(odph_odpthread_t *thread_tbl) > > } while (!thread_tbl[i++].last); > > - return (retval < 0) ? retval : terminated; > + /* free the allocated table: */ > + free(*thread_tbl_ptr); > + > + return (retval < 0) ? retval : i; > } > > /* > diff --git a/helper/test/odpthreads.c b/helper/test/odpthreads.c > index bba4fa5..489ecaa 100644 > --- a/helper/test/odpthreads.c > +++ b/helper/test/odpthreads.c > @@ -31,7 +31,7 @@ int main(int argc, char *argv[]) > { > odp_instance_t instance; > odph_odpthread_params_t thr_params; > - odph_odpthread_t thread_tbl[NUMBER_WORKERS]; > + odph_odpthread_tbl_t thread_tbl; > odp_cpumask_t cpu_mask; > int num_workers; > int cpu; > @@ -80,9 +80,9 @@ int main(int argc, char *argv[]) > thr_params.thr_type = ODP_THREAD_WORKER; > thr_params.instance = instance; > > - odph_odpthreads_create(&thread_tbl[0], &cpu_mask, &thr_params); > + odph_odpthreads_create(&thread_tbl, &cpu_mask, &thr_params); > > - ret = odph_odpthreads_join(thread_tbl); > + ret = odph_odpthreads_join(&thread_tbl); > if (ret < 0) > exit(EXIT_FAILURE); > > diff --git a/test/performance/odp_crypto.c b/test/performance/odp_crypto.c > index 404984d..b4ce793 100644 > --- a/test/performance/odp_crypto.c > +++ b/test/performance/odp_crypto.c > @@ -724,7 +724,7 @@ int main(int argc, char *argv[]) > odp_cpumask_t cpumask; > char cpumaskstr[ODP_CPUMASK_STR_SIZE]; > int num_workers = 1; > - odph_odpthread_t thr[num_workers]; > + odph_odpthread_tbl_t thr; > odp_instance_t instance; > > memset(&cargs, 0, sizeof(cargs)); > @@ -794,8 +794,6 @@ int main(int argc, char *argv[]) > printf("Run in sync mode\n"); > } > > - memset(thr, 0, sizeof(thr)); > - > if (cargs.alg_config) { > odph_odpthread_params_t thr_params; > > @@ -806,8 +804,8 @@ int main(int argc, char *argv[]) > thr_params.instance = instance; > > if (cargs.schedule) { > - odph_odpthreads_create(&thr[0], &cpumask, > &thr_params); > - odph_odpthreads_join(&thr[0]); > + odph_odpthreads_create(&thr, &cpumask, > &thr_params); > + odph_odpthreads_join(&thr); > } else { > run_measure_one_config(&cargs, cargs.alg_config); > } > diff --git a/test/performance/odp_l2fwd.c b/test/performance/odp_l2fwd.c > index e38e6f8..28874d1 100644 > --- a/test/performance/odp_l2fwd.c > +++ b/test/performance/odp_l2fwd.c > @@ -1293,7 +1293,7 @@ static void gbl_args_init(args_t *args) > */ > int main(int argc, char *argv[]) > { > - odph_odpthread_t thread_tbl[MAX_WORKERS]; > + odph_odpthread_tbl_t thread_tbl[MAX_WORKERS]; > odp_pool_t pool; > int i; > int cpu; > @@ -1424,8 +1424,6 @@ int main(int argc, char *argv[]) > gbl_args->appl.in_mode == PLAIN_QUEUE) > print_port_mapping(); > > - memset(thread_tbl, 0, sizeof(thread_tbl)); > - > stats = gbl_args->stats; > > odp_barrier_init(&barrier, num_workers + 1); > diff --git a/test/performance/odp_pktio_perf.c > b/test/performance/odp_pktio_perf.c > index 98ec681..bda7d95 100644 > --- a/test/performance/odp_pktio_perf.c > +++ b/test/performance/odp_pktio_perf.c > @@ -601,10 +601,11 @@ static int run_test_single(odp_cpumask_t > *thd_mask_tx, > odp_cpumask_t *thd_mask_rx, > test_status_t *status) > { > - odph_odpthread_t thd_tbl[MAX_WORKERS]; > + odph_odpthread_tbl_t rx_thd_tbl; > + odph_odpthread_tbl_t tx_thd_tbl; > thread_args_t args_tx, args_rx; > uint64_t expected_tx_cnt; > - int num_tx_workers, num_rx_workers; > + int num_tx_workers; > odph_odpthread_params_t thr_params; > > memset(&thr_params, 0, sizeof(thr_params)); > @@ -613,7 +614,6 @@ static int run_test_single(odp_cpumask_t *thd_mask_tx, > > odp_atomic_store_u32(&shutdown, 0); > > - memset(thd_tbl, 0, sizeof(thd_tbl)); > memset(gbl_args->rx_stats, 0, gbl_args->rx_stats_size); > memset(gbl_args->tx_stats, 0, gbl_args->tx_stats_size); > > @@ -623,9 +623,8 @@ static int run_test_single(odp_cpumask_t *thd_mask_tx, > thr_params.start = run_thread_rx; > thr_params.arg = &args_rx; > args_rx.batch_len = gbl_args->args.rx_batch_len; > - odph_odpthreads_create(&thd_tbl[0], thd_mask_rx, &thr_params); > + odph_odpthreads_create(&rx_thd_tbl, thd_mask_rx, &thr_params); > odp_barrier_wait(&gbl_args->rx_barrier); > - num_rx_workers = odp_cpumask_count(thd_mask_rx); > > /* then start transmitters */ > thr_params.start = run_thread_tx; > @@ -634,12 +633,12 @@ static int run_test_single(odp_cpumask_t > *thd_mask_tx, > args_tx.pps = status->pps_curr / num_tx_workers; > args_tx.duration = gbl_args->args.duration; > args_tx.batch_len = gbl_args->args.tx_batch_len; > - odph_odpthreads_create(&thd_tbl[num_rx_workers], thd_mask_tx, > + odph_odpthreads_create(&tx_thd_tbl, thd_mask_tx, > &thr_params); > odp_barrier_wait(&gbl_args->tx_barrier); > > /* wait for transmitter threads to terminate */ > - odph_odpthreads_join(&thd_tbl[num_rx_workers]); > + odph_odpthreads_join(&tx_thd_tbl); > > /* delay to allow transmitted packets to reach the receivers */ > odp_time_wait_ns(SHUTDOWN_DELAY_NS); > @@ -648,7 +647,7 @@ static int run_test_single(odp_cpumask_t *thd_mask_tx, > odp_atomic_store_u32(&shutdown, 1); > > /* wait for receivers */ > - odph_odpthreads_join(&thd_tbl[0]); > + odph_odpthreads_join(&rx_thd_tbl); > > if (!status->warmup) > return process_results(expected_tx_cnt, status); > diff --git a/test/performance/odp_scheduling.c > b/test/performance/odp_scheduling.c > index 1d3bfd1..b1b2a86 100644 > --- a/test/performance/odp_scheduling.c > +++ b/test/performance/odp_scheduling.c > @@ -775,7 +775,7 @@ static void parse_args(int argc, char *argv[], > test_args_t *args) > */ > int main(int argc, char *argv[]) > { > - odph_odpthread_t thread_tbl[MAX_WORKERS]; > + odph_odpthread_tbl_t thread_tbl; > test_args_t args; > int num_workers; > odp_cpumask_t cpumask; > @@ -795,8 +795,6 @@ int main(int argc, char *argv[]) > memset(&args, 0, sizeof(args)); > parse_args(argc, argv, &args); > > - memset(thread_tbl, 0, sizeof(thread_tbl)); > - > /* ODP global init */ > if (odp_init_global(&instance, NULL, NULL)) { > LOG_ERR("ODP global init failed.\n"); > @@ -943,10 +941,10 @@ int main(int argc, char *argv[]) > thr_params.instance = instance; > thr_params.start = run_thread; > thr_params.arg = NULL; > - odph_odpthreads_create(thread_tbl, &cpumask, &thr_params); > + odph_odpthreads_create(&thread_tbl, &cpumask, &thr_params); > > /* Wait for worker threads to terminate */ > - odph_odpthreads_join(thread_tbl); > + odph_odpthreads_join(&thread_tbl); > > printf("ODP example complete\n\n"); > > diff --git a/test/validation/common/odp_cunit_common.c > b/test/validation/common/odp_cunit_common.c > index 7df9aa6..cef0f4d 100644 > --- a/test/validation/common/odp_cunit_common.c > +++ b/test/validation/common/odp_cunit_common.c > @@ -9,7 +9,7 @@ > #include <odp_cunit_common.h> > #include <odp/helper/linux.h> > /* Globals */ > -static odph_odpthread_t thread_tbl[MAX_WORKERS]; > +static odph_odpthread_tbl_t thread_tbl; > static odp_instance_t instance; > > /* > @@ -40,14 +40,14 @@ int odp_cunit_thread_create(int func_ptr(void *), > pthrd_arg *arg) > /* Create and init additional threads */ > odp_cpumask_default_worker(&cpumask, arg->numthrds); > > - return odph_odpthreads_create(thread_tbl, &cpumask, &thr_params); > + return odph_odpthreads_create(&thread_tbl, &cpumask, &thr_params); > } > > /** exit from test thread */ > int odp_cunit_thread_exit(pthrd_arg *arg) > { > /* Wait for other threads to exit */ > - if (odph_odpthreads_join(thread_tbl) != arg->numthrds) { > + if (odph_odpthreads_join(&thread_tbl) != arg->numthrds) { > fprintf(stderr, > "error: odph_odpthreads_join() failed.\n"); > return -1; > -- > 2.5.0 > >
On 26 May 2016 at 11:50, Yi He <yi.he@linaro.org> wrote: > > > On 25 May 2016 at 21:12, Christophe Milard <christophe.milard@linaro.org> > wrote: > >> The odph_odpthread_t (non opaque array of odpthread) is now replaced by a >> single type odph_odpthread_tbl_t abstracted as a void*. >> The table describing the odpthreads being created is now malloc'd and >> freed by the helper function themselves. >> >> Signed-off-by: Christophe Milard <christophe.milard@linaro.org> >> --- >> example/classifier/odp_classifier.c | 9 ++--- >> example/generator/odp_generator.c | 5 +-- >> example/ipsec/odp_ipsec.c | 6 +-- >> example/l2fwd_simple/odp_l2fwd_simple.c | 2 +- >> example/packet/odp_pktio.c | 5 +-- >> example/switch/odp_switch.c | 4 +- >> example/time/time_global_test.c | 6 +-- >> example/timer/odp_timer_test.c | 8 ++-- >> helper/include/odp/helper/linux.h | 35 ++---------------- >> helper/linux.c | 61 >> +++++++++++++++++++++++++++---- >> helper/test/odpthreads.c | 6 +-- >> test/performance/odp_crypto.c | 8 ++-- >> test/performance/odp_l2fwd.c | 4 +- >> test/performance/odp_pktio_perf.c | 15 ++++---- >> test/performance/odp_scheduling.c | 8 ++-- >> test/validation/common/odp_cunit_common.c | 6 +-- >> 16 files changed, 93 insertions(+), 95 deletions(-) >> >> diff --git a/example/classifier/odp_classifier.c >> b/example/classifier/odp_classifier.c >> index 20e64ec..7512ec6 100644 >> --- a/example/classifier/odp_classifier.c >> +++ b/example/classifier/odp_classifier.c >> @@ -469,7 +469,7 @@ static void configure_cos(odp_cos_t default_cos, >> appl_args_t *args) >> */ > > int main(int argc, char *argv[]) >> { >> - odph_odpthread_t thread_tbl[MAX_WORKERS]; >> + odph_odpthread_tbl_t thread_tbl; >> odp_pool_t pool; >> int num_workers; >> int i; >> @@ -562,21 +562,18 @@ int main(int argc, char *argv[]) >> exit(EXIT_FAILURE); >> } >> >> - /* Create and init worker threads */ >> - memset(thread_tbl, 0, sizeof(thread_tbl)); >> - >> memset(&thr_params, 0, sizeof(thr_params)); >> thr_params.start = pktio_receive_thread; >> thr_params.arg = args; >> thr_params.thr_type = ODP_THREAD_WORKER; >> thr_params.instance = instance; >> - odph_odpthreads_create(thread_tbl, &cpumask, &thr_params); >> + odph_odpthreads_create(&thread_tbl, &cpumask, &thr_params); >> >> print_cls_statistics(args); >> >> odp_pktio_stop(pktio); >> shutdown = 1; >> - odph_odpthreads_join(thread_tbl); >> + odph_odpthreads_join(&thread_tbl); >> >> for (i = 0; i < args->policy_count; i++) { >> if ((i != args->policy_count - 1) && >> diff --git a/example/generator/odp_generator.c >> b/example/generator/odp_generator.c >> index ccae907..e29b008 100644 >> --- a/example/generator/odp_generator.c >> +++ b/example/generator/odp_generator.c >> @@ -621,7 +621,7 @@ static void print_global_stats(int num_workers) >> */ >> int main(int argc, char *argv[]) >> { >> - odph_odpthread_t thread_tbl[MAX_WORKERS]; >> + odph_odpthread_tbl_t thread_tbl[MAX_WORKERS]; >> > > understanding the overall goals: > * to make odpthread data structure opaque to application developer > * and dispel the duty of odpthread data structure allocation/free from > application developer. > review comments: > is odph_odpthread_tbl_t a proper name? > like in this example and several others, to create threads of > different functions, it still invokes > odph_odpthread_create() separately for each, thus makes the above name > not intuitively. > No, I think you misunderstood the thing there (or I was not clear enough). You do invoke odph_odpthread_create() only once for a complete set of threads, hence the tbl name. If you look at the modification for the classifier, for instance, you will notice that odph_odpthread_t thread_tbl[MAX_WORKERS]; is changed to: odph_odpthread_tbl_t thread_tbl; This is indeed because odph_odpthread_tbl_t is the table. Some other tests, such as the odp generator, for instance keep the array: odph_odpthread_t thread_tbl[MAX_WORKERS]; becomes odph_odpthread_tbl_t thread_tbl[MAX_WORKERS]; This is due to the fact that the different threads do not share their start parameters (such as the function to be run). In that case, the caller decides to handle N table of 1 entries rather than 1 table of N entries. There is , of course, nothing preventing you to create tables of size 1. But these are tables anyway! If I can do anything to avoid the confusion, please tell. But I thing that _tbl (table) name is appropriate. suggestions: > instead of naming opaque odph_odpthread_tbl_t, define odph_odpthread_t > as opaque void *; > in examples which fan out identical workers, can code like: > * odph_odpthread_t *thread_tbl = NULL;* > * ....* > * odph_odpthread_create(thread_tbl, cpuset (batch), ...);* > in examples which spawn multiple different functional workers, can > code like: > * odph_odpthread_t thread_tbl[WORKERS_NUMBER];* > * ....* > * odph_odpthread_create(&thread_tbl[i], cpuset(each), ...);* > I am confused here: would thread_tbl[i] be the entry for one or many threads?? The mask passed as parameter to odph_odpthread_create may contain many bits set in which case many threads are created... > > odp_pool_t pool; >> int num_workers; >> int i; >> @@ -746,9 +746,6 @@ int main(int argc, char *argv[]) >> for (i = 0; i < args->appl.if_count; ++i) >> pktio[i] = create_pktio(args->appl.if_names[i], pool); >> >> - /* Create and init worker threads */ >> - memset(thread_tbl, 0, sizeof(thread_tbl)); >> - >> > > In this example program and several others, this seems still good > practice, but not mandatory. > Not sure I understand: would you like to see thread_tbl=NULL here? maybe this gets clearer once we agree on what thread_tbl should be! > >> /* Init threads params */ >> memset(&thr_params, 0, sizeof(thr_params)); >> thr_params.thr_type = ODP_THREAD_WORKER; >> diff --git a/example/ipsec/odp_ipsec.c b/example/ipsec/odp_ipsec.c >> index fb4385f..95b907f 100644 >> --- a/example/ipsec/odp_ipsec.c >> +++ b/example/ipsec/odp_ipsec.c >> @@ -1212,7 +1212,7 @@ int pktio_thread(void *arg EXAMPLE_UNUSED) >> int >> main(int argc, char *argv[]) >> { >> - odph_odpthread_t thread_tbl[MAX_WORKERS]; >> + odph_odpthread_tbl_t thread_tbl; >> int num_workers; >> int i; >> int stream_count; >> @@ -1341,7 +1341,7 @@ main(int argc, char *argv[]) >> thr_params.arg = NULL; >> thr_params.thr_type = ODP_THREAD_WORKER; >> thr_params.instance = instance; >> - odph_odpthreads_create(thread_tbl, &cpumask, &thr_params); >> + odph_odpthreads_create(&thread_tbl, &cpumask, &thr_params); >> >> /* >> * If there are streams attempt to verify them else >> @@ -1355,7 +1355,7 @@ main(int argc, char *argv[]) >> } while (!done); >> printf("All received\n"); >> } else { >> - odph_odpthreads_join(thread_tbl); >> + odph_odpthreads_join(&thread_tbl); >> } >> >> free(args->appl.if_names); >> diff --git a/example/l2fwd_simple/odp_l2fwd_simple.c >> b/example/l2fwd_simple/odp_l2fwd_simple.c >> index daae038..8bd51e1 100644 >> --- a/example/l2fwd_simple/odp_l2fwd_simple.c >> +++ b/example/l2fwd_simple/odp_l2fwd_simple.c >> @@ -119,7 +119,7 @@ int main(int argc, char **argv) >> odp_pool_t pool; >> odp_pool_param_t params; >> odp_cpumask_t cpumask; >> - odph_odpthread_t thd; >> + odph_odpthread_tbl_t thd; >> odp_instance_t instance; >> odph_odpthread_params_t thr_params; >> int opt; >> diff --git a/example/packet/odp_pktio.c b/example/packet/odp_pktio.c >> index 9028ab2..cab9da2 100644 >> --- a/example/packet/odp_pktio.c >> +++ b/example/packet/odp_pktio.c >> @@ -342,7 +342,7 @@ static int pktio_ifburst_thread(void *arg) >> */ >> int main(int argc, char *argv[]) >> { >> - odph_odpthread_t thread_tbl[MAX_WORKERS]; >> + odph_odpthread_tbl_t thread_tbl[MAX_WORKERS]; >> odp_pool_t pool; >> int num_workers; >> int i; >> @@ -409,9 +409,6 @@ int main(int argc, char *argv[]) >> for (i = 0; i < args->appl.if_count; ++i) >> create_pktio(args->appl.if_names[i], pool, >> args->appl.mode); >> >> - /* Create and init worker threads */ >> - memset(thread_tbl, 0, sizeof(thread_tbl)); >> - >> memset(&thr_params, 0, sizeof(thr_params)); >> thr_params.thr_type = ODP_THREAD_WORKER; >> thr_params.instance = instance; >> diff --git a/example/switch/odp_switch.c b/example/switch/odp_switch.c >> index 0c9b257..5cfc742 100644 >> --- a/example/switch/odp_switch.c >> +++ b/example/switch/odp_switch.c >> @@ -884,7 +884,7 @@ static void gbl_args_init(args_t *args) >> >> int main(int argc, char **argv) >> { >> - odph_odpthread_t thread_tbl[MAX_WORKERS]; >> + odph_odpthread_tbl_t thread_tbl[MAX_WORKERS]; >> int i, j; >> int cpu; >> int num_workers; >> @@ -986,8 +986,6 @@ int main(int argc, char **argv) >> >> print_port_mapping(); >> >> - memset(thread_tbl, 0, sizeof(thread_tbl)); >> - >> odp_barrier_init(&barrier, num_workers + 1); >> >> stats = gbl_args->stats; >> diff --git a/example/time/time_global_test.c >> b/example/time/time_global_test.c >> index 372d96b..8f80149 100644 >> --- a/example/time/time_global_test.c >> +++ b/example/time/time_global_test.c >> @@ -252,7 +252,7 @@ int main(int argc, char *argv[]) >> odp_shm_t shm_glbls = ODP_SHM_INVALID; >> odp_shm_t shm_log = ODP_SHM_INVALID; >> int log_size, log_enries_num; >> - odph_odpthread_t thread_tbl[MAX_WORKERS]; >> + odph_odpthread_tbl_t thread_tbl; >> odp_instance_t instance; >> odph_odpthread_params_t thr_params; >> >> @@ -326,10 +326,10 @@ int main(int argc, char *argv[]) >> thr_params.instance = instance; >> >> /* Create and launch worker threads */ >> - odph_odpthreads_create(thread_tbl, &cpumask, &thr_params); >> + odph_odpthreads_create(&thread_tbl, &cpumask, &thr_params); >> >> /* Wait for worker threads to exit */ >> - odph_odpthreads_join(thread_tbl); >> + odph_odpthreads_join(&thread_tbl); >> >> print_log(gbls); >> >> diff --git a/example/timer/odp_timer_test.c >> b/example/timer/odp_timer_test.c >> index cb58dfe..c594a9f 100644 >> --- a/example/timer/odp_timer_test.c >> +++ b/example/timer/odp_timer_test.c >> @@ -330,7 +330,7 @@ static void parse_args(int argc, char *argv[], >> test_args_t *args) >> */ >> int main(int argc, char *argv[]) >> { >> - odph_odpthread_t thread_tbl[MAX_WORKERS]; >> + odph_odpthread_tbl_t thread_tbl; >> int num_workers; >> odp_queue_t queue; >> uint64_t tick, ns; >> @@ -393,8 +393,6 @@ int main(int argc, char *argv[]) >> >> parse_args(argc, argv, &gbls->args); >> >> - memset(thread_tbl, 0, sizeof(thread_tbl)); >> - >> /* Default to system CPU count unless user specified */ >> num_workers = MAX_WORKERS; >> if (gbls->args.cpu_count) >> @@ -505,10 +503,10 @@ int main(int argc, char *argv[]) >> thr_params.thr_type = ODP_THREAD_WORKER; >> thr_params.instance = instance; >> >> - odph_odpthreads_create(thread_tbl, &cpumask, &thr_params); >> + odph_odpthreads_create(&thread_tbl, &cpumask, &thr_params); >> >> /* Wait for worker threads to exit */ >> - odph_odpthreads_join(thread_tbl); >> + odph_odpthreads_join(&thread_tbl); >> >> /* free resources */ >> if (odp_queue_destroy(queue)) >> diff --git a/helper/include/odp/helper/linux.h >> b/helper/include/odp/helper/linux.h >> index 2e89833..2085768 100644 >> --- a/helper/include/odp/helper/linux.h >> +++ b/helper/include/odp/helper/linux.h >> @@ -56,13 +56,6 @@ typedef struct { >> int status; /**< Process state change status */ >> } odph_linux_process_t; >> >> -/** odpthread linux type: whether an ODP thread is a linux thread or >> process */ >> -typedef enum odph_odpthread_linuxtype_e { >> - ODPTHREAD_NOT_STARTED = 0, >> - ODPTHREAD_PROCESS, >> - ODPTHREAD_PTHREAD >> -} odph_odpthread_linuxtype_t; >> - >> /** odpthread parameters for odp threads (pthreads and processes) */ >> typedef struct { >> int (*start)(void *); /**< Thread entry point function */ >> @@ -71,28 +64,8 @@ typedef struct { >> odp_instance_t instance; /**< ODP instance handle */ >> } odph_odpthread_params_t; >> >> -/** The odpthread starting arguments, used both in process or thread >> mode */ >> -typedef struct { >> - odph_odpthread_linuxtype_t linuxtype; /**< process or pthread */ >> - odph_odpthread_params_t thr_params; /**< odpthread start >> parameters */ >> -} odph_odpthread_start_args_t; >> - >> -/** Linux odpthread state information, used both in process or thread >> mode */ >> -typedef struct { >> - odph_odpthread_start_args_t start_args; /**< start arguments >> */ >> - int cpu; /**< CPU ID */ >> - int last; /**< true if last table >> entry */ >> - union { >> - struct { /* for thread implementation */ >> - pthread_t thread_id; /**< Pthread ID */ >> - pthread_attr_t attr; /**< Pthread attributes */ >> - } thread; >> - struct { /* for process implementation */ >> - pid_t pid; /**< Process ID */ >> - int status; /**< Process state chge >> status*/ >> - } proc; >> - }; >> -} odph_odpthread_t; >> +/** abstract type for odpthread arrays:*/ >> +typedef void *odph_odpthread_tbl_t; >> >> > Yes, understand, to expose only opaque odpthread representation for user > interface. > > >> /** >> * Creates and launches pthreads >> @@ -182,7 +155,7 @@ int odph_linux_process_wait_n(odph_linux_process_t >> *proc_tbl, int num); >> * >> * @return Number of threads created >> */ >> -int odph_odpthreads_create(odph_odpthread_t *thread_tbl, >> +int odph_odpthreads_create(odph_odpthread_tbl_t *thread_tbl_ptr, >> const odp_cpumask_t *mask, >> const odph_odpthread_params_t *thr_params); >> >> @@ -197,7 +170,7 @@ int odph_odpthreads_create(odph_odpthread_t >> *thread_tbl, >> * the thread join/process wait itself failed -e.g. as the result of a >> kill) >> * >> */ >> -int odph_odpthreads_join(odph_odpthread_t *thread_tbl); >> +int odph_odpthreads_join(odph_odpthread_tbl_t *thread_tbl_ptr); >> >> /** >> * Merge getopt options >> diff --git a/helper/linux.c b/helper/linux.c >> index 6366694..57cd8a7 100644 >> --- a/helper/linux.c >> +++ b/helper/linux.c >> @@ -21,6 +21,36 @@ >> #include <odp/helper/linux.h> >> #include "odph_debug.h" >> >> +/** odpthread linux type: whether an ODP thread is a linux thread or >> process */ >> +typedef enum _odph_odpthread_linuxtype_e { >> + ODPTHREAD_NOT_STARTED = 0, >> + ODPTHREAD_PROCESS, >> + ODPTHREAD_PTHREAD >> +} _odph_odpthread_linuxtype_t; >> + >> +/** The odpthread starting arguments, used both in process or thread >> mode */ >> +typedef struct { >> + _odph_odpthread_linuxtype_t linuxtype; >> + odph_odpthread_params_t thr_params; /*copy of thread start >> parameter*/ >> +} _odph_odpthread_start_args_t; >> + >> +/** Linux odpthread state information, used both in process or thread >> mode */ >> +typedef struct { >> + _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_id; /**< Pthread ID */ >> + pthread_attr_t attr; /**< Pthread attributes */ >> + } thread; >> + struct { /* for process implementation */ >> + pid_t pid; /**< Process ID */ >> + int status; /**< Process state chge >> status*/ >> + } proc; >> + }; >> +} _odph_odpthread_t; >> + >> > > Can name like _odph_odpthread_impl_t, only a suggestion for your > consideration :). > I can change that in V2, sure! > > >> static struct { >> int proc; /* true when process mode is required, false otherwise >> */ >> } helper_options; >> @@ -251,7 +281,7 @@ static void *odpthread_run_start_routine(void *arg) >> int ret; >> odph_odpthread_params_t *thr_params; >> >> - odph_odpthread_start_args_t *start_args = arg; >> + _odph_odpthread_start_args_t *start_args = arg; >> >> thr_params = &start_args->thr_params; >> >> @@ -289,7 +319,7 @@ static void *odpthread_run_start_routine(void *arg) >> /* >> * Create a single ODPthread as a linux process >> */ >> -static int odph_linux_process_create(odph_odpthread_t *thread_tbl, >> +static int odph_linux_process_create(_odph_odpthread_t *thread_tbl, >> int cpu, >> const odph_odpthread_params_t >> *thr_params) >> { >> @@ -338,7 +368,7 @@ static int odph_linux_process_create(odph_odpthread_t >> *thread_tbl, >> /* >> * Create a single ODPthread as a linux thread >> */ >> -static int odph_linux_thread_create(odph_odpthread_t *thread_tbl, >> +static int odph_linux_thread_create(_odph_odpthread_t *thread_tbl, >> int cpu, >> const odph_odpthread_params_t >> *thr_params) >> { >> @@ -374,7 +404,7 @@ static int odph_linux_thread_create(odph_odpthread_t >> *thread_tbl, >> /* >> * create an odpthread set (as linux processes or linux threads or both) >> */ >> -int odph_odpthreads_create(odph_odpthread_t *thread_tbl, >> +int odph_odpthreads_create(odph_odpthread_tbl_t *thread_tbl_ptr, >> const odp_cpumask_t *mask, >> const odph_odpthread_params_t *thr_params) >> { >> @@ -382,11 +412,10 @@ int odph_odpthreads_create(odph_odpthread_t >> *thread_tbl, >> int num; >> int cpu_count; >> int cpu; >> + _odph_odpthread_t *thread_tbl; >> >> num = odp_cpumask_count(mask); >> >> - memset(thread_tbl, 0, num * sizeof(odph_odpthread_t)); >> - >> cpu_count = odp_cpu_count(); >> >> if (num < 1 || num > cpu_count) { >> @@ -396,6 +425,11 @@ int odph_odpthreads_create(odph_odpthread_t >> *thread_tbl, >> return -1; >> } >> >> + thread_tbl = malloc(num * sizeof(_odph_odpthread_t)); >> + *thread_tbl_ptr = (void *)thread_tbl; >> + >> + memset(thread_tbl, 0, num * sizeof(_odph_odpthread_t)); >> + >> > > Need a check for potential malloc failures. > > >> cpu = odp_cpumask_first(mask); >> for (i = 0; i < num; i++) { >> if (!helper_options.proc) { >> @@ -420,8 +454,9 @@ int odph_odpthreads_create(odph_odpthread_t >> *thread_tbl, >> /* >> * wait for the odpthreads termination (linux processes and threads) >> */ >> -int odph_odpthreads_join(odph_odpthread_t *thread_tbl) >> +int odph_odpthreads_join(odph_odpthread_tbl_t *thread_tbl_ptr) >> { >> + _odph_odpthread_t *thread_tbl; >> pid_t pid; >> int i = 0; >> int terminated = 0; >> @@ -430,6 +465,13 @@ int odph_odpthreads_join(odph_odpthread_t >> *thread_tbl) >> int ret; >> int retval = 0; >> >> + thread_tbl = (_odph_odpthread_t *)*thread_tbl_ptr; >> + >> + if (!thread_tbl) { >> + ODPH_ERR("Attempt to join thread from invalid table\n"); >> + return -1; >> + } >> + >> /* joins linux threads or wait for processes */ >> do { >> /* pthreads: */ >> @@ -489,7 +531,10 @@ int odph_odpthreads_join(odph_odpthread_t >> *thread_tbl) >> >> } while (!thread_tbl[i++].last); >> >> - return (retval < 0) ? retval : terminated; >> + /* free the allocated table: */ >> + free(*thread_tbl_ptr); >> + >> + return (retval < 0) ? retval : i; >> } >> >> /* >> diff --git a/helper/test/odpthreads.c b/helper/test/odpthreads.c >> index bba4fa5..489ecaa 100644 >> --- a/helper/test/odpthreads.c >> +++ b/helper/test/odpthreads.c >> @@ -31,7 +31,7 @@ int main(int argc, char *argv[]) >> { >> odp_instance_t instance; >> odph_odpthread_params_t thr_params; >> - odph_odpthread_t thread_tbl[NUMBER_WORKERS]; >> + odph_odpthread_tbl_t thread_tbl; >> odp_cpumask_t cpu_mask; >> int num_workers; >> int cpu; >> @@ -80,9 +80,9 @@ int main(int argc, char *argv[]) >> thr_params.thr_type = ODP_THREAD_WORKER; >> thr_params.instance = instance; >> >> - odph_odpthreads_create(&thread_tbl[0], &cpu_mask, &thr_params); >> + odph_odpthreads_create(&thread_tbl, &cpu_mask, &thr_params); >> >> - ret = odph_odpthreads_join(thread_tbl); >> + ret = odph_odpthreads_join(&thread_tbl); >> if (ret < 0) >> exit(EXIT_FAILURE); >> >> diff --git a/test/performance/odp_crypto.c b/test/performance/odp_crypto.c >> index 404984d..b4ce793 100644 >> --- a/test/performance/odp_crypto.c >> +++ b/test/performance/odp_crypto.c >> @@ -724,7 +724,7 @@ int main(int argc, char *argv[]) >> odp_cpumask_t cpumask; >> char cpumaskstr[ODP_CPUMASK_STR_SIZE]; >> int num_workers = 1; >> - odph_odpthread_t thr[num_workers]; >> + odph_odpthread_tbl_t thr; >> odp_instance_t instance; >> >> memset(&cargs, 0, sizeof(cargs)); >> @@ -794,8 +794,6 @@ int main(int argc, char *argv[]) >> printf("Run in sync mode\n"); >> } >> >> - memset(thr, 0, sizeof(thr)); >> - >> if (cargs.alg_config) { >> odph_odpthread_params_t thr_params; >> >> @@ -806,8 +804,8 @@ int main(int argc, char *argv[]) >> thr_params.instance = instance; >> >> if (cargs.schedule) { >> - odph_odpthreads_create(&thr[0], &cpumask, >> &thr_params); >> - odph_odpthreads_join(&thr[0]); >> + odph_odpthreads_create(&thr, &cpumask, >> &thr_params); >> + odph_odpthreads_join(&thr); >> } else { >> run_measure_one_config(&cargs, cargs.alg_config); >> } >> diff --git a/test/performance/odp_l2fwd.c b/test/performance/odp_l2fwd.c >> index e38e6f8..28874d1 100644 >> --- a/test/performance/odp_l2fwd.c >> +++ b/test/performance/odp_l2fwd.c >> @@ -1293,7 +1293,7 @@ static void gbl_args_init(args_t *args) >> */ >> int main(int argc, char *argv[]) >> { >> - odph_odpthread_t thread_tbl[MAX_WORKERS]; >> + odph_odpthread_tbl_t thread_tbl[MAX_WORKERS]; >> odp_pool_t pool; >> int i; >> int cpu; >> @@ -1424,8 +1424,6 @@ int main(int argc, char *argv[]) >> gbl_args->appl.in_mode == PLAIN_QUEUE) >> print_port_mapping(); >> >> - memset(thread_tbl, 0, sizeof(thread_tbl)); >> - >> stats = gbl_args->stats; >> >> odp_barrier_init(&barrier, num_workers + 1); >> diff --git a/test/performance/odp_pktio_perf.c >> b/test/performance/odp_pktio_perf.c >> index 98ec681..bda7d95 100644 >> --- a/test/performance/odp_pktio_perf.c >> +++ b/test/performance/odp_pktio_perf.c >> @@ -601,10 +601,11 @@ static int run_test_single(odp_cpumask_t >> *thd_mask_tx, >> odp_cpumask_t *thd_mask_rx, >> test_status_t *status) >> { >> - odph_odpthread_t thd_tbl[MAX_WORKERS]; >> + odph_odpthread_tbl_t rx_thd_tbl; >> + odph_odpthread_tbl_t tx_thd_tbl; >> thread_args_t args_tx, args_rx; >> uint64_t expected_tx_cnt; >> - int num_tx_workers, num_rx_workers; >> + int num_tx_workers; >> odph_odpthread_params_t thr_params; >> >> memset(&thr_params, 0, sizeof(thr_params)); >> @@ -613,7 +614,6 @@ static int run_test_single(odp_cpumask_t *thd_mask_tx, >> >> odp_atomic_store_u32(&shutdown, 0); >> >> - memset(thd_tbl, 0, sizeof(thd_tbl)); >> memset(gbl_args->rx_stats, 0, gbl_args->rx_stats_size); >> memset(gbl_args->tx_stats, 0, gbl_args->tx_stats_size); >> >> @@ -623,9 +623,8 @@ static int run_test_single(odp_cpumask_t *thd_mask_tx, >> thr_params.start = run_thread_rx; >> thr_params.arg = &args_rx; >> args_rx.batch_len = gbl_args->args.rx_batch_len; >> - odph_odpthreads_create(&thd_tbl[0], thd_mask_rx, &thr_params); >> + odph_odpthreads_create(&rx_thd_tbl, thd_mask_rx, &thr_params); >> odp_barrier_wait(&gbl_args->rx_barrier); >> - num_rx_workers = odp_cpumask_count(thd_mask_rx); >> >> /* then start transmitters */ >> thr_params.start = run_thread_tx; >> @@ -634,12 +633,12 @@ static int run_test_single(odp_cpumask_t >> *thd_mask_tx, >> args_tx.pps = status->pps_curr / num_tx_workers; >> args_tx.duration = gbl_args->args.duration; >> args_tx.batch_len = gbl_args->args.tx_batch_len; >> - odph_odpthreads_create(&thd_tbl[num_rx_workers], thd_mask_tx, >> + odph_odpthreads_create(&tx_thd_tbl, thd_mask_tx, >> &thr_params); >> odp_barrier_wait(&gbl_args->tx_barrier); >> >> /* wait for transmitter threads to terminate */ >> - odph_odpthreads_join(&thd_tbl[num_rx_workers]); >> + odph_odpthreads_join(&tx_thd_tbl); >> >> /* delay to allow transmitted packets to reach the receivers */ >> odp_time_wait_ns(SHUTDOWN_DELAY_NS); >> @@ -648,7 +647,7 @@ static int run_test_single(odp_cpumask_t *thd_mask_tx, >> odp_atomic_store_u32(&shutdown, 1); >> >> /* wait for receivers */ >> - odph_odpthreads_join(&thd_tbl[0]); >> + odph_odpthreads_join(&rx_thd_tbl); >> >> if (!status->warmup) >> return process_results(expected_tx_cnt, status); >> diff --git a/test/performance/odp_scheduling.c >> b/test/performance/odp_scheduling.c >> index 1d3bfd1..b1b2a86 100644 >> --- a/test/performance/odp_scheduling.c >> +++ b/test/performance/odp_scheduling.c >> @@ -775,7 +775,7 @@ static void parse_args(int argc, char *argv[], >> test_args_t *args) >> */ >> int main(int argc, char *argv[]) >> { >> - odph_odpthread_t thread_tbl[MAX_WORKERS]; >> + odph_odpthread_tbl_t thread_tbl; >> test_args_t args; >> int num_workers; >> odp_cpumask_t cpumask; >> @@ -795,8 +795,6 @@ int main(int argc, char *argv[]) >> memset(&args, 0, sizeof(args)); >> parse_args(argc, argv, &args); >> >> - memset(thread_tbl, 0, sizeof(thread_tbl)); >> - >> /* ODP global init */ >> if (odp_init_global(&instance, NULL, NULL)) { >> LOG_ERR("ODP global init failed.\n"); >> @@ -943,10 +941,10 @@ int main(int argc, char *argv[]) >> thr_params.instance = instance; >> thr_params.start = run_thread; >> thr_params.arg = NULL; >> - odph_odpthreads_create(thread_tbl, &cpumask, &thr_params); >> + odph_odpthreads_create(&thread_tbl, &cpumask, &thr_params); >> >> /* Wait for worker threads to terminate */ >> - odph_odpthreads_join(thread_tbl); >> + odph_odpthreads_join(&thread_tbl); >> >> printf("ODP example complete\n\n"); >> >> diff --git a/test/validation/common/odp_cunit_common.c >> b/test/validation/common/odp_cunit_common.c >> index 7df9aa6..cef0f4d 100644 >> --- a/test/validation/common/odp_cunit_common.c >> +++ b/test/validation/common/odp_cunit_common.c >> @@ -9,7 +9,7 @@ >> #include <odp_cunit_common.h> >> #include <odp/helper/linux.h> >> /* Globals */ >> -static odph_odpthread_t thread_tbl[MAX_WORKERS]; >> +static odph_odpthread_tbl_t thread_tbl; >> static odp_instance_t instance; >> >> /* >> @@ -40,14 +40,14 @@ int odp_cunit_thread_create(int func_ptr(void *), >> pthrd_arg *arg) >> /* Create and init additional threads */ >> odp_cpumask_default_worker(&cpumask, arg->numthrds); >> >> - return odph_odpthreads_create(thread_tbl, &cpumask, &thr_params); >> + return odph_odpthreads_create(&thread_tbl, &cpumask, &thr_params); >> } >> >> /** exit from test thread */ >> int odp_cunit_thread_exit(pthrd_arg *arg) >> { >> /* Wait for other threads to exit */ >> - if (odph_odpthreads_join(thread_tbl) != arg->numthrds) { >> + if (odph_odpthreads_join(&thread_tbl) != arg->numthrds) { >> fprintf(stderr, >> "error: odph_odpthreads_join() failed.\n"); >> return -1; >> -- >> 2.5.0 >> >> > Tell me if my answer make sense and if you have other comments, I will put the _odph_odpthread_t to _odph_odpthread_impl_t name change for v2 Christophe.
On 26 May 2016 at 18:39, Christophe Milard <christophe.milard@linaro.org> wrote: > > > On 26 May 2016 at 11:50, Yi He <yi.he@linaro.org> wrote: > >> >> >> On 25 May 2016 at 21:12, Christophe Milard <christophe.milard@linaro.org> >> wrote: >> >>> The odph_odpthread_t (non opaque array of odpthread) is now replaced by a >>> single type odph_odpthread_tbl_t abstracted as a void*. >>> The table describing the odpthreads being created is now malloc'd and >>> freed by the helper function themselves. >>> >>> Signed-off-by: Christophe Milard <christophe.milard@linaro.org> >>> --- >>> example/classifier/odp_classifier.c | 9 ++--- >>> example/generator/odp_generator.c | 5 +-- >>> example/ipsec/odp_ipsec.c | 6 +-- >>> example/l2fwd_simple/odp_l2fwd_simple.c | 2 +- >>> example/packet/odp_pktio.c | 5 +-- >>> example/switch/odp_switch.c | 4 +- >>> example/time/time_global_test.c | 6 +-- >>> example/timer/odp_timer_test.c | 8 ++-- >>> helper/include/odp/helper/linux.h | 35 ++---------------- >>> helper/linux.c | 61 >>> +++++++++++++++++++++++++++---- >>> helper/test/odpthreads.c | 6 +-- >>> test/performance/odp_crypto.c | 8 ++-- >>> test/performance/odp_l2fwd.c | 4 +- >>> test/performance/odp_pktio_perf.c | 15 ++++---- >>> test/performance/odp_scheduling.c | 8 ++-- >>> test/validation/common/odp_cunit_common.c | 6 +-- >>> 16 files changed, 93 insertions(+), 95 deletions(-) >>> >>> diff --git a/example/classifier/odp_classifier.c >>> b/example/classifier/odp_classifier.c >>> index 20e64ec..7512ec6 100644 >>> --- a/example/classifier/odp_classifier.c >>> +++ b/example/classifier/odp_classifier.c >>> @@ -469,7 +469,7 @@ static void configure_cos(odp_cos_t default_cos, >>> appl_args_t *args) >>> */ >> >> int main(int argc, char *argv[]) >>> { >>> - odph_odpthread_t thread_tbl[MAX_WORKERS]; >>> + odph_odpthread_tbl_t thread_tbl; >>> odp_pool_t pool; >>> int num_workers; >>> int i; >>> @@ -562,21 +562,18 @@ int main(int argc, char *argv[]) >>> exit(EXIT_FAILURE); >>> } >>> >>> - /* Create and init worker threads */ >>> - memset(thread_tbl, 0, sizeof(thread_tbl)); >>> - >>> memset(&thr_params, 0, sizeof(thr_params)); >>> thr_params.start = pktio_receive_thread; >>> thr_params.arg = args; >>> thr_params.thr_type = ODP_THREAD_WORKER; >>> thr_params.instance = instance; >>> - odph_odpthreads_create(thread_tbl, &cpumask, &thr_params); >>> + odph_odpthreads_create(&thread_tbl, &cpumask, &thr_params); >>> >>> print_cls_statistics(args); >>> >>> odp_pktio_stop(pktio); >>> shutdown = 1; >>> - odph_odpthreads_join(thread_tbl); >>> + odph_odpthreads_join(&thread_tbl); >>> >>> for (i = 0; i < args->policy_count; i++) { >>> if ((i != args->policy_count - 1) && >>> diff --git a/example/generator/odp_generator.c >>> b/example/generator/odp_generator.c >>> index ccae907..e29b008 100644 >>> --- a/example/generator/odp_generator.c >>> +++ b/example/generator/odp_generator.c >>> @@ -621,7 +621,7 @@ static void print_global_stats(int num_workers) >>> */ >>> int main(int argc, char *argv[]) >>> { >>> - odph_odpthread_t thread_tbl[MAX_WORKERS]; >>> + odph_odpthread_tbl_t thread_tbl[MAX_WORKERS]; >>> >> >> understanding the overall goals: >> * to make odpthread data structure opaque to application developer >> * and dispel the duty of odpthread data structure allocation/free >> from application developer. >> review comments: >> is odph_odpthread_tbl_t a proper name? >> like in this example and several others, to create threads of >> different functions, it still invokes >> odph_odpthread_create() separately for each, thus makes the above >> name not intuitively. >> > No, I think you misunderstood the thing there (or I was not clear enough). >> You do invoke odph_odpthread_create() only once for a complete set of >> threads, hence the tbl name. If you look at the modification for the >> classifier, for instance, you will notice that >> > odph_odpthread_t thread_tbl[MAX_WORKERS]; > is changed to: > odph_odpthread_tbl_t thread_tbl; > This is indeed because odph_odpthread_tbl_t is the table. > Some other tests, such as the odp generator, for instance keep the array: > odph_odpthread_t thread_tbl[MAX_WORKERS]; > becomes > odph_odpthread_tbl_t thread_tbl[MAX_WORKERS]; > This is due to the fact that the different threads do not share their > start parameters (such as the function to be run). In that case, the caller > decides to handle N table of 1 entries rather than 1 table of N entries. > There is , of course, nothing preventing you to create tables of size 1. > But these are tables anyway! If I can do anything to avoid the confusion, > please tell. But I thing that _tbl (table) name is appropriate. > *Hi, Christophe, my major concern lies in this user case, that "*N table of 1 entries" seems not very straightforward. *I accept yours.* suggestions: >> instead of naming opaque odph_odpthread_tbl_t, define >> odph_odpthread_t as opaque void *; >> in examples which fan out identical workers, can code like: >> * odph_odpthread_t *thread_tbl = NULL;* >> * ....* >> * odph_odpthread_create(thread_tbl, cpuset (batch), ...);* >> in examples which spawn multiple different functional workers, can >> code like: >> * odph_odpthread_t thread_tbl[WORKERS_NUMBER];* >> * ....* >> * odph_odpthread_create(&thread_tbl[i], cpuset(each), ...);* >> > > I am confused here: would thread_tbl[i] be the entry for one or many > threads?? The mask passed as parameter to odph_odpthread_create may > contain many bits set in which case many threads are created... > >> odp_pool_t pool; >>> int num_workers; >>> int i; >>> @@ -746,9 +746,6 @@ int main(int argc, char *argv[]) >>> for (i = 0; i < args->appl.if_count; ++i) >>> pktio[i] = create_pktio(args->appl.if_names[i], pool); >>> >>> - /* Create and init worker threads */ >>> - memset(thread_tbl, 0, sizeof(thread_tbl)); >>> - >>> >> >> In this example program and several others, this seems still good >> practice, but not mandatory. >> > > Not sure I understand: would you like to see thread_tbl=NULL here? maybe > this gets clearer once we agree on what thread_tbl should be! > > Here I mean in this example defines: *odph_odpthread_tbl_t thread_tbl[MAX_WORKERS];* so a memset is still needed, right? >> >>> /* Init threads params */ >>> memset(&thr_params, 0, sizeof(thr_params)); >>> thr_params.thr_type = ODP_THREAD_WORKER; >>> diff --git a/example/ipsec/odp_ipsec.c b/example/ipsec/odp_ipsec.c >>> index fb4385f..95b907f 100644 >>> --- a/example/ipsec/odp_ipsec.c >>> +++ b/example/ipsec/odp_ipsec.c >>> @@ -1212,7 +1212,7 @@ int pktio_thread(void *arg EXAMPLE_UNUSED) >>> int >>> main(int argc, char *argv[]) >>> { >>> - odph_odpthread_t thread_tbl[MAX_WORKERS]; >>> + odph_odpthread_tbl_t thread_tbl; >>> int num_workers; >>> int i; >>> int stream_count; >>> @@ -1341,7 +1341,7 @@ main(int argc, char *argv[]) >>> thr_params.arg = NULL; >>> thr_params.thr_type = ODP_THREAD_WORKER; >>> thr_params.instance = instance; >>> - odph_odpthreads_create(thread_tbl, &cpumask, &thr_params); >>> + odph_odpthreads_create(&thread_tbl, &cpumask, &thr_params); >>> >>> /* >>> * If there are streams attempt to verify them else >>> @@ -1355,7 +1355,7 @@ main(int argc, char *argv[]) >>> } while (!done); >>> printf("All received\n"); >>> } else { >>> - odph_odpthreads_join(thread_tbl); >>> + odph_odpthreads_join(&thread_tbl); >>> } >>> >>> free(args->appl.if_names); >>> diff --git a/example/l2fwd_simple/odp_l2fwd_simple.c >>> b/example/l2fwd_simple/odp_l2fwd_simple.c >>> index daae038..8bd51e1 100644 >>> --- a/example/l2fwd_simple/odp_l2fwd_simple.c >>> +++ b/example/l2fwd_simple/odp_l2fwd_simple.c >>> @@ -119,7 +119,7 @@ int main(int argc, char **argv) >>> odp_pool_t pool; >>> odp_pool_param_t params; >>> odp_cpumask_t cpumask; >>> - odph_odpthread_t thd; >>> + odph_odpthread_tbl_t thd; >>> odp_instance_t instance; >>> odph_odpthread_params_t thr_params; >>> int opt; >>> diff --git a/example/packet/odp_pktio.c b/example/packet/odp_pktio.c >>> index 9028ab2..cab9da2 100644 >>> --- a/example/packet/odp_pktio.c >>> +++ b/example/packet/odp_pktio.c >>> @@ -342,7 +342,7 @@ static int pktio_ifburst_thread(void *arg) >>> */ >>> int main(int argc, char *argv[]) >>> { >>> - odph_odpthread_t thread_tbl[MAX_WORKERS]; >>> + odph_odpthread_tbl_t thread_tbl[MAX_WORKERS]; >>> odp_pool_t pool; >>> int num_workers; >>> int i; >>> @@ -409,9 +409,6 @@ int main(int argc, char *argv[]) >>> for (i = 0; i < args->appl.if_count; ++i) >>> create_pktio(args->appl.if_names[i], pool, >>> args->appl.mode); >>> >>> - /* Create and init worker threads */ >>> - memset(thread_tbl, 0, sizeof(thread_tbl)); >>> - >>> memset(&thr_params, 0, sizeof(thr_params)); >>> thr_params.thr_type = ODP_THREAD_WORKER; >>> thr_params.instance = instance; >>> diff --git a/example/switch/odp_switch.c b/example/switch/odp_switch.c >>> index 0c9b257..5cfc742 100644 >>> --- a/example/switch/odp_switch.c >>> +++ b/example/switch/odp_switch.c >>> @@ -884,7 +884,7 @@ static void gbl_args_init(args_t *args) >>> >>> int main(int argc, char **argv) >>> { >>> - odph_odpthread_t thread_tbl[MAX_WORKERS]; >>> + odph_odpthread_tbl_t thread_tbl[MAX_WORKERS]; >>> int i, j; >>> int cpu; >>> int num_workers; >>> @@ -986,8 +986,6 @@ int main(int argc, char **argv) >>> >>> print_port_mapping(); >>> >>> - memset(thread_tbl, 0, sizeof(thread_tbl)); >>> - >>> odp_barrier_init(&barrier, num_workers + 1); >>> >>> stats = gbl_args->stats; >>> diff --git a/example/time/time_global_test.c >>> b/example/time/time_global_test.c >>> index 372d96b..8f80149 100644 >>> --- a/example/time/time_global_test.c >>> +++ b/example/time/time_global_test.c >>> @@ -252,7 +252,7 @@ int main(int argc, char *argv[]) >>> odp_shm_t shm_glbls = ODP_SHM_INVALID; >>> odp_shm_t shm_log = ODP_SHM_INVALID; >>> int log_size, log_enries_num; >>> - odph_odpthread_t thread_tbl[MAX_WORKERS]; >>> + odph_odpthread_tbl_t thread_tbl; >>> odp_instance_t instance; >>> odph_odpthread_params_t thr_params; >>> >>> @@ -326,10 +326,10 @@ int main(int argc, char *argv[]) >>> thr_params.instance = instance; >>> >>> /* Create and launch worker threads */ >>> - odph_odpthreads_create(thread_tbl, &cpumask, &thr_params); >>> + odph_odpthreads_create(&thread_tbl, &cpumask, &thr_params); >>> >>> /* Wait for worker threads to exit */ >>> - odph_odpthreads_join(thread_tbl); >>> + odph_odpthreads_join(&thread_tbl); >>> >>> print_log(gbls); >>> >>> diff --git a/example/timer/odp_timer_test.c >>> b/example/timer/odp_timer_test.c >>> index cb58dfe..c594a9f 100644 >>> --- a/example/timer/odp_timer_test.c >>> +++ b/example/timer/odp_timer_test.c >>> @@ -330,7 +330,7 @@ static void parse_args(int argc, char *argv[], >>> test_args_t *args) >>> */ >>> int main(int argc, char *argv[]) >>> { >>> - odph_odpthread_t thread_tbl[MAX_WORKERS]; >>> + odph_odpthread_tbl_t thread_tbl; >>> int num_workers; >>> odp_queue_t queue; >>> uint64_t tick, ns; >>> @@ -393,8 +393,6 @@ int main(int argc, char *argv[]) >>> >>> parse_args(argc, argv, &gbls->args); >>> >>> - memset(thread_tbl, 0, sizeof(thread_tbl)); >>> - >>> /* Default to system CPU count unless user specified */ >>> num_workers = MAX_WORKERS; >>> if (gbls->args.cpu_count) >>> @@ -505,10 +503,10 @@ int main(int argc, char *argv[]) >>> thr_params.thr_type = ODP_THREAD_WORKER; >>> thr_params.instance = instance; >>> >>> - odph_odpthreads_create(thread_tbl, &cpumask, &thr_params); >>> + odph_odpthreads_create(&thread_tbl, &cpumask, &thr_params); >>> >>> /* Wait for worker threads to exit */ >>> - odph_odpthreads_join(thread_tbl); >>> + odph_odpthreads_join(&thread_tbl); >>> >>> /* free resources */ >>> if (odp_queue_destroy(queue)) >>> diff --git a/helper/include/odp/helper/linux.h >>> b/helper/include/odp/helper/linux.h >>> index 2e89833..2085768 100644 >>> --- a/helper/include/odp/helper/linux.h >>> +++ b/helper/include/odp/helper/linux.h >>> @@ -56,13 +56,6 @@ typedef struct { >>> int status; /**< Process state change status */ >>> } odph_linux_process_t; >>> >>> -/** odpthread linux type: whether an ODP thread is a linux thread or >>> process */ >>> -typedef enum odph_odpthread_linuxtype_e { >>> - ODPTHREAD_NOT_STARTED = 0, >>> - ODPTHREAD_PROCESS, >>> - ODPTHREAD_PTHREAD >>> -} odph_odpthread_linuxtype_t; >>> - >>> /** odpthread parameters for odp threads (pthreads and processes) */ >>> typedef struct { >>> int (*start)(void *); /**< Thread entry point function */ >>> @@ -71,28 +64,8 @@ typedef struct { >>> odp_instance_t instance; /**< ODP instance handle */ >>> } odph_odpthread_params_t; >>> >>> -/** The odpthread starting arguments, used both in process or thread >>> mode */ >>> -typedef struct { >>> - odph_odpthread_linuxtype_t linuxtype; /**< process or pthread */ >>> - odph_odpthread_params_t thr_params; /**< odpthread start >>> parameters */ >>> -} odph_odpthread_start_args_t; >>> - >>> -/** Linux odpthread state information, used both in process or thread >>> mode */ >>> -typedef struct { >>> - odph_odpthread_start_args_t start_args; /**< start arguments >>> */ >>> - int cpu; /**< CPU ID */ >>> - int last; /**< true if last table >>> entry */ >>> - union { >>> - struct { /* for thread implementation */ >>> - pthread_t thread_id; /**< Pthread ID */ >>> - pthread_attr_t attr; /**< Pthread attributes >>> */ >>> - } thread; >>> - struct { /* for process implementation */ >>> - pid_t pid; /**< Process ID */ >>> - int status; /**< Process state chge >>> status*/ >>> - } proc; >>> - }; >>> -} odph_odpthread_t; >>> +/** abstract type for odpthread arrays:*/ >>> +typedef void *odph_odpthread_tbl_t; >>> >>> >> Yes, understand, to expose only opaque odpthread representation for user >> interface. >> >> >>> /** >>> * Creates and launches pthreads >>> @@ -182,7 +155,7 @@ int odph_linux_process_wait_n(odph_linux_process_t >>> *proc_tbl, int num); >>> * >>> * @return Number of threads created >>> */ >>> -int odph_odpthreads_create(odph_odpthread_t *thread_tbl, >>> +int odph_odpthreads_create(odph_odpthread_tbl_t *thread_tbl_ptr, >>> const odp_cpumask_t *mask, >>> const odph_odpthread_params_t *thr_params); >>> >>> @@ -197,7 +170,7 @@ int odph_odpthreads_create(odph_odpthread_t >>> *thread_tbl, >>> * the thread join/process wait itself failed -e.g. as the result of a >>> kill) >>> * >>> */ >>> -int odph_odpthreads_join(odph_odpthread_t *thread_tbl); >>> +int odph_odpthreads_join(odph_odpthread_tbl_t *thread_tbl_ptr); >>> >>> /** >>> * Merge getopt options >>> diff --git a/helper/linux.c b/helper/linux.c >>> index 6366694..57cd8a7 100644 >>> --- a/helper/linux.c >>> +++ b/helper/linux.c >>> @@ -21,6 +21,36 @@ >>> #include <odp/helper/linux.h> >>> #include "odph_debug.h" >>> >>> +/** odpthread linux type: whether an ODP thread is a linux thread or >>> process */ >>> +typedef enum _odph_odpthread_linuxtype_e { >>> + ODPTHREAD_NOT_STARTED = 0, >>> + ODPTHREAD_PROCESS, >>> + ODPTHREAD_PTHREAD >>> +} _odph_odpthread_linuxtype_t; >>> + >>> +/** The odpthread starting arguments, used both in process or thread >>> mode */ >>> +typedef struct { >>> + _odph_odpthread_linuxtype_t linuxtype; >>> + odph_odpthread_params_t thr_params; /*copy of thread start >>> parameter*/ >>> +} _odph_odpthread_start_args_t; >>> + >>> +/** Linux odpthread state information, used both in process or thread >>> mode */ >>> +typedef struct { >>> + _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_id; /**< Pthread ID */ >>> + pthread_attr_t attr; /**< Pthread attributes >>> */ >>> + } thread; >>> + struct { /* for process implementation */ >>> + pid_t pid; /**< Process ID */ >>> + int status; /**< Process state chge >>> status*/ >>> + } proc; >>> + }; >>> +} _odph_odpthread_t; >>> + >>> >> >> Can name like _odph_odpthread_impl_t, only a suggestion for your >> consideration :). >> > > I can change that in V2, sure! > > >> >> >>> static struct { >>> int proc; /* true when process mode is required, false otherwise >>> */ >>> } helper_options; >>> @@ -251,7 +281,7 @@ static void *odpthread_run_start_routine(void *arg) >>> int ret; >>> odph_odpthread_params_t *thr_params; >>> >>> - odph_odpthread_start_args_t *start_args = arg; >>> + _odph_odpthread_start_args_t *start_args = arg; >>> >>> thr_params = &start_args->thr_params; >>> >>> @@ -289,7 +319,7 @@ static void *odpthread_run_start_routine(void *arg) >>> /* >>> * Create a single ODPthread as a linux process >>> */ >>> -static int odph_linux_process_create(odph_odpthread_t *thread_tbl, >>> +static int odph_linux_process_create(_odph_odpthread_t *thread_tbl, >>> int cpu, >>> const odph_odpthread_params_t >>> *thr_params) >>> { >>> @@ -338,7 +368,7 @@ static int >>> odph_linux_process_create(odph_odpthread_t *thread_tbl, >>> /* >>> * Create a single ODPthread as a linux thread >>> */ >>> -static int odph_linux_thread_create(odph_odpthread_t *thread_tbl, >>> +static int odph_linux_thread_create(_odph_odpthread_t *thread_tbl, >>> int cpu, >>> const odph_odpthread_params_t >>> *thr_params) >>> { >>> @@ -374,7 +404,7 @@ static int odph_linux_thread_create(odph_odpthread_t >>> *thread_tbl, >>> /* >>> * create an odpthread set (as linux processes or linux threads or both) >>> */ >>> -int odph_odpthreads_create(odph_odpthread_t *thread_tbl, >>> +int odph_odpthreads_create(odph_odpthread_tbl_t *thread_tbl_ptr, >>> const odp_cpumask_t *mask, >>> const odph_odpthread_params_t *thr_params) >>> { >>> @@ -382,11 +412,10 @@ int odph_odpthreads_create(odph_odpthread_t >>> *thread_tbl, >>> int num; >>> int cpu_count; >>> int cpu; >>> + _odph_odpthread_t *thread_tbl; >>> >>> num = odp_cpumask_count(mask); >>> >>> - memset(thread_tbl, 0, num * sizeof(odph_odpthread_t)); >>> - >>> cpu_count = odp_cpu_count(); >>> >>> if (num < 1 || num > cpu_count) { >>> @@ -396,6 +425,11 @@ int odph_odpthreads_create(odph_odpthread_t >>> *thread_tbl, >>> return -1; >>> } >>> >>> + thread_tbl = malloc(num * sizeof(_odph_odpthread_t)); >>> + *thread_tbl_ptr = (void *)thread_tbl; >>> + >>> + memset(thread_tbl, 0, num * sizeof(_odph_odpthread_t)); >>> + >>> >> >> Need a check for potential malloc failures. >> >> >>> cpu = odp_cpumask_first(mask); >>> for (i = 0; i < num; i++) { >>> if (!helper_options.proc) { >>> @@ -420,8 +454,9 @@ int odph_odpthreads_create(odph_odpthread_t >>> *thread_tbl, >>> /* >>> * wait for the odpthreads termination (linux processes and threads) >>> */ >>> -int odph_odpthreads_join(odph_odpthread_t *thread_tbl) >>> +int odph_odpthreads_join(odph_odpthread_tbl_t *thread_tbl_ptr) >>> { >>> + _odph_odpthread_t *thread_tbl; >>> pid_t pid; >>> int i = 0; >>> int terminated = 0; >>> @@ -430,6 +465,13 @@ int odph_odpthreads_join(odph_odpthread_t >>> *thread_tbl) >>> int ret; >>> int retval = 0; >>> >>> + thread_tbl = (_odph_odpthread_t *)*thread_tbl_ptr; >>> + >>> + if (!thread_tbl) { >>> + ODPH_ERR("Attempt to join thread from invalid table\n"); >>> + return -1; >>> + } >>> + >>> /* joins linux threads or wait for processes */ >>> do { >>> /* pthreads: */ >>> @@ -489,7 +531,10 @@ int odph_odpthreads_join(odph_odpthread_t >>> *thread_tbl) >>> >>> } while (!thread_tbl[i++].last); >>> >>> - return (retval < 0) ? retval : terminated; >>> + /* free the allocated table: */ >>> + free(*thread_tbl_ptr); >>> + >>> + return (retval < 0) ? retval : i; >>> } >>> >>> /* >>> diff --git a/helper/test/odpthreads.c b/helper/test/odpthreads.c >>> index bba4fa5..489ecaa 100644 >>> --- a/helper/test/odpthreads.c >>> +++ b/helper/test/odpthreads.c >>> @@ -31,7 +31,7 @@ int main(int argc, char *argv[]) >>> { >>> odp_instance_t instance; >>> odph_odpthread_params_t thr_params; >>> - odph_odpthread_t thread_tbl[NUMBER_WORKERS]; >>> + odph_odpthread_tbl_t thread_tbl; >>> odp_cpumask_t cpu_mask; >>> int num_workers; >>> int cpu; >>> @@ -80,9 +80,9 @@ int main(int argc, char *argv[]) >>> thr_params.thr_type = ODP_THREAD_WORKER; >>> thr_params.instance = instance; >>> >>> - odph_odpthreads_create(&thread_tbl[0], &cpu_mask, &thr_params); >>> + odph_odpthreads_create(&thread_tbl, &cpu_mask, &thr_params); >>> >>> - ret = odph_odpthreads_join(thread_tbl); >>> + ret = odph_odpthreads_join(&thread_tbl); >>> if (ret < 0) >>> exit(EXIT_FAILURE); >>> >>> diff --git a/test/performance/odp_crypto.c >>> b/test/performance/odp_crypto.c >>> index 404984d..b4ce793 100644 >>> --- a/test/performance/odp_crypto.c >>> +++ b/test/performance/odp_crypto.c >>> @@ -724,7 +724,7 @@ int main(int argc, char *argv[]) >>> odp_cpumask_t cpumask; >>> char cpumaskstr[ODP_CPUMASK_STR_SIZE]; >>> int num_workers = 1; >>> - odph_odpthread_t thr[num_workers]; >>> + odph_odpthread_tbl_t thr; >>> odp_instance_t instance; >>> >>> memset(&cargs, 0, sizeof(cargs)); >>> @@ -794,8 +794,6 @@ int main(int argc, char *argv[]) >>> printf("Run in sync mode\n"); >>> } >>> >>> - memset(thr, 0, sizeof(thr)); >>> - >>> if (cargs.alg_config) { >>> odph_odpthread_params_t thr_params; >>> >>> @@ -806,8 +804,8 @@ int main(int argc, char *argv[]) >>> thr_params.instance = instance; >>> >>> if (cargs.schedule) { >>> - odph_odpthreads_create(&thr[0], &cpumask, >>> &thr_params); >>> - odph_odpthreads_join(&thr[0]); >>> + odph_odpthreads_create(&thr, &cpumask, >>> &thr_params); >>> + odph_odpthreads_join(&thr); >>> } else { >>> run_measure_one_config(&cargs, cargs.alg_config); >>> } >>> diff --git a/test/performance/odp_l2fwd.c b/test/performance/odp_l2fwd.c >>> index e38e6f8..28874d1 100644 >>> --- a/test/performance/odp_l2fwd.c >>> +++ b/test/performance/odp_l2fwd.c >>> @@ -1293,7 +1293,7 @@ static void gbl_args_init(args_t *args) >>> */ >>> int main(int argc, char *argv[]) >>> { >>> - odph_odpthread_t thread_tbl[MAX_WORKERS]; >>> + odph_odpthread_tbl_t thread_tbl[MAX_WORKERS]; >>> odp_pool_t pool; >>> int i; >>> int cpu; >>> @@ -1424,8 +1424,6 @@ int main(int argc, char *argv[]) >>> gbl_args->appl.in_mode == PLAIN_QUEUE) >>> print_port_mapping(); >>> >>> - memset(thread_tbl, 0, sizeof(thread_tbl)); >>> - >>> stats = gbl_args->stats; >>> >>> odp_barrier_init(&barrier, num_workers + 1); >>> diff --git a/test/performance/odp_pktio_perf.c >>> b/test/performance/odp_pktio_perf.c >>> index 98ec681..bda7d95 100644 >>> --- a/test/performance/odp_pktio_perf.c >>> +++ b/test/performance/odp_pktio_perf.c >>> @@ -601,10 +601,11 @@ static int run_test_single(odp_cpumask_t >>> *thd_mask_tx, >>> odp_cpumask_t *thd_mask_rx, >>> test_status_t *status) >>> { >>> - odph_odpthread_t thd_tbl[MAX_WORKERS]; >>> + odph_odpthread_tbl_t rx_thd_tbl; >>> + odph_odpthread_tbl_t tx_thd_tbl; >>> thread_args_t args_tx, args_rx; >>> uint64_t expected_tx_cnt; >>> - int num_tx_workers, num_rx_workers; >>> + int num_tx_workers; >>> odph_odpthread_params_t thr_params; >>> >>> memset(&thr_params, 0, sizeof(thr_params)); >>> @@ -613,7 +614,6 @@ static int run_test_single(odp_cpumask_t >>> *thd_mask_tx, >>> >>> odp_atomic_store_u32(&shutdown, 0); >>> >>> - memset(thd_tbl, 0, sizeof(thd_tbl)); >>> memset(gbl_args->rx_stats, 0, gbl_args->rx_stats_size); >>> memset(gbl_args->tx_stats, 0, gbl_args->tx_stats_size); >>> >>> @@ -623,9 +623,8 @@ static int run_test_single(odp_cpumask_t >>> *thd_mask_tx, >>> thr_params.start = run_thread_rx; >>> thr_params.arg = &args_rx; >>> args_rx.batch_len = gbl_args->args.rx_batch_len; >>> - odph_odpthreads_create(&thd_tbl[0], thd_mask_rx, &thr_params); >>> + odph_odpthreads_create(&rx_thd_tbl, thd_mask_rx, &thr_params); >>> odp_barrier_wait(&gbl_args->rx_barrier); >>> - num_rx_workers = odp_cpumask_count(thd_mask_rx); >>> >>> /* then start transmitters */ >>> thr_params.start = run_thread_tx; >>> @@ -634,12 +633,12 @@ static int run_test_single(odp_cpumask_t >>> *thd_mask_tx, >>> args_tx.pps = status->pps_curr / num_tx_workers; >>> args_tx.duration = gbl_args->args.duration; >>> args_tx.batch_len = gbl_args->args.tx_batch_len; >>> - odph_odpthreads_create(&thd_tbl[num_rx_workers], thd_mask_tx, >>> + odph_odpthreads_create(&tx_thd_tbl, thd_mask_tx, >>> &thr_params); >>> odp_barrier_wait(&gbl_args->tx_barrier); >>> >>> /* wait for transmitter threads to terminate */ >>> - odph_odpthreads_join(&thd_tbl[num_rx_workers]); >>> + odph_odpthreads_join(&tx_thd_tbl); >>> >>> /* delay to allow transmitted packets to reach the receivers */ >>> odp_time_wait_ns(SHUTDOWN_DELAY_NS); >>> @@ -648,7 +647,7 @@ static int run_test_single(odp_cpumask_t >>> *thd_mask_tx, >>> odp_atomic_store_u32(&shutdown, 1); >>> >>> /* wait for receivers */ >>> - odph_odpthreads_join(&thd_tbl[0]); >>> + odph_odpthreads_join(&rx_thd_tbl); >>> >>> if (!status->warmup) >>> return process_results(expected_tx_cnt, status); >>> diff --git a/test/performance/odp_scheduling.c >>> b/test/performance/odp_scheduling.c >>> index 1d3bfd1..b1b2a86 100644 >>> --- a/test/performance/odp_scheduling.c >>> +++ b/test/performance/odp_scheduling.c >>> @@ -775,7 +775,7 @@ static void parse_args(int argc, char *argv[], >>> test_args_t *args) >>> */ >>> int main(int argc, char *argv[]) >>> { >>> - odph_odpthread_t thread_tbl[MAX_WORKERS]; >>> + odph_odpthread_tbl_t thread_tbl; >>> test_args_t args; >>> int num_workers; >>> odp_cpumask_t cpumask; >>> @@ -795,8 +795,6 @@ int main(int argc, char *argv[]) >>> memset(&args, 0, sizeof(args)); >>> parse_args(argc, argv, &args); >>> >>> - memset(thread_tbl, 0, sizeof(thread_tbl)); >>> - >>> /* ODP global init */ >>> if (odp_init_global(&instance, NULL, NULL)) { >>> LOG_ERR("ODP global init failed.\n"); >>> @@ -943,10 +941,10 @@ int main(int argc, char *argv[]) >>> thr_params.instance = instance; >>> thr_params.start = run_thread; >>> thr_params.arg = NULL; >>> - odph_odpthreads_create(thread_tbl, &cpumask, &thr_params); >>> + odph_odpthreads_create(&thread_tbl, &cpumask, &thr_params); >>> >>> /* Wait for worker threads to terminate */ >>> - odph_odpthreads_join(thread_tbl); >>> + odph_odpthreads_join(&thread_tbl); >>> >>> printf("ODP example complete\n\n"); >>> >>> diff --git a/test/validation/common/odp_cunit_common.c >>> b/test/validation/common/odp_cunit_common.c >>> index 7df9aa6..cef0f4d 100644 >>> --- a/test/validation/common/odp_cunit_common.c >>> +++ b/test/validation/common/odp_cunit_common.c >>> @@ -9,7 +9,7 @@ >>> #include <odp_cunit_common.h> >>> #include <odp/helper/linux.h> >>> /* Globals */ >>> -static odph_odpthread_t thread_tbl[MAX_WORKERS]; >>> +static odph_odpthread_tbl_t thread_tbl; >>> static odp_instance_t instance; >>> >>> /* >>> @@ -40,14 +40,14 @@ int odp_cunit_thread_create(int func_ptr(void *), >>> pthrd_arg *arg) >>> /* Create and init additional threads */ >>> odp_cpumask_default_worker(&cpumask, arg->numthrds); >>> >>> - return odph_odpthreads_create(thread_tbl, &cpumask, &thr_params); >>> + return odph_odpthreads_create(&thread_tbl, &cpumask, >>> &thr_params); >>> } >>> >>> /** exit from test thread */ >>> int odp_cunit_thread_exit(pthrd_arg *arg) >>> { >>> /* Wait for other threads to exit */ >>> - if (odph_odpthreads_join(thread_tbl) != arg->numthrds) { >>> + if (odph_odpthreads_join(&thread_tbl) != arg->numthrds) { >>> fprintf(stderr, >>> "error: odph_odpthreads_join() failed.\n"); >>> return -1; >>> -- >>> 2.5.0 >>> >>> >> Tell me if my answer make sense and if you have other comments, I will > put the _odph_odpthread_t to _odph_odpthread_impl_t name change for v2 > > Christophe. >
On 26 May 2016 at 14:25, Yi He <yi.he@linaro.org> wrote: > > > On 26 May 2016 at 18:39, Christophe Milard <christophe.milard@linaro.org> > wrote: > >> >> >> On 26 May 2016 at 11:50, Yi He <yi.he@linaro.org> wrote: >> >>> >>> >>> On 25 May 2016 at 21:12, Christophe Milard <christophe.milard@linaro.org >>> > wrote: >>> >>>> The odph_odpthread_t (non opaque array of odpthread) is now replaced by >>>> a >>>> single type odph_odpthread_tbl_t abstracted as a void*. >>>> The table describing the odpthreads being created is now malloc'd and >>>> freed by the helper function themselves. >>>> >>>> Signed-off-by: Christophe Milard <christophe.milard@linaro.org> >>>> --- >>>> example/classifier/odp_classifier.c | 9 ++--- >>>> example/generator/odp_generator.c | 5 +-- >>>> example/ipsec/odp_ipsec.c | 6 +-- >>>> example/l2fwd_simple/odp_l2fwd_simple.c | 2 +- >>>> example/packet/odp_pktio.c | 5 +-- >>>> example/switch/odp_switch.c | 4 +- >>>> example/time/time_global_test.c | 6 +-- >>>> example/timer/odp_timer_test.c | 8 ++-- >>>> helper/include/odp/helper/linux.h | 35 ++---------------- >>>> helper/linux.c | 61 >>>> +++++++++++++++++++++++++++---- >>>> helper/test/odpthreads.c | 6 +-- >>>> test/performance/odp_crypto.c | 8 ++-- >>>> test/performance/odp_l2fwd.c | 4 +- >>>> test/performance/odp_pktio_perf.c | 15 ++++---- >>>> test/performance/odp_scheduling.c | 8 ++-- >>>> test/validation/common/odp_cunit_common.c | 6 +-- >>>> 16 files changed, 93 insertions(+), 95 deletions(-) >>>> >>>> diff --git a/example/classifier/odp_classifier.c >>>> b/example/classifier/odp_classifier.c >>>> index 20e64ec..7512ec6 100644 >>>> --- a/example/classifier/odp_classifier.c >>>> +++ b/example/classifier/odp_classifier.c >>>> @@ -469,7 +469,7 @@ static void configure_cos(odp_cos_t default_cos, >>>> appl_args_t *args) >>>> */ >>> >>> int main(int argc, char *argv[]) >>>> { >>>> - odph_odpthread_t thread_tbl[MAX_WORKERS]; >>>> + odph_odpthread_tbl_t thread_tbl; >>>> odp_pool_t pool; >>>> int num_workers; >>>> int i; >>>> @@ -562,21 +562,18 @@ int main(int argc, char *argv[]) >>>> exit(EXIT_FAILURE); >>>> } >>>> >>>> - /* Create and init worker threads */ >>>> - memset(thread_tbl, 0, sizeof(thread_tbl)); >>>> - >>>> memset(&thr_params, 0, sizeof(thr_params)); >>>> thr_params.start = pktio_receive_thread; >>>> thr_params.arg = args; >>>> thr_params.thr_type = ODP_THREAD_WORKER; >>>> thr_params.instance = instance; >>>> - odph_odpthreads_create(thread_tbl, &cpumask, &thr_params); >>>> + odph_odpthreads_create(&thread_tbl, &cpumask, &thr_params); >>>> >>>> print_cls_statistics(args); >>>> >>>> odp_pktio_stop(pktio); >>>> shutdown = 1; >>>> - odph_odpthreads_join(thread_tbl); >>>> + odph_odpthreads_join(&thread_tbl); >>>> >>>> for (i = 0; i < args->policy_count; i++) { >>>> if ((i != args->policy_count - 1) && >>>> diff --git a/example/generator/odp_generator.c >>>> b/example/generator/odp_generator.c >>>> index ccae907..e29b008 100644 >>>> --- a/example/generator/odp_generator.c >>>> +++ b/example/generator/odp_generator.c >>>> @@ -621,7 +621,7 @@ static void print_global_stats(int num_workers) >>>> */ >>>> int main(int argc, char *argv[]) >>>> { >>>> - odph_odpthread_t thread_tbl[MAX_WORKERS]; >>>> + odph_odpthread_tbl_t thread_tbl[MAX_WORKERS]; >>>> >>> >>> understanding the overall goals: >>> * to make odpthread data structure opaque to application developer >>> * and dispel the duty of odpthread data structure allocation/free >>> from application developer. >>> review comments: >>> is odph_odpthread_tbl_t a proper name? >>> like in this example and several others, to create threads of >>> different functions, it still invokes >>> odph_odpthread_create() separately for each, thus makes the above >>> name not intuitively. >>> >> No, I think you misunderstood the thing there (or I was not clear >>> enough). You do invoke odph_odpthread_create() only once for a complete >>> set of threads, hence the tbl name. If you look at the modification for the >>> classifier, for instance, you will notice that >>> >> odph_odpthread_t thread_tbl[MAX_WORKERS]; >> is changed to: >> odph_odpthread_tbl_t thread_tbl; >> > This is indeed because odph_odpthread_tbl_t is the table. >> Some other tests, such as the odp generator, for instance keep the array: >> odph_odpthread_t thread_tbl[MAX_WORKERS]; >> becomes >> odph_odpthread_tbl_t thread_tbl[MAX_WORKERS]; >> This is due to the fact that the different threads do not share their >> start parameters (such as the function to be run). In that case, the caller >> decides to handle N table of 1 entries rather than 1 table of N entries. >> There is , of course, nothing preventing you to create tables of size 1. >> But these are tables anyway! If I can do anything to avoid the confusion, >> please tell. But I thing that _tbl (table) name is appropriate. >> > > *Hi, Christophe, my major concern lies in this user case, that "*N table > of 1 entries" seems not very straightforward. *I accept yours.* > Ok, thanks > > suggestions: >>> instead of naming opaque odph_odpthread_tbl_t, define >>> odph_odpthread_t as opaque void *; >>> in examples which fan out identical workers, can code like: >>> * odph_odpthread_t *thread_tbl = NULL;* >>> * ....* >>> * odph_odpthread_create(thread_tbl, cpuset (batch), ...);* >>> in examples which spawn multiple different functional workers, can >>> code like: >>> * odph_odpthread_t thread_tbl[WORKERS_NUMBER];* >>> * ....* >>> * odph_odpthread_create(&thread_tbl[i], cpuset(each), ...);* >>> >> >> I am confused here: would thread_tbl[i] be the entry for one or many >> threads?? The mask passed as parameter to odph_odpthread_create may >> contain many bits set in which case many threads are created... >> > >>> odp_pool_t pool; >>>> int num_workers; >>>> int i; >>>> @@ -746,9 +746,6 @@ int main(int argc, char *argv[]) >>>> for (i = 0; i < args->appl.if_count; ++i) >>>> pktio[i] = create_pktio(args->appl.if_names[i], pool); >>>> >>>> - /* Create and init worker threads */ >>>> - memset(thread_tbl, 0, sizeof(thread_tbl)); >>>> - >>>> >>> >>> In this example program and several others, this seems still good >>> practice, but not mandatory. >>> >> >> Not sure I understand: would you like to see thread_tbl=NULL here? maybe >> this gets clearer once we agree on what thread_tbl should be! >> >> > Here I mean in this example defines: > *odph_odpthread_tbl_t thread_tbl[MAX_WORKERS];* > so a memset is still needed, right? > > hmmm interresting... :-) If you think "odph_odpthread_tbl_t thread_tbl[MAX_WORKERS];" deserves a memset(), set, I assume "odph_odpthread_tbl_t thread_tbl;" should deserve a memset() as well as there is no reason that a table of MAX_WORKERS odpthread sets would deserve more initialisation than a table of 1 such set. Now the odph_odpthread_tbl_t type is opaque to the test/application. You happen to know it is a pointer, but really the application should not assume anything. So why would a memset() be appropriate? I guess, if we want to do things properly, we should define a odph_odpthread_tbl_init() function in the helpers. Now what would be the meaning of such a function: initialising a odph_odpthread_tbl_t to a nice value... isn't it what odph_odpthread_create() does? Yes! odph_odpthread_create() just does that... So I don't really see the added value of such a function. Think of the following C code: int i; for (i=0; i<X; i++){... You wouldn't require the first line (int i;) to be "int i=0;", would you? Calling a first function to set the value of an odph_odpthread_tbl_t object before calling another function to set another value to the same object does not really make sense to me. The interrest of such memset is to cover the case of unitialised fields of struct/union: this is covered by the memset at line 429 of linux.c I have also added *thread_tbl_ptr = NULL; in odph_odpthreads_create() in case of error so that the odph_odpthread_tbl_t object always gets defined. I think this answer better your legitimate worry of initiatisation as linux.h does know about the appropiate init value I will send a V2 with this change. Hopefully that answers your legitimate concern. if not let me know. Christophe >>> >>>> /* Init threads params */ >>>> memset(&thr_params, 0, sizeof(thr_params)); >>>> thr_params.thr_type = ODP_THREAD_WORKER; >>>> diff --git a/example/ipsec/odp_ipsec.c b/example/ipsec/odp_ipsec.c >>>> index fb4385f..95b907f 100644 >>>> --- a/example/ipsec/odp_ipsec.c >>>> +++ b/example/ipsec/odp_ipsec.c >>>> @@ -1212,7 +1212,7 @@ int pktio_thread(void *arg EXAMPLE_UNUSED) >>>> int >>>> main(int argc, char *argv[]) >>>> { >>>> - odph_odpthread_t thread_tbl[MAX_WORKERS]; >>>> + odph_odpthread_tbl_t thread_tbl; >>>> int num_workers; >>>> int i; >>>> int stream_count; >>>> @@ -1341,7 +1341,7 @@ main(int argc, char *argv[]) >>>> thr_params.arg = NULL; >>>> thr_params.thr_type = ODP_THREAD_WORKER; >>>> thr_params.instance = instance; >>>> - odph_odpthreads_create(thread_tbl, &cpumask, &thr_params); >>>> + odph_odpthreads_create(&thread_tbl, &cpumask, &thr_params); >>>> >>>> /* >>>> * If there are streams attempt to verify them else >>>> @@ -1355,7 +1355,7 @@ main(int argc, char *argv[]) >>>> } while (!done); >>>> printf("All received\n"); >>>> } else { >>>> - odph_odpthreads_join(thread_tbl); >>>> + odph_odpthreads_join(&thread_tbl); >>>> } >>>> >>>> free(args->appl.if_names); >>>> diff --git a/example/l2fwd_simple/odp_l2fwd_simple.c >>>> b/example/l2fwd_simple/odp_l2fwd_simple.c >>>> index daae038..8bd51e1 100644 >>>> --- a/example/l2fwd_simple/odp_l2fwd_simple.c >>>> +++ b/example/l2fwd_simple/odp_l2fwd_simple.c >>>> @@ -119,7 +119,7 @@ int main(int argc, char **argv) >>>> odp_pool_t pool; >>>> odp_pool_param_t params; >>>> odp_cpumask_t cpumask; >>>> - odph_odpthread_t thd; >>>> + odph_odpthread_tbl_t thd; >>>> odp_instance_t instance; >>>> odph_odpthread_params_t thr_params; >>>> int opt; >>>> diff --git a/example/packet/odp_pktio.c b/example/packet/odp_pktio.c >>>> index 9028ab2..cab9da2 100644 >>>> --- a/example/packet/odp_pktio.c >>>> +++ b/example/packet/odp_pktio.c >>>> @@ -342,7 +342,7 @@ static int pktio_ifburst_thread(void *arg) >>>> */ >>>> int main(int argc, char *argv[]) >>>> { >>>> - odph_odpthread_t thread_tbl[MAX_WORKERS]; >>>> + odph_odpthread_tbl_t thread_tbl[MAX_WORKERS]; >>>> odp_pool_t pool; >>>> int num_workers; >>>> int i; >>>> @@ -409,9 +409,6 @@ int main(int argc, char *argv[]) >>>> for (i = 0; i < args->appl.if_count; ++i) >>>> create_pktio(args->appl.if_names[i], pool, >>>> args->appl.mode); >>>> >>>> - /* Create and init worker threads */ >>>> - memset(thread_tbl, 0, sizeof(thread_tbl)); >>>> - >>>> memset(&thr_params, 0, sizeof(thr_params)); >>>> thr_params.thr_type = ODP_THREAD_WORKER; >>>> thr_params.instance = instance; >>>> diff --git a/example/switch/odp_switch.c b/example/switch/odp_switch.c >>>> index 0c9b257..5cfc742 100644 >>>> --- a/example/switch/odp_switch.c >>>> +++ b/example/switch/odp_switch.c >>>> @@ -884,7 +884,7 @@ static void gbl_args_init(args_t *args) >>>> >>>> int main(int argc, char **argv) >>>> { >>>> - odph_odpthread_t thread_tbl[MAX_WORKERS]; >>>> + odph_odpthread_tbl_t thread_tbl[MAX_WORKERS]; >>>> int i, j; >>>> int cpu; >>>> int num_workers; >>>> @@ -986,8 +986,6 @@ int main(int argc, char **argv) >>>> >>>> print_port_mapping(); >>>> >>>> - memset(thread_tbl, 0, sizeof(thread_tbl)); >>>> - >>>> odp_barrier_init(&barrier, num_workers + 1); >>>> >>>> stats = gbl_args->stats; >>>> diff --git a/example/time/time_global_test.c >>>> b/example/time/time_global_test.c >>>> index 372d96b..8f80149 100644 >>>> --- a/example/time/time_global_test.c >>>> +++ b/example/time/time_global_test.c >>>> @@ -252,7 +252,7 @@ int main(int argc, char *argv[]) >>>> odp_shm_t shm_glbls = ODP_SHM_INVALID; >>>> odp_shm_t shm_log = ODP_SHM_INVALID; >>>> int log_size, log_enries_num; >>>> - odph_odpthread_t thread_tbl[MAX_WORKERS]; >>>> + odph_odpthread_tbl_t thread_tbl; >>>> odp_instance_t instance; >>>> odph_odpthread_params_t thr_params; >>>> >>>> @@ -326,10 +326,10 @@ int main(int argc, char *argv[]) >>>> thr_params.instance = instance; >>>> >>>> /* Create and launch worker threads */ >>>> - odph_odpthreads_create(thread_tbl, &cpumask, &thr_params); >>>> + odph_odpthreads_create(&thread_tbl, &cpumask, &thr_params); >>>> >>>> /* Wait for worker threads to exit */ >>>> - odph_odpthreads_join(thread_tbl); >>>> + odph_odpthreads_join(&thread_tbl); >>>> >>>> print_log(gbls); >>>> >>>> diff --git a/example/timer/odp_timer_test.c >>>> b/example/timer/odp_timer_test.c >>>> index cb58dfe..c594a9f 100644 >>>> --- a/example/timer/odp_timer_test.c >>>> +++ b/example/timer/odp_timer_test.c >>>> @@ -330,7 +330,7 @@ static void parse_args(int argc, char *argv[], >>>> test_args_t *args) >>>> */ >>>> int main(int argc, char *argv[]) >>>> { >>>> - odph_odpthread_t thread_tbl[MAX_WORKERS]; >>>> + odph_odpthread_tbl_t thread_tbl; >>>> int num_workers; >>>> odp_queue_t queue; >>>> uint64_t tick, ns; >>>> @@ -393,8 +393,6 @@ int main(int argc, char *argv[]) >>>> >>>> parse_args(argc, argv, &gbls->args); >>>> >>>> - memset(thread_tbl, 0, sizeof(thread_tbl)); >>>> - >>>> /* Default to system CPU count unless user specified */ >>>> num_workers = MAX_WORKERS; >>>> if (gbls->args.cpu_count) >>>> @@ -505,10 +503,10 @@ int main(int argc, char *argv[]) >>>> thr_params.thr_type = ODP_THREAD_WORKER; >>>> thr_params.instance = instance; >>>> >>>> - odph_odpthreads_create(thread_tbl, &cpumask, &thr_params); >>>> + odph_odpthreads_create(&thread_tbl, &cpumask, &thr_params); >>>> >>>> /* Wait for worker threads to exit */ >>>> - odph_odpthreads_join(thread_tbl); >>>> + odph_odpthreads_join(&thread_tbl); >>>> >>>> /* free resources */ >>>> if (odp_queue_destroy(queue)) >>>> diff --git a/helper/include/odp/helper/linux.h >>>> b/helper/include/odp/helper/linux.h >>>> index 2e89833..2085768 100644 >>>> --- a/helper/include/odp/helper/linux.h >>>> +++ b/helper/include/odp/helper/linux.h >>>> @@ -56,13 +56,6 @@ typedef struct { >>>> int status; /**< Process state change status */ >>>> } odph_linux_process_t; >>>> >>>> -/** odpthread linux type: whether an ODP thread is a linux thread or >>>> process */ >>>> -typedef enum odph_odpthread_linuxtype_e { >>>> - ODPTHREAD_NOT_STARTED = 0, >>>> - ODPTHREAD_PROCESS, >>>> - ODPTHREAD_PTHREAD >>>> -} odph_odpthread_linuxtype_t; >>>> - >>>> /** odpthread parameters for odp threads (pthreads and processes) */ >>>> typedef struct { >>>> int (*start)(void *); /**< Thread entry point function */ >>>> @@ -71,28 +64,8 @@ typedef struct { >>>> odp_instance_t instance; /**< ODP instance handle */ >>>> } odph_odpthread_params_t; >>>> >>>> -/** The odpthread starting arguments, used both in process or thread >>>> mode */ >>>> -typedef struct { >>>> - odph_odpthread_linuxtype_t linuxtype; /**< process or pthread */ >>>> - odph_odpthread_params_t thr_params; /**< odpthread start >>>> parameters */ >>>> -} odph_odpthread_start_args_t; >>>> - >>>> -/** Linux odpthread state information, used both in process or thread >>>> mode */ >>>> -typedef struct { >>>> - odph_odpthread_start_args_t start_args; /**< start >>>> arguments */ >>>> - int cpu; /**< CPU ID */ >>>> - int last; /**< true if last table >>>> entry */ >>>> - union { >>>> - struct { /* for thread implementation */ >>>> - pthread_t thread_id; /**< Pthread ID */ >>>> - pthread_attr_t attr; /**< Pthread attributes >>>> */ >>>> - } thread; >>>> - struct { /* for process implementation */ >>>> - pid_t pid; /**< Process ID */ >>>> - int status; /**< Process state chge >>>> status*/ >>>> - } proc; >>>> - }; >>>> -} odph_odpthread_t; >>>> +/** abstract type for odpthread arrays:*/ >>>> +typedef void *odph_odpthread_tbl_t; >>>> >>>> >>> Yes, understand, to expose only opaque odpthread representation for user >>> interface. >>> >>> >>>> /** >>>> * Creates and launches pthreads >>>> @@ -182,7 +155,7 @@ int odph_linux_process_wait_n(odph_linux_process_t >>>> *proc_tbl, int num); >>>> * >>>> * @return Number of threads created >>>> */ >>>> -int odph_odpthreads_create(odph_odpthread_t *thread_tbl, >>>> +int odph_odpthreads_create(odph_odpthread_tbl_t *thread_tbl_ptr, >>>> const odp_cpumask_t *mask, >>>> const odph_odpthread_params_t *thr_params); >>>> >>>> @@ -197,7 +170,7 @@ int odph_odpthreads_create(odph_odpthread_t >>>> *thread_tbl, >>>> * the thread join/process wait itself failed -e.g. as the result of >>>> a kill) >>>> * >>>> */ >>>> -int odph_odpthreads_join(odph_odpthread_t *thread_tbl); >>>> +int odph_odpthreads_join(odph_odpthread_tbl_t *thread_tbl_ptr); >>>> >>>> /** >>>> * Merge getopt options >>>> diff --git a/helper/linux.c b/helper/linux.c >>>> index 6366694..57cd8a7 100644 >>>> --- a/helper/linux.c >>>> +++ b/helper/linux.c >>>> @@ -21,6 +21,36 @@ >>>> #include <odp/helper/linux.h> >>>> #include "odph_debug.h" >>>> >>>> +/** odpthread linux type: whether an ODP thread is a linux thread or >>>> process */ >>>> +typedef enum _odph_odpthread_linuxtype_e { >>>> + ODPTHREAD_NOT_STARTED = 0, >>>> + ODPTHREAD_PROCESS, >>>> + ODPTHREAD_PTHREAD >>>> +} _odph_odpthread_linuxtype_t; >>>> + >>>> +/** The odpthread starting arguments, used both in process or thread >>>> mode */ >>>> +typedef struct { >>>> + _odph_odpthread_linuxtype_t linuxtype; >>>> + odph_odpthread_params_t thr_params; /*copy of thread start >>>> parameter*/ >>>> +} _odph_odpthread_start_args_t; >>>> + >>>> +/** Linux odpthread state information, used both in process or thread >>>> mode */ >>>> +typedef struct { >>>> + _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_id; /**< Pthread ID */ >>>> + pthread_attr_t attr; /**< Pthread attributes >>>> */ >>>> + } thread; >>>> + struct { /* for process implementation */ >>>> + pid_t pid; /**< Process ID */ >>>> + int status; /**< Process state chge >>>> status*/ >>>> + } proc; >>>> + }; >>>> +} _odph_odpthread_t; >>>> + >>>> >>> >>> Can name like _odph_odpthread_impl_t, only a suggestion for your >>> consideration :). >>> >> >> I can change that in V2, sure! >> >> >>> >>> >>>> static struct { >>>> int proc; /* true when process mode is required, false >>>> otherwise */ >>>> } helper_options; >>>> @@ -251,7 +281,7 @@ static void *odpthread_run_start_routine(void *arg) >>>> int ret; >>>> odph_odpthread_params_t *thr_params; >>>> >>>> - odph_odpthread_start_args_t *start_args = arg; >>>> + _odph_odpthread_start_args_t *start_args = arg; >>>> >>>> thr_params = &start_args->thr_params; >>>> >>>> @@ -289,7 +319,7 @@ static void *odpthread_run_start_routine(void *arg) >>>> /* >>>> * Create a single ODPthread as a linux process >>>> */ >>>> -static int odph_linux_process_create(odph_odpthread_t *thread_tbl, >>>> +static int odph_linux_process_create(_odph_odpthread_t *thread_tbl, >>>> int cpu, >>>> const odph_odpthread_params_t >>>> *thr_params) >>>> { >>>> @@ -338,7 +368,7 @@ static int >>>> odph_linux_process_create(odph_odpthread_t *thread_tbl, >>>> /* >>>> * Create a single ODPthread as a linux thread >>>> */ >>>> -static int odph_linux_thread_create(odph_odpthread_t *thread_tbl, >>>> +static int odph_linux_thread_create(_odph_odpthread_t *thread_tbl, >>>> int cpu, >>>> const odph_odpthread_params_t >>>> *thr_params) >>>> { >>>> @@ -374,7 +404,7 @@ static int >>>> odph_linux_thread_create(odph_odpthread_t *thread_tbl, >>>> /* >>>> * create an odpthread set (as linux processes or linux threads or >>>> both) >>>> */ >>>> -int odph_odpthreads_create(odph_odpthread_t *thread_tbl, >>>> +int odph_odpthreads_create(odph_odpthread_tbl_t *thread_tbl_ptr, >>>> const odp_cpumask_t *mask, >>>> const odph_odpthread_params_t *thr_params) >>>> { >>>> @@ -382,11 +412,10 @@ int odph_odpthreads_create(odph_odpthread_t >>>> *thread_tbl, >>>> int num; >>>> int cpu_count; >>>> int cpu; >>>> + _odph_odpthread_t *thread_tbl; >>>> >>>> num = odp_cpumask_count(mask); >>>> >>>> - memset(thread_tbl, 0, num * sizeof(odph_odpthread_t)); >>>> - >>>> cpu_count = odp_cpu_count(); >>>> >>>> if (num < 1 || num > cpu_count) { >>>> @@ -396,6 +425,11 @@ int odph_odpthreads_create(odph_odpthread_t >>>> *thread_tbl, >>>> return -1; >>>> } >>>> >>>> + thread_tbl = malloc(num * sizeof(_odph_odpthread_t)); >>>> + *thread_tbl_ptr = (void *)thread_tbl; >>>> + >>>> + memset(thread_tbl, 0, num * sizeof(_odph_odpthread_t)); >>>> + >>>> >>> >>> Need a check for potential malloc failures. >>> >>> >>>> cpu = odp_cpumask_first(mask); >>>> for (i = 0; i < num; i++) { >>>> if (!helper_options.proc) { >>>> @@ -420,8 +454,9 @@ int odph_odpthreads_create(odph_odpthread_t >>>> *thread_tbl, >>>> /* >>>> * wait for the odpthreads termination (linux processes and threads) >>>> */ >>>> -int odph_odpthreads_join(odph_odpthread_t *thread_tbl) >>>> +int odph_odpthreads_join(odph_odpthread_tbl_t *thread_tbl_ptr) >>>> { >>>> + _odph_odpthread_t *thread_tbl; >>>> pid_t pid; >>>> int i = 0; >>>> int terminated = 0; >>>> @@ -430,6 +465,13 @@ int odph_odpthreads_join(odph_odpthread_t >>>> *thread_tbl) >>>> int ret; >>>> int retval = 0; >>>> >>>> + thread_tbl = (_odph_odpthread_t *)*thread_tbl_ptr; >>>> + >>>> + if (!thread_tbl) { >>>> + ODPH_ERR("Attempt to join thread from invalid table\n"); >>>> + return -1; >>>> + } >>>> + >>>> /* joins linux threads or wait for processes */ >>>> do { >>>> /* pthreads: */ >>>> @@ -489,7 +531,10 @@ int odph_odpthreads_join(odph_odpthread_t >>>> *thread_tbl) >>>> >>>> } while (!thread_tbl[i++].last); >>>> >>>> - return (retval < 0) ? retval : terminated; >>>> + /* free the allocated table: */ >>>> + free(*thread_tbl_ptr); >>>> + >>>> + return (retval < 0) ? retval : i; >>>> } >>>> >>>> /* >>>> diff --git a/helper/test/odpthreads.c b/helper/test/odpthreads.c >>>> index bba4fa5..489ecaa 100644 >>>> --- a/helper/test/odpthreads.c >>>> +++ b/helper/test/odpthreads.c >>>> @@ -31,7 +31,7 @@ int main(int argc, char *argv[]) >>>> { >>>> odp_instance_t instance; >>>> odph_odpthread_params_t thr_params; >>>> - odph_odpthread_t thread_tbl[NUMBER_WORKERS]; >>>> + odph_odpthread_tbl_t thread_tbl; >>>> odp_cpumask_t cpu_mask; >>>> int num_workers; >>>> int cpu; >>>> @@ -80,9 +80,9 @@ int main(int argc, char *argv[]) >>>> thr_params.thr_type = ODP_THREAD_WORKER; >>>> thr_params.instance = instance; >>>> >>>> - odph_odpthreads_create(&thread_tbl[0], &cpu_mask, &thr_params); >>>> + odph_odpthreads_create(&thread_tbl, &cpu_mask, &thr_params); >>>> >>>> - ret = odph_odpthreads_join(thread_tbl); >>>> + ret = odph_odpthreads_join(&thread_tbl); >>>> if (ret < 0) >>>> exit(EXIT_FAILURE); >>>> >>>> diff --git a/test/performance/odp_crypto.c >>>> b/test/performance/odp_crypto.c >>>> index 404984d..b4ce793 100644 >>>> --- a/test/performance/odp_crypto.c >>>> +++ b/test/performance/odp_crypto.c >>>> @@ -724,7 +724,7 @@ int main(int argc, char *argv[]) >>>> odp_cpumask_t cpumask; >>>> char cpumaskstr[ODP_CPUMASK_STR_SIZE]; >>>> int num_workers = 1; >>>> - odph_odpthread_t thr[num_workers]; >>>> + odph_odpthread_tbl_t thr; >>>> odp_instance_t instance; >>>> >>>> memset(&cargs, 0, sizeof(cargs)); >>>> @@ -794,8 +794,6 @@ int main(int argc, char *argv[]) >>>> printf("Run in sync mode\n"); >>>> } >>>> >>>> - memset(thr, 0, sizeof(thr)); >>>> - >>>> if (cargs.alg_config) { >>>> odph_odpthread_params_t thr_params; >>>> >>>> @@ -806,8 +804,8 @@ int main(int argc, char *argv[]) >>>> thr_params.instance = instance; >>>> >>>> if (cargs.schedule) { >>>> - odph_odpthreads_create(&thr[0], &cpumask, >>>> &thr_params); >>>> - odph_odpthreads_join(&thr[0]); >>>> + odph_odpthreads_create(&thr, &cpumask, >>>> &thr_params); >>>> + odph_odpthreads_join(&thr); >>>> } else { >>>> run_measure_one_config(&cargs, >>>> cargs.alg_config); >>>> } >>>> diff --git a/test/performance/odp_l2fwd.c b/test/performance/odp_l2fwd.c >>>> index e38e6f8..28874d1 100644 >>>> --- a/test/performance/odp_l2fwd.c >>>> +++ b/test/performance/odp_l2fwd.c >>>> @@ -1293,7 +1293,7 @@ static void gbl_args_init(args_t *args) >>>> */ >>>> int main(int argc, char *argv[]) >>>> { >>>> - odph_odpthread_t thread_tbl[MAX_WORKERS]; >>>> + odph_odpthread_tbl_t thread_tbl[MAX_WORKERS]; >>>> odp_pool_t pool; >>>> int i; >>>> int cpu; >>>> @@ -1424,8 +1424,6 @@ int main(int argc, char *argv[]) >>>> gbl_args->appl.in_mode == PLAIN_QUEUE) >>>> print_port_mapping(); >>>> >>>> - memset(thread_tbl, 0, sizeof(thread_tbl)); >>>> - >>>> stats = gbl_args->stats; >>>> >>>> odp_barrier_init(&barrier, num_workers + 1); >>>> diff --git a/test/performance/odp_pktio_perf.c >>>> b/test/performance/odp_pktio_perf.c >>>> index 98ec681..bda7d95 100644 >>>> --- a/test/performance/odp_pktio_perf.c >>>> +++ b/test/performance/odp_pktio_perf.c >>>> @@ -601,10 +601,11 @@ static int run_test_single(odp_cpumask_t >>>> *thd_mask_tx, >>>> odp_cpumask_t *thd_mask_rx, >>>> test_status_t *status) >>>> { >>>> - odph_odpthread_t thd_tbl[MAX_WORKERS]; >>>> + odph_odpthread_tbl_t rx_thd_tbl; >>>> + odph_odpthread_tbl_t tx_thd_tbl; >>>> thread_args_t args_tx, args_rx; >>>> uint64_t expected_tx_cnt; >>>> - int num_tx_workers, num_rx_workers; >>>> + int num_tx_workers; >>>> odph_odpthread_params_t thr_params; >>>> >>>> memset(&thr_params, 0, sizeof(thr_params)); >>>> @@ -613,7 +614,6 @@ static int run_test_single(odp_cpumask_t >>>> *thd_mask_tx, >>>> >>>> odp_atomic_store_u32(&shutdown, 0); >>>> >>>> - memset(thd_tbl, 0, sizeof(thd_tbl)); >>>> memset(gbl_args->rx_stats, 0, gbl_args->rx_stats_size); >>>> memset(gbl_args->tx_stats, 0, gbl_args->tx_stats_size); >>>> >>>> @@ -623,9 +623,8 @@ static int run_test_single(odp_cpumask_t >>>> *thd_mask_tx, >>>> thr_params.start = run_thread_rx; >>>> thr_params.arg = &args_rx; >>>> args_rx.batch_len = gbl_args->args.rx_batch_len; >>>> - odph_odpthreads_create(&thd_tbl[0], thd_mask_rx, &thr_params); >>>> + odph_odpthreads_create(&rx_thd_tbl, thd_mask_rx, &thr_params); >>>> odp_barrier_wait(&gbl_args->rx_barrier); >>>> - num_rx_workers = odp_cpumask_count(thd_mask_rx); >>>> >>>> /* then start transmitters */ >>>> thr_params.start = run_thread_tx; >>>> @@ -634,12 +633,12 @@ static int run_test_single(odp_cpumask_t >>>> *thd_mask_tx, >>>> args_tx.pps = status->pps_curr / num_tx_workers; >>>> args_tx.duration = gbl_args->args.duration; >>>> args_tx.batch_len = gbl_args->args.tx_batch_len; >>>> - odph_odpthreads_create(&thd_tbl[num_rx_workers], thd_mask_tx, >>>> + odph_odpthreads_create(&tx_thd_tbl, thd_mask_tx, >>>> &thr_params); >>>> odp_barrier_wait(&gbl_args->tx_barrier); >>>> >>>> /* wait for transmitter threads to terminate */ >>>> - odph_odpthreads_join(&thd_tbl[num_rx_workers]); >>>> + odph_odpthreads_join(&tx_thd_tbl); >>>> >>>> /* delay to allow transmitted packets to reach the receivers */ >>>> odp_time_wait_ns(SHUTDOWN_DELAY_NS); >>>> @@ -648,7 +647,7 @@ static int run_test_single(odp_cpumask_t >>>> *thd_mask_tx, >>>> odp_atomic_store_u32(&shutdown, 1); >>>> >>>> /* wait for receivers */ >>>> - odph_odpthreads_join(&thd_tbl[0]); >>>> + odph_odpthreads_join(&rx_thd_tbl); >>>> >>>> if (!status->warmup) >>>> return process_results(expected_tx_cnt, status); >>>> diff --git a/test/performance/odp_scheduling.c >>>> b/test/performance/odp_scheduling.c >>>> index 1d3bfd1..b1b2a86 100644 >>>> --- a/test/performance/odp_scheduling.c >>>> +++ b/test/performance/odp_scheduling.c >>>> @@ -775,7 +775,7 @@ static void parse_args(int argc, char *argv[], >>>> test_args_t *args) >>>> */ >>>> int main(int argc, char *argv[]) >>>> { >>>> - odph_odpthread_t thread_tbl[MAX_WORKERS]; >>>> + odph_odpthread_tbl_t thread_tbl; >>>> test_args_t args; >>>> int num_workers; >>>> odp_cpumask_t cpumask; >>>> @@ -795,8 +795,6 @@ int main(int argc, char *argv[]) >>>> memset(&args, 0, sizeof(args)); >>>> parse_args(argc, argv, &args); >>>> >>>> - memset(thread_tbl, 0, sizeof(thread_tbl)); >>>> - >>>> /* ODP global init */ >>>> if (odp_init_global(&instance, NULL, NULL)) { >>>> LOG_ERR("ODP global init failed.\n"); >>>> @@ -943,10 +941,10 @@ int main(int argc, char *argv[]) >>>> thr_params.instance = instance; >>>> thr_params.start = run_thread; >>>> thr_params.arg = NULL; >>>> - odph_odpthreads_create(thread_tbl, &cpumask, &thr_params); >>>> + odph_odpthreads_create(&thread_tbl, &cpumask, &thr_params); >>>> >>>> /* Wait for worker threads to terminate */ >>>> - odph_odpthreads_join(thread_tbl); >>>> + odph_odpthreads_join(&thread_tbl); >>>> >>>> printf("ODP example complete\n\n"); >>>> >>>> diff --git a/test/validation/common/odp_cunit_common.c >>>> b/test/validation/common/odp_cunit_common.c >>>> index 7df9aa6..cef0f4d 100644 >>>> --- a/test/validation/common/odp_cunit_common.c >>>> +++ b/test/validation/common/odp_cunit_common.c >>>> @@ -9,7 +9,7 @@ >>>> #include <odp_cunit_common.h> >>>> #include <odp/helper/linux.h> >>>> /* Globals */ >>>> -static odph_odpthread_t thread_tbl[MAX_WORKERS]; >>>> +static odph_odpthread_tbl_t thread_tbl; >>>> static odp_instance_t instance; >>>> >>>> /* >>>> @@ -40,14 +40,14 @@ int odp_cunit_thread_create(int func_ptr(void *), >>>> pthrd_arg *arg) >>>> /* Create and init additional threads */ >>>> odp_cpumask_default_worker(&cpumask, arg->numthrds); >>>> >>>> - return odph_odpthreads_create(thread_tbl, &cpumask, >>>> &thr_params); >>>> + return odph_odpthreads_create(&thread_tbl, &cpumask, >>>> &thr_params); >>>> } >>>> >>>> /** exit from test thread */ >>>> int odp_cunit_thread_exit(pthrd_arg *arg) >>>> { >>>> /* Wait for other threads to exit */ >>>> - if (odph_odpthreads_join(thread_tbl) != arg->numthrds) { >>>> + if (odph_odpthreads_join(&thread_tbl) != arg->numthrds) { >>>> fprintf(stderr, >>>> "error: odph_odpthreads_join() failed.\n"); >>>> return -1; >>>> -- >>>> 2.5.0 >>>> >>>> >>> Tell me if my answer make sense and if you have other comments, I will >> put the _odph_odpthread_t to _odph_odpthread_impl_t name change for v2 >> >> Christophe. >> > >
diff --git a/example/classifier/odp_classifier.c b/example/classifier/odp_classifier.c index 20e64ec..7512ec6 100644 --- a/example/classifier/odp_classifier.c +++ b/example/classifier/odp_classifier.c @@ -469,7 +469,7 @@ static void configure_cos(odp_cos_t default_cos, appl_args_t *args) */ int main(int argc, char *argv[]) { - odph_odpthread_t thread_tbl[MAX_WORKERS]; + odph_odpthread_tbl_t thread_tbl; odp_pool_t pool; int num_workers; int i; @@ -562,21 +562,18 @@ int main(int argc, char *argv[]) exit(EXIT_FAILURE); } - /* Create and init worker threads */ - memset(thread_tbl, 0, sizeof(thread_tbl)); - memset(&thr_params, 0, sizeof(thr_params)); thr_params.start = pktio_receive_thread; thr_params.arg = args; thr_params.thr_type = ODP_THREAD_WORKER; thr_params.instance = instance; - odph_odpthreads_create(thread_tbl, &cpumask, &thr_params); + odph_odpthreads_create(&thread_tbl, &cpumask, &thr_params); print_cls_statistics(args); odp_pktio_stop(pktio); shutdown = 1; - odph_odpthreads_join(thread_tbl); + odph_odpthreads_join(&thread_tbl); for (i = 0; i < args->policy_count; i++) { if ((i != args->policy_count - 1) && diff --git a/example/generator/odp_generator.c b/example/generator/odp_generator.c index ccae907..e29b008 100644 --- a/example/generator/odp_generator.c +++ b/example/generator/odp_generator.c @@ -621,7 +621,7 @@ static void print_global_stats(int num_workers) */ int main(int argc, char *argv[]) { - odph_odpthread_t thread_tbl[MAX_WORKERS]; + odph_odpthread_tbl_t thread_tbl[MAX_WORKERS]; odp_pool_t pool; int num_workers; int i; @@ -746,9 +746,6 @@ int main(int argc, char *argv[]) for (i = 0; i < args->appl.if_count; ++i) pktio[i] = create_pktio(args->appl.if_names[i], pool); - /* Create and init worker threads */ - memset(thread_tbl, 0, sizeof(thread_tbl)); - /* Init threads params */ memset(&thr_params, 0, sizeof(thr_params)); thr_params.thr_type = ODP_THREAD_WORKER; diff --git a/example/ipsec/odp_ipsec.c b/example/ipsec/odp_ipsec.c index fb4385f..95b907f 100644 --- a/example/ipsec/odp_ipsec.c +++ b/example/ipsec/odp_ipsec.c @@ -1212,7 +1212,7 @@ int pktio_thread(void *arg EXAMPLE_UNUSED) int main(int argc, char *argv[]) { - odph_odpthread_t thread_tbl[MAX_WORKERS]; + odph_odpthread_tbl_t thread_tbl; int num_workers; int i; int stream_count; @@ -1341,7 +1341,7 @@ main(int argc, char *argv[]) thr_params.arg = NULL; thr_params.thr_type = ODP_THREAD_WORKER; thr_params.instance = instance; - odph_odpthreads_create(thread_tbl, &cpumask, &thr_params); + odph_odpthreads_create(&thread_tbl, &cpumask, &thr_params); /* * If there are streams attempt to verify them else @@ -1355,7 +1355,7 @@ main(int argc, char *argv[]) } while (!done); printf("All received\n"); } else { - odph_odpthreads_join(thread_tbl); + odph_odpthreads_join(&thread_tbl); } free(args->appl.if_names); diff --git a/example/l2fwd_simple/odp_l2fwd_simple.c b/example/l2fwd_simple/odp_l2fwd_simple.c index daae038..8bd51e1 100644 --- a/example/l2fwd_simple/odp_l2fwd_simple.c +++ b/example/l2fwd_simple/odp_l2fwd_simple.c @@ -119,7 +119,7 @@ int main(int argc, char **argv) odp_pool_t pool; odp_pool_param_t params; odp_cpumask_t cpumask; - odph_odpthread_t thd; + odph_odpthread_tbl_t thd; odp_instance_t instance; odph_odpthread_params_t thr_params; int opt; diff --git a/example/packet/odp_pktio.c b/example/packet/odp_pktio.c index 9028ab2..cab9da2 100644 --- a/example/packet/odp_pktio.c +++ b/example/packet/odp_pktio.c @@ -342,7 +342,7 @@ static int pktio_ifburst_thread(void *arg) */ int main(int argc, char *argv[]) { - odph_odpthread_t thread_tbl[MAX_WORKERS]; + odph_odpthread_tbl_t thread_tbl[MAX_WORKERS]; odp_pool_t pool; int num_workers; int i; @@ -409,9 +409,6 @@ int main(int argc, char *argv[]) for (i = 0; i < args->appl.if_count; ++i) create_pktio(args->appl.if_names[i], pool, args->appl.mode); - /* Create and init worker threads */ - memset(thread_tbl, 0, sizeof(thread_tbl)); - memset(&thr_params, 0, sizeof(thr_params)); thr_params.thr_type = ODP_THREAD_WORKER; thr_params.instance = instance; diff --git a/example/switch/odp_switch.c b/example/switch/odp_switch.c index 0c9b257..5cfc742 100644 --- a/example/switch/odp_switch.c +++ b/example/switch/odp_switch.c @@ -884,7 +884,7 @@ static void gbl_args_init(args_t *args) int main(int argc, char **argv) { - odph_odpthread_t thread_tbl[MAX_WORKERS]; + odph_odpthread_tbl_t thread_tbl[MAX_WORKERS]; int i, j; int cpu; int num_workers; @@ -986,8 +986,6 @@ int main(int argc, char **argv) print_port_mapping(); - memset(thread_tbl, 0, sizeof(thread_tbl)); - odp_barrier_init(&barrier, num_workers + 1); stats = gbl_args->stats; diff --git a/example/time/time_global_test.c b/example/time/time_global_test.c index 372d96b..8f80149 100644 --- a/example/time/time_global_test.c +++ b/example/time/time_global_test.c @@ -252,7 +252,7 @@ int main(int argc, char *argv[]) odp_shm_t shm_glbls = ODP_SHM_INVALID; odp_shm_t shm_log = ODP_SHM_INVALID; int log_size, log_enries_num; - odph_odpthread_t thread_tbl[MAX_WORKERS]; + odph_odpthread_tbl_t thread_tbl; odp_instance_t instance; odph_odpthread_params_t thr_params; @@ -326,10 +326,10 @@ int main(int argc, char *argv[]) thr_params.instance = instance; /* Create and launch worker threads */ - odph_odpthreads_create(thread_tbl, &cpumask, &thr_params); + odph_odpthreads_create(&thread_tbl, &cpumask, &thr_params); /* Wait for worker threads to exit */ - odph_odpthreads_join(thread_tbl); + odph_odpthreads_join(&thread_tbl); print_log(gbls); diff --git a/example/timer/odp_timer_test.c b/example/timer/odp_timer_test.c index cb58dfe..c594a9f 100644 --- a/example/timer/odp_timer_test.c +++ b/example/timer/odp_timer_test.c @@ -330,7 +330,7 @@ static void parse_args(int argc, char *argv[], test_args_t *args) */ int main(int argc, char *argv[]) { - odph_odpthread_t thread_tbl[MAX_WORKERS]; + odph_odpthread_tbl_t thread_tbl; int num_workers; odp_queue_t queue; uint64_t tick, ns; @@ -393,8 +393,6 @@ int main(int argc, char *argv[]) parse_args(argc, argv, &gbls->args); - memset(thread_tbl, 0, sizeof(thread_tbl)); - /* Default to system CPU count unless user specified */ num_workers = MAX_WORKERS; if (gbls->args.cpu_count) @@ -505,10 +503,10 @@ int main(int argc, char *argv[]) thr_params.thr_type = ODP_THREAD_WORKER; thr_params.instance = instance; - odph_odpthreads_create(thread_tbl, &cpumask, &thr_params); + odph_odpthreads_create(&thread_tbl, &cpumask, &thr_params); /* Wait for worker threads to exit */ - odph_odpthreads_join(thread_tbl); + odph_odpthreads_join(&thread_tbl); /* free resources */ if (odp_queue_destroy(queue)) diff --git a/helper/include/odp/helper/linux.h b/helper/include/odp/helper/linux.h index 2e89833..2085768 100644 --- a/helper/include/odp/helper/linux.h +++ b/helper/include/odp/helper/linux.h @@ -56,13 +56,6 @@ typedef struct { int status; /**< Process state change status */ } odph_linux_process_t; -/** odpthread linux type: whether an ODP thread is a linux thread or process */ -typedef enum odph_odpthread_linuxtype_e { - ODPTHREAD_NOT_STARTED = 0, - ODPTHREAD_PROCESS, - ODPTHREAD_PTHREAD -} odph_odpthread_linuxtype_t; - /** odpthread parameters for odp threads (pthreads and processes) */ typedef struct { int (*start)(void *); /**< Thread entry point function */ @@ -71,28 +64,8 @@ typedef struct { odp_instance_t instance; /**< ODP instance handle */ } odph_odpthread_params_t; -/** The odpthread starting arguments, used both in process or thread mode */ -typedef struct { - odph_odpthread_linuxtype_t linuxtype; /**< process or pthread */ - odph_odpthread_params_t thr_params; /**< odpthread start parameters */ -} odph_odpthread_start_args_t; - -/** Linux odpthread state information, used both in process or thread mode */ -typedef struct { - odph_odpthread_start_args_t start_args; /**< start arguments */ - int cpu; /**< CPU ID */ - int last; /**< true if last table entry */ - union { - struct { /* for thread implementation */ - pthread_t thread_id; /**< Pthread ID */ - pthread_attr_t attr; /**< Pthread attributes */ - } thread; - struct { /* for process implementation */ - pid_t pid; /**< Process ID */ - int status; /**< Process state chge status*/ - } proc; - }; -} odph_odpthread_t; +/** abstract type for odpthread arrays:*/ +typedef void *odph_odpthread_tbl_t; /** * Creates and launches pthreads @@ -182,7 +155,7 @@ int odph_linux_process_wait_n(odph_linux_process_t *proc_tbl, int num); * * @return Number of threads created */ -int odph_odpthreads_create(odph_odpthread_t *thread_tbl, +int odph_odpthreads_create(odph_odpthread_tbl_t *thread_tbl_ptr, const odp_cpumask_t *mask, const odph_odpthread_params_t *thr_params); @@ -197,7 +170,7 @@ int odph_odpthreads_create(odph_odpthread_t *thread_tbl, * the thread join/process wait itself failed -e.g. as the result of a kill) * */ -int odph_odpthreads_join(odph_odpthread_t *thread_tbl); +int odph_odpthreads_join(odph_odpthread_tbl_t *thread_tbl_ptr); /** * Merge getopt options diff --git a/helper/linux.c b/helper/linux.c index 6366694..57cd8a7 100644 --- a/helper/linux.c +++ b/helper/linux.c @@ -21,6 +21,36 @@ #include <odp/helper/linux.h> #include "odph_debug.h" +/** odpthread linux type: whether an ODP thread is a linux thread or process */ +typedef enum _odph_odpthread_linuxtype_e { + ODPTHREAD_NOT_STARTED = 0, + ODPTHREAD_PROCESS, + ODPTHREAD_PTHREAD +} _odph_odpthread_linuxtype_t; + +/** The odpthread starting arguments, used both in process or thread mode */ +typedef struct { + _odph_odpthread_linuxtype_t linuxtype; + odph_odpthread_params_t thr_params; /*copy of thread start parameter*/ +} _odph_odpthread_start_args_t; + +/** Linux odpthread state information, used both in process or thread mode */ +typedef struct { + _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_id; /**< Pthread ID */ + pthread_attr_t attr; /**< Pthread attributes */ + } thread; + struct { /* for process implementation */ + pid_t pid; /**< Process ID */ + int status; /**< Process state chge status*/ + } proc; + }; +} _odph_odpthread_t; + static struct { int proc; /* true when process mode is required, false otherwise */ } helper_options; @@ -251,7 +281,7 @@ static void *odpthread_run_start_routine(void *arg) int ret; odph_odpthread_params_t *thr_params; - odph_odpthread_start_args_t *start_args = arg; + _odph_odpthread_start_args_t *start_args = arg; thr_params = &start_args->thr_params; @@ -289,7 +319,7 @@ static void *odpthread_run_start_routine(void *arg) /* * Create a single ODPthread as a linux process */ -static int odph_linux_process_create(odph_odpthread_t *thread_tbl, +static int odph_linux_process_create(_odph_odpthread_t *thread_tbl, int cpu, const odph_odpthread_params_t *thr_params) { @@ -338,7 +368,7 @@ static int odph_linux_process_create(odph_odpthread_t *thread_tbl, /* * Create a single ODPthread as a linux thread */ -static int odph_linux_thread_create(odph_odpthread_t *thread_tbl, +static int odph_linux_thread_create(_odph_odpthread_t *thread_tbl, int cpu, const odph_odpthread_params_t *thr_params) { @@ -374,7 +404,7 @@ static int odph_linux_thread_create(odph_odpthread_t *thread_tbl, /* * create an odpthread set (as linux processes or linux threads or both) */ -int odph_odpthreads_create(odph_odpthread_t *thread_tbl, +int odph_odpthreads_create(odph_odpthread_tbl_t *thread_tbl_ptr, const odp_cpumask_t *mask, const odph_odpthread_params_t *thr_params) { @@ -382,11 +412,10 @@ int odph_odpthreads_create(odph_odpthread_t *thread_tbl, int num; int cpu_count; int cpu; + _odph_odpthread_t *thread_tbl; num = odp_cpumask_count(mask); - memset(thread_tbl, 0, num * sizeof(odph_odpthread_t)); - cpu_count = odp_cpu_count(); if (num < 1 || num > cpu_count) { @@ -396,6 +425,11 @@ int odph_odpthreads_create(odph_odpthread_t *thread_tbl, return -1; } + thread_tbl = malloc(num * sizeof(_odph_odpthread_t)); + *thread_tbl_ptr = (void *)thread_tbl; + + memset(thread_tbl, 0, num * sizeof(_odph_odpthread_t)); + cpu = odp_cpumask_first(mask); for (i = 0; i < num; i++) { if (!helper_options.proc) { @@ -420,8 +454,9 @@ int odph_odpthreads_create(odph_odpthread_t *thread_tbl, /* * wait for the odpthreads termination (linux processes and threads) */ -int odph_odpthreads_join(odph_odpthread_t *thread_tbl) +int odph_odpthreads_join(odph_odpthread_tbl_t *thread_tbl_ptr) { + _odph_odpthread_t *thread_tbl; pid_t pid; int i = 0; int terminated = 0; @@ -430,6 +465,13 @@ int odph_odpthreads_join(odph_odpthread_t *thread_tbl) int ret; int retval = 0; + thread_tbl = (_odph_odpthread_t *)*thread_tbl_ptr; + + if (!thread_tbl) { + ODPH_ERR("Attempt to join thread from invalid table\n"); + return -1; + } + /* joins linux threads or wait for processes */ do { /* pthreads: */ @@ -489,7 +531,10 @@ int odph_odpthreads_join(odph_odpthread_t *thread_tbl) } while (!thread_tbl[i++].last); - return (retval < 0) ? retval : terminated; + /* free the allocated table: */ + free(*thread_tbl_ptr); + + return (retval < 0) ? retval : i; } /* diff --git a/helper/test/odpthreads.c b/helper/test/odpthreads.c index bba4fa5..489ecaa 100644 --- a/helper/test/odpthreads.c +++ b/helper/test/odpthreads.c @@ -31,7 +31,7 @@ int main(int argc, char *argv[]) { odp_instance_t instance; odph_odpthread_params_t thr_params; - odph_odpthread_t thread_tbl[NUMBER_WORKERS]; + odph_odpthread_tbl_t thread_tbl; odp_cpumask_t cpu_mask; int num_workers; int cpu; @@ -80,9 +80,9 @@ int main(int argc, char *argv[]) thr_params.thr_type = ODP_THREAD_WORKER; thr_params.instance = instance; - odph_odpthreads_create(&thread_tbl[0], &cpu_mask, &thr_params); + odph_odpthreads_create(&thread_tbl, &cpu_mask, &thr_params); - ret = odph_odpthreads_join(thread_tbl); + ret = odph_odpthreads_join(&thread_tbl); if (ret < 0) exit(EXIT_FAILURE); diff --git a/test/performance/odp_crypto.c b/test/performance/odp_crypto.c index 404984d..b4ce793 100644 --- a/test/performance/odp_crypto.c +++ b/test/performance/odp_crypto.c @@ -724,7 +724,7 @@ int main(int argc, char *argv[]) odp_cpumask_t cpumask; char cpumaskstr[ODP_CPUMASK_STR_SIZE]; int num_workers = 1; - odph_odpthread_t thr[num_workers]; + odph_odpthread_tbl_t thr; odp_instance_t instance; memset(&cargs, 0, sizeof(cargs)); @@ -794,8 +794,6 @@ int main(int argc, char *argv[]) printf("Run in sync mode\n"); } - memset(thr, 0, sizeof(thr)); - if (cargs.alg_config) { odph_odpthread_params_t thr_params; @@ -806,8 +804,8 @@ int main(int argc, char *argv[]) thr_params.instance = instance; if (cargs.schedule) { - odph_odpthreads_create(&thr[0], &cpumask, &thr_params); - odph_odpthreads_join(&thr[0]); + odph_odpthreads_create(&thr, &cpumask, &thr_params); + odph_odpthreads_join(&thr); } else { run_measure_one_config(&cargs, cargs.alg_config); } diff --git a/test/performance/odp_l2fwd.c b/test/performance/odp_l2fwd.c index e38e6f8..28874d1 100644 --- a/test/performance/odp_l2fwd.c +++ b/test/performance/odp_l2fwd.c @@ -1293,7 +1293,7 @@ static void gbl_args_init(args_t *args) */ int main(int argc, char *argv[]) { - odph_odpthread_t thread_tbl[MAX_WORKERS]; + odph_odpthread_tbl_t thread_tbl[MAX_WORKERS]; odp_pool_t pool; int i; int cpu; @@ -1424,8 +1424,6 @@ int main(int argc, char *argv[]) gbl_args->appl.in_mode == PLAIN_QUEUE) print_port_mapping(); - memset(thread_tbl, 0, sizeof(thread_tbl)); - stats = gbl_args->stats; odp_barrier_init(&barrier, num_workers + 1); diff --git a/test/performance/odp_pktio_perf.c b/test/performance/odp_pktio_perf.c index 98ec681..bda7d95 100644 --- a/test/performance/odp_pktio_perf.c +++ b/test/performance/odp_pktio_perf.c @@ -601,10 +601,11 @@ static int run_test_single(odp_cpumask_t *thd_mask_tx, odp_cpumask_t *thd_mask_rx, test_status_t *status) { - odph_odpthread_t thd_tbl[MAX_WORKERS]; + odph_odpthread_tbl_t rx_thd_tbl; + odph_odpthread_tbl_t tx_thd_tbl; thread_args_t args_tx, args_rx; uint64_t expected_tx_cnt; - int num_tx_workers, num_rx_workers; + int num_tx_workers; odph_odpthread_params_t thr_params; memset(&thr_params, 0, sizeof(thr_params)); @@ -613,7 +614,6 @@ static int run_test_single(odp_cpumask_t *thd_mask_tx, odp_atomic_store_u32(&shutdown, 0); - memset(thd_tbl, 0, sizeof(thd_tbl)); memset(gbl_args->rx_stats, 0, gbl_args->rx_stats_size); memset(gbl_args->tx_stats, 0, gbl_args->tx_stats_size); @@ -623,9 +623,8 @@ static int run_test_single(odp_cpumask_t *thd_mask_tx, thr_params.start = run_thread_rx; thr_params.arg = &args_rx; args_rx.batch_len = gbl_args->args.rx_batch_len; - odph_odpthreads_create(&thd_tbl[0], thd_mask_rx, &thr_params); + odph_odpthreads_create(&rx_thd_tbl, thd_mask_rx, &thr_params); odp_barrier_wait(&gbl_args->rx_barrier); - num_rx_workers = odp_cpumask_count(thd_mask_rx); /* then start transmitters */ thr_params.start = run_thread_tx; @@ -634,12 +633,12 @@ static int run_test_single(odp_cpumask_t *thd_mask_tx, args_tx.pps = status->pps_curr / num_tx_workers; args_tx.duration = gbl_args->args.duration; args_tx.batch_len = gbl_args->args.tx_batch_len; - odph_odpthreads_create(&thd_tbl[num_rx_workers], thd_mask_tx, + odph_odpthreads_create(&tx_thd_tbl, thd_mask_tx, &thr_params); odp_barrier_wait(&gbl_args->tx_barrier); /* wait for transmitter threads to terminate */ - odph_odpthreads_join(&thd_tbl[num_rx_workers]); + odph_odpthreads_join(&tx_thd_tbl); /* delay to allow transmitted packets to reach the receivers */ odp_time_wait_ns(SHUTDOWN_DELAY_NS); @@ -648,7 +647,7 @@ static int run_test_single(odp_cpumask_t *thd_mask_tx, odp_atomic_store_u32(&shutdown, 1); /* wait for receivers */ - odph_odpthreads_join(&thd_tbl[0]); + odph_odpthreads_join(&rx_thd_tbl); if (!status->warmup) return process_results(expected_tx_cnt, status); diff --git a/test/performance/odp_scheduling.c b/test/performance/odp_scheduling.c index 1d3bfd1..b1b2a86 100644 --- a/test/performance/odp_scheduling.c +++ b/test/performance/odp_scheduling.c @@ -775,7 +775,7 @@ static void parse_args(int argc, char *argv[], test_args_t *args) */ int main(int argc, char *argv[]) { - odph_odpthread_t thread_tbl[MAX_WORKERS]; + odph_odpthread_tbl_t thread_tbl; test_args_t args; int num_workers; odp_cpumask_t cpumask; @@ -795,8 +795,6 @@ int main(int argc, char *argv[]) memset(&args, 0, sizeof(args)); parse_args(argc, argv, &args); - memset(thread_tbl, 0, sizeof(thread_tbl)); - /* ODP global init */ if (odp_init_global(&instance, NULL, NULL)) { LOG_ERR("ODP global init failed.\n"); @@ -943,10 +941,10 @@ int main(int argc, char *argv[]) thr_params.instance = instance; thr_params.start = run_thread; thr_params.arg = NULL; - odph_odpthreads_create(thread_tbl, &cpumask, &thr_params); + odph_odpthreads_create(&thread_tbl, &cpumask, &thr_params); /* Wait for worker threads to terminate */ - odph_odpthreads_join(thread_tbl); + odph_odpthreads_join(&thread_tbl); printf("ODP example complete\n\n"); diff --git a/test/validation/common/odp_cunit_common.c b/test/validation/common/odp_cunit_common.c index 7df9aa6..cef0f4d 100644 --- a/test/validation/common/odp_cunit_common.c +++ b/test/validation/common/odp_cunit_common.c @@ -9,7 +9,7 @@ #include <odp_cunit_common.h> #include <odp/helper/linux.h> /* Globals */ -static odph_odpthread_t thread_tbl[MAX_WORKERS]; +static odph_odpthread_tbl_t thread_tbl; static odp_instance_t instance; /* @@ -40,14 +40,14 @@ int odp_cunit_thread_create(int func_ptr(void *), pthrd_arg *arg) /* Create and init additional threads */ odp_cpumask_default_worker(&cpumask, arg->numthrds); - return odph_odpthreads_create(thread_tbl, &cpumask, &thr_params); + return odph_odpthreads_create(&thread_tbl, &cpumask, &thr_params); } /** exit from test thread */ int odp_cunit_thread_exit(pthrd_arg *arg) { /* Wait for other threads to exit */ - if (odph_odpthreads_join(thread_tbl) != arg->numthrds) { + if (odph_odpthreads_join(&thread_tbl) != arg->numthrds) { fprintf(stderr, "error: odph_odpthreads_join() failed.\n"); return -1;
The odph_odpthread_t (non opaque array of odpthread) is now replaced by a single type odph_odpthread_tbl_t abstracted as a void*. The table describing the odpthreads being created is now malloc'd and freed by the helper function themselves. Signed-off-by: Christophe Milard <christophe.milard@linaro.org> --- example/classifier/odp_classifier.c | 9 ++--- example/generator/odp_generator.c | 5 +-- example/ipsec/odp_ipsec.c | 6 +-- example/l2fwd_simple/odp_l2fwd_simple.c | 2 +- example/packet/odp_pktio.c | 5 +-- example/switch/odp_switch.c | 4 +- example/time/time_global_test.c | 6 +-- example/timer/odp_timer_test.c | 8 ++-- helper/include/odp/helper/linux.h | 35 ++---------------- helper/linux.c | 61 +++++++++++++++++++++++++++---- helper/test/odpthreads.c | 6 +-- test/performance/odp_crypto.c | 8 ++-- test/performance/odp_l2fwd.c | 4 +- test/performance/odp_pktio_perf.c | 15 ++++---- test/performance/odp_scheduling.c | 8 ++-- test/validation/common/odp_cunit_common.c | 6 +-- 16 files changed, 93 insertions(+), 95 deletions(-)