| Message ID | 1464334744-32746-1-git-send-email-christophe.milard@linaro.org |
|---|---|
| State | New |
| Headers | show |
Reviewed-and-tested-by: Yi He <yi.he@linaro.org> Tested with command on an *ubuntu xenial xerus 16.04 SMP (Intel(R) Core(TM) i7-4790 CPU @ 3.60GHz) *system. *GIT_URL=file:///~/Development/odp-linux/.git GIT_BRANCH=review HELPER_TEST=1 PERF_TEST=1 EXAMPLE_TEST=1 LCOV=1 ./build.sh* Best Regards, Yi On 27 May 2016 at 15:39, 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> > --- > since V2: > - malloc return value check in odph_odpthreads_create() > > since V1: > -_odph_odpthread_t changed to _odph_odpthread_impl_t (Hi) > - full init of the odph_odpthread_tbl_t by odph_odpthreads_create() > even in case of error. (Hi) > > 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 | 67 > +++++++++++++++++++++++++++---- > 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, 99 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]; > 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..405ae23 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_impl_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_impl_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_impl_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,20 +412,30 @@ int odph_odpthreads_create(odph_odpthread_t > *thread_tbl, > int num; > int cpu_count; > int cpu; > + _odph_odpthread_impl_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) { > ODPH_ERR("Invalid number of odpthreads:%d" > " (%d cores available)\n", > num, cpu_count); > + *thread_tbl_ptr = NULL; > + return -1; > + } > + > + thread_tbl = malloc(num * sizeof(_odph_odpthread_impl_t)); > + *thread_tbl_ptr = (void *)thread_tbl; > + > + if (!thread_tbl) { > + ODPH_ERR("malloc failed!"); > return -1; > } > > + memset(thread_tbl, 0, num * sizeof(_odph_odpthread_impl_t)); > + > cpu = odp_cpumask_first(mask); > for (i = 0; i < num; i++) { > if (!helper_options.proc) { > @@ -420,8 +460,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_impl_t *thread_tbl; > pid_t pid; > int i = 0; > int terminated = 0; > @@ -430,6 +471,13 @@ int odph_odpthreads_join(odph_odpthread_t *thread_tbl) > int ret; > int retval = 0; > > + thread_tbl = (_odph_odpthread_impl_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 +537,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 27 May 2016 at 09:56, Savolainen, Petri (Nokia - FI/Espoo) < petri.savolainen@nokia.com> wrote: > > > > -----Original Message----- > > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of > > Christophe Milard > > Sent: Friday, May 27, 2016 10:39 AM > > To: yi.he@linaro.org; lng-odp@lists.linaro.org > > Subject: [lng-odp] [PATCHv3] helper: cleaner interface to > > odph_odpthreads_create/join > > > > 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. > > > > > This change assumes that application will create all threads with a single > call (which is not always practical or even possible). If application > creates threads one by one, you'd end up with N separate thread table > pointers, which application would need to store into another table anyway. > Correct: My approach let tha application decides how it wants to group the threads: If the threads share the same functions to run etc, the application can handle it in a single call (1 table of N threads). If the threads cannot be stardes at the same time, tha application my take the N table if 1 entry instead. > > It would be better to either define opaque odph_odpthread_t with correct > size, or define alloc/free calls for a odph_odpthread_tbl_t. > > odph_odpthread_tbl_t *thr_tbl; > int num = 5; > odp_cpumask_t cpumask; > > thr_tbl = odph_odpthread_tbl_alloc(num); > > odp_cpumask_zero(&cpumask); > odp_cpumask_set(&cpumask, 1); > odph_odpthreads_create(thr_tbl, &cpumask, ...); > > ... > > odp_cpumask_zero(&cpumask); > odp_cpumask_set(&cpumask, 2); > odph_odpthreads_create(thr_tbl, &cpumask, ...); > > ... > > odp_cpumask_zero(&cpumask); > odp_cpumask_set(&cpumask, 3); > odp_cpumask_set(&cpumask, 4); > odp_cpumask_set(&cpumask, 5); > odph_odpthreads_create(thr_tbl, &cpumask, ...); > > hmmm... what would happen at join time? would the application have to provide a mask with the set of CPU threads to be join hence letting the responsibility of the join consistency to the application? or would all the threads in the table be joined (which is maybe not what the application wants)?... What happens the day we say more than one thread can run on each cpu? (the current approach can easily be modified by passing an array of N cpu number -possibly including the same cpu many times-, instead of the current mask at creation time.). How would you describe which threads should be started or joined in your case? I guess an array of odp_thread ids would have to be given as well... or? I think the appraoch taken here is cleaner: each table is a group of threads which are created and joined at the same time. The helpers guarantees this consistency over such a group: the table size, the starting of all group threads and the joining of all group threads is done by the helper. Yes, the application may have the need to create many such tables if it needs different groups of threads, but the consistency within each group is garanteed by the helper. Besides, the need to have different tables for different group of threads does not necessarily make the application itself less clear: the pktio performance test, for instance, now defines two sets of threads: the TX threads and the RX threads. Is that worse than having a single table of threads and letting the application try to distinguish them by some other means (the cpu mask in your approach, as long as this works...) Your approach removes the need to have many tables within the application, indeed, but the consistency (size of table, number of started threads and number of joined threads) has to be maintained by the caller. My approach lets the helper maintain the consistency within a table and force the application to separate different thread categories in different tables... I still think my approach is more consistent. Hope I can convince you :-) Have a nice week-end, Christophe. > -Petri > > > >
On 30 May 2016 at 11:23, Savolainen, Petri (Nokia - FI/Espoo) < petri.savolainen@nokia.com> wrote: > > > From: Christophe Milard [mailto:christophe.milard@linaro.org] > Sent: Friday, May 27, 2016 4:08 PM > To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia.com> > Cc: yi.he@linaro.org; lng-odp@lists.linaro.org > Subject: Re: [lng-odp] [PATCHv3] helper: cleaner interface to > odph_odpthreads_create/join > > > > On 27 May 2016 at 09:56, Savolainen, Petri (Nokia - FI/Espoo) < > petri.savolainen@nokia.com> wrote: > > > > -----Original Message----- > > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of > > Christophe Milard > > Sent: Friday, May 27, 2016 10:39 AM > > To: yi.he@linaro.org; lng-odp@lists.linaro.org > > Subject: [lng-odp] [PATCHv3] helper: cleaner interface to > > odph_odpthreads_create/join > > > > 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. > > > > > This change assumes that application will create all threads with a single > call (which is not always practical or even possible). If application > creates threads one by one, you'd end up with N separate thread table > pointers, which application would need to store into another table anyway. > > Correct: My approach let tha application decides how it wants to group the > threads: If the threads share the same functions to run etc, the > application can handle it in a single call (1 table of N threads). If the > threads cannot be stardes at the same time, tha application my take the N > table if 1 entry instead. > > > It would be better to either define opaque odph_odpthread_t with correct > size, or define alloc/free calls for a odph_odpthread_tbl_t. > > odph_odpthread_tbl_t *thr_tbl; > int num = 5; > odp_cpumask_t cpumask; > > thr_tbl = odph_odpthread_tbl_alloc(num); > > odp_cpumask_zero(&cpumask); > odp_cpumask_set(&cpumask, 1); > odph_odpthreads_create(thr_tbl, &cpumask, ...); > > ... > > odp_cpumask_zero(&cpumask); > odp_cpumask_set(&cpumask, 2); > odph_odpthreads_create(thr_tbl, &cpumask, ...); > > ... > > odp_cpumask_zero(&cpumask); > odp_cpumask_set(&cpumask, 3); > odp_cpumask_set(&cpumask, 4); > odp_cpumask_set(&cpumask, 5); > odph_odpthreads_create(thr_tbl, &cpumask, ...); > > hmmm... what would happen at join time? would the application have to > provide a mask with the set of CPU threads to be join hence letting the > responsibility of the join consistency to the application? or would all the > threads in the table be joined (which is maybe not what the application > wants)?... > > What happens the day we say more than one thread can run on each cpu? > (the current approach can easily be modified by passing an array of N cpu > number -possibly including the same cpu many times-, instead of the current > mask at creation time.). How would you describe which threads should be > started or joined in your case? I guess an array of odp_thread ids would > have to be given as well... or? > > I think the appraoch taken here is cleaner: each table is a group of > threads which are created and joined at the same time. The helpers > guarantees this consistency over such a group: the table size, the starting > of all group threads and the joining of all group threads is done by the > helper. > Yes, the application may have the need to create many such tables if it > needs different groups of threads, but the consistency within each group is > garanteed by the helper. > Besides, the need to have different tables for different group of threads > does not necessarily make the application itself less clear: the pktio > performance test, for instance, now defines two sets of threads: the TX > threads and the RX threads. Is that worse than having a single table of > threads and letting the application try to distinguish them by some other > means (the cpu mask in your approach, as long as this works...) > > Your approach removes the need to have many tables within the application, > indeed, but the consistency (size of table, number of started threads and > number of joined threads) has to be maintained by the caller. > My approach lets the helper maintain the consistency within a table and > force the application to separate different thread categories in different > tables... > > I still think my approach is more consistent. Hope I can convince you :-) > > Have a nice week-end, > > Christophe. > > > > Of course, I had a bug in my example above and missed the index into the > table... Logic is the same we already have in direct pthread/process calls > (provides the same flexibility). > > odph_odpthread_tbl_t *thr_tbl; > int num = 5; > odp_cpumask_t cpumask; > > thr_tbl = odph_odpthread_tbl_alloc(num); > > // create first > odp_cpumask_zero(&cpumask); > odp_cpumask_set(&cpumask, 1); > odph_odpthreads_create(&thr_tbl[0], &cpumask, ...); > > ... > > // create second > odp_cpumask_zero(&cpumask); > odp_cpumask_set(&cpumask, 2); > odph_odpthreads_create(&thr_tbl[1], &cpumask, ...); > > ... > > // create three more > odp_cpumask_zero(&cpumask); > odp_cpumask_set(&cpumask, 3); > odp_cpumask_set(&cpumask, 4); > odp_cpumask_set(&cpumask, 5); > odph_odpthreads_create(&thr_tbl[2], &cpumask, ...); > > ... > > // wait for the first two > odph_odpthreads_join(&thr_tbl[0], 2); > > // wait for the rest > odph_odpthreads_join(&thr_tbl[2], 3); > > odph_odpthread_tbl_free(thr_tbl); > > > -Petri > > That works, but is it really better? The applications are then forced to handle implicit sub tables in this single table (making sure those entries which have to be joined together are contiguous in the table). The application has to handle that by itself, remembering at which index each sub section starts and how many threads they includes. and allocating enough room for all at start. The approach I took delegates all this to the helpers: your sub-tables are forced into visibility (as separate table). Within each of these, the helpers maintain consistency. I still don't see what makes your approach better. Christophe.
On 30 May 2016 at 14:12, Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia.com> wrote: > > > > From: Christophe Milard [mailto:christophe.milard@linaro.org] > Sent: Monday, May 30, 2016 1:34 PM > To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia.com> > Cc: yi.he@linaro.org; lng-odp@lists.linaro.org > Subject: Re: [lng-odp] [PATCHv3] helper: cleaner interface to odph_odpthreads_create/join > > > > On 30 May 2016 at 11:23, Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia.com> wrote: > > > From: Christophe Milard [mailto:christophe.milard@linaro.org] > Sent: Friday, May 27, 2016 4:08 PM > To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia.com> > Cc: yi.he@linaro.org; lng-odp@lists.linaro.org > Subject: Re: [lng-odp] [PATCHv3] helper: cleaner interface to odph_odpthreads_create/join > > > > On 27 May 2016 at 09:56, Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia.com> wrote: > > > > -----Original Message----- > > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of > > Christophe Milard > > Sent: Friday, May 27, 2016 10:39 AM > > To: yi.he@linaro.org; lng-odp@lists.linaro.org > > Subject: [lng-odp] [PATCHv3] helper: cleaner interface to > > odph_odpthreads_create/join > > > > 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. > > > > > This change assumes that application will create all threads with a single call (which is not always practical or even possible). If application creates threads one by one, you'd end up with N separate thread table pointers, which application would need to store into another table anyway. > > Correct: My approach let tha application decides how it wants to group the threads: If the threads share the same functions to run etc, the application can handle it in a single call (1 table of N threads). If the threads cannot be stardes at the same time, tha application my take the N table if 1 entry instead. > > > It would be better to either define opaque odph_odpthread_t with correct size, or define alloc/free calls for a odph_odpthread_tbl_t. > > odph_odpthread_tbl_t *thr_tbl; > int num = 5; > odp_cpumask_t cpumask; > > thr_tbl = odph_odpthread_tbl_alloc(num); > > odp_cpumask_zero(&cpumask); > odp_cpumask_set(&cpumask, 1); > odph_odpthreads_create(thr_tbl, &cpumask, ...); > > ... > > odp_cpumask_zero(&cpumask); > odp_cpumask_set(&cpumask, 2); > odph_odpthreads_create(thr_tbl, &cpumask, ...); > > ... > > odp_cpumask_zero(&cpumask); > odp_cpumask_set(&cpumask, 3); > odp_cpumask_set(&cpumask, 4); > odp_cpumask_set(&cpumask, 5); > odph_odpthreads_create(thr_tbl, &cpumask, ...); > > hmmm... what would happen at join time? would the application have to provide a mask with the set of CPU threads to be join hence letting the responsibility of the join consistency to the application? or would all the threads in the table be joined (which is maybe not what the application wants)?... > > What happens the day we say more than one thread can run on each cpu? > (the current approach can easily be modified by passing an array of N cpu number -possibly including the same cpu many times-, instead of the current mask at creation time.). How would you describe which threads should be started or joined in your case? I guess an array of odp_thread ids would have to be given as well... or? > > I think the appraoch taken here is cleaner: each table is a group of threads which are created and joined at the same time. The helpers guarantees this consistency over such a group: the table size, the starting of all group threads and the joining of all group threads is done by the helper. > Yes, the application may have the need to create many such tables if it needs different groups of threads, but the consistency within each group is garanteed by the helper. > Besides, the need to have different tables for different group of threads does not necessarily make the application itself less clear: the pktio performance test, for instance, now defines two sets of threads: the TX threads and the RX threads. Is that worse than having a single table of threads and letting the application try to distinguish them by some other means (the cpu mask in your approach, as long as this works...) > > Your approach removes the need to have many tables within the application, indeed, but the consistency (size of table, number of started threads and number of joined threads) has to be maintained by the caller. > My approach lets the helper maintain the consistency within a table and force the application to separate different thread categories in different tables... > > I still think my approach is more consistent. Hope I can convince you :-) > > Have a nice week-end, > > Christophe. > > > Of course, I had a bug in my example above and missed the index into the table... Logic is the same we already have in direct pthread/process calls (provides the same flexibility). > > odph_odpthread_tbl_t *thr_tbl; > int num = 5; > odp_cpumask_t cpumask; > > thr_tbl = odph_odpthread_tbl_alloc(num); > > // create first > odp_cpumask_zero(&cpumask); > odp_cpumask_set(&cpumask, 1); > odph_odpthreads_create(&thr_tbl[0], &cpumask, ...); > > ... > > // create second > odp_cpumask_zero(&cpumask); > odp_cpumask_set(&cpumask, 2); > odph_odpthreads_create(&thr_tbl[1], &cpumask, ...); > > ... > > // create three more > odp_cpumask_zero(&cpumask); > odp_cpumask_set(&cpumask, 3); > odp_cpumask_set(&cpumask, 4); > odp_cpumask_set(&cpumask, 5); > odph_odpthreads_create(&thr_tbl[2], &cpumask, ...); > > ... > > // wait for the first two > odph_odpthreads_join(&thr_tbl[0], 2); > > // wait for the rest > odph_odpthreads_join(&thr_tbl[2], 3); > > odph_odpthread_tbl_free(thr_tbl); > > > -Petri > That works, but is it really better? The applications are then forced to handle implicit sub tables in this single table (making sure those entries which have to be joined together are contiguous in the table). The application has to handle that by itself, remembering at which index each sub section starts and how many threads they includes. and allocating enough room for all at start. > > The approach I took delegates all this to the helpers: your sub-tables are forced into visibility (as separate table). Within each of these, the helpers maintain consistency. > > I still don't see what makes your approach better. > > Christophe. > > > > No HTML mails, please ... the list converts HTML to plain text, but direct HTML mail to me still messes up my client. Sorry. Hope this is better now. > > > This approach is more flexible - application can decide how to manage the table(s). If application wants, it can also create N tables and save those N pointers (into another table). > > > odph_odpthread_tbl_t *thr_tbl[3]; > odp_cpumask_t cpumask; > > thr_tbl[0] = odph_odpthread_tbl_alloc(1); > thr_tbl[1] = odph_odpthread_tbl_alloc(1); > thr_tbl[2] = odph_odpthread_tbl_alloc(3); > > > // create first > odp_cpumask_zero(&cpumask); > odp_cpumask_set(&cpumask, 1); > odph_odpthreads_create(thr_tbl[0], &cpumask, ...); > > ... > > // create second > odp_cpumask_zero(&cpumask); > odp_cpumask_set(&cpumask, 2); > odph_odpthreads_create(thr_tbl[1], &cpumask, ...); > > ... > > // create three more > odp_cpumask_zero(&cpumask); > odp_cpumask_set(&cpumask, 3); > odp_cpumask_set(&cpumask, 4); > odp_cpumask_set(&cpumask, 5); > odph_odpthreads_create(thr_tbl[2], &cpumask, ...); > > ... > > // wait for the first > odph_odpthreads_join(thr_tbl[0], 1); > > // wait for the second > odph_odpthreads_join(thr_tbl[1], 1); > > // wait for the rest > odph_odpthreads_join(thr_tbl[2], 3); > > odph_odpthread_tbl_free(thr_tbl[0]); > odph_odpthread_tbl_free(thr_tbl[1]); > odph_odpthread_tbl_free(thr_tbl[2]); > > > I don't see why inflexibility would be better than flexibility here. confused. What is inflexible? Using my original approach, the application is given the chance to delegate the table consistency to the helpers. If it doesn't want to do so, it can create N tables of 1 entry, and handle things manually. There are actually examples of both ways in the different tests. Your approach forces the manual approach only, as far as I can see. Any other voices to comment on this? Yi? Others? Christophe. > > > -Petri > >
On 30 May 2016 at 15:44, Savolainen, Petri (Nokia - FI/Espoo) < petri.savolainen@nokia.com> wrote: > > >> -----Original Message----- >> From: Christophe Milard [mailto:christophe.milard@linaro.org] >> Sent: Monday, May 30, 2016 3:52 PM >> To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia.com> >> Cc: yi.he@linaro.org; lng-odp@lists.linaro.org >> Subject: Re: [lng-odp] [PATCHv3] helper: cleaner interface to >> odph_odpthreads_create/join >> >> On 30 May 2016 at 14:12, Savolainen, Petri (Nokia - FI/Espoo) >> <petri.savolainen@nokia.com> wrote: >> > >> > >> > >> > From: Christophe Milard [mailto:christophe.milard@linaro.org] >> > Sent: Monday, May 30, 2016 1:34 PM >> > To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia.com> >> > Cc: yi.he@linaro.org; lng-odp@lists.linaro.org >> > Subject: Re: [lng-odp] [PATCHv3] helper: cleaner interface to >> odph_odpthreads_create/join >> > >> > >> > >> > On 30 May 2016 at 11:23, Savolainen, Petri (Nokia - FI/Espoo) >> <petri.savolainen@nokia.com> wrote: >> > >> > >> > From: Christophe Milard [mailto:christophe.milard@linaro.org] >> > Sent: Friday, May 27, 2016 4:08 PM >> > To: Savolainen, Petri (Nokia - FI/Espoo) <petri.savolainen@nokia.com> >> > Cc: yi.he@linaro.org; lng-odp@lists.linaro.org >> > Subject: Re: [lng-odp] [PATCHv3] helper: cleaner interface to >> odph_odpthreads_create/join >> > >> > >> > >> > On 27 May 2016 at 09:56, Savolainen, Petri (Nokia - FI/Espoo) >> <petri.savolainen@nokia.com> wrote: >> > >> > >> > > -----Original Message----- >> > > From: lng-odp [mailto:lng-odp-bounces@lists.linaro.org] On Behalf Of >> > > Christophe Milard >> > > Sent: Friday, May 27, 2016 10:39 AM >> > > To: yi.he@linaro.org; lng-odp@lists.linaro.org >> > > Subject: [lng-odp] [PATCHv3] helper: cleaner interface to >> > > odph_odpthreads_create/join >> > > >> > > 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. >> > > >> > >> > >> > This change assumes that application will create all threads with a >> single call (which is not always practical or even possible). If >> application creates threads one by one, you'd end up with N separate >> thread table pointers, which application would need to store into another >> table anyway. >> > >> > Correct: My approach let tha application decides how it wants to group >> the threads: If the threads share the same functions to run etc, the >> application can handle it in a single call (1 table of N threads). If the >> threads cannot be stardes at the same time, tha application my take the N >> table if 1 entry instead. >> > >> > >> > It would be better to either define opaque odph_odpthread_t with correct >> size, or define alloc/free calls for a odph_odpthread_tbl_t. >> > >> > odph_odpthread_tbl_t *thr_tbl; >> > int num = 5; >> > odp_cpumask_t cpumask; >> > >> > thr_tbl = odph_odpthread_tbl_alloc(num); >> > >> > odp_cpumask_zero(&cpumask); >> > odp_cpumask_set(&cpumask, 1); >> > odph_odpthreads_create(thr_tbl, &cpumask, ...); >> > >> > ... >> > >> > odp_cpumask_zero(&cpumask); >> > odp_cpumask_set(&cpumask, 2); >> > odph_odpthreads_create(thr_tbl, &cpumask, ...); >> > >> > ... >> > >> > odp_cpumask_zero(&cpumask); >> > odp_cpumask_set(&cpumask, 3); >> > odp_cpumask_set(&cpumask, 4); >> > odp_cpumask_set(&cpumask, 5); >> > odph_odpthreads_create(thr_tbl, &cpumask, ...); >> > >> > hmmm... what would happen at join time? would the application have to >> provide a mask with the set of CPU threads to be join hence letting the >> responsibility of the join consistency to the application? or would all >> the threads in the table be joined (which is maybe not what the >> application wants)?... >> > >> > What happens the day we say more than one thread can run on each cpu? >> > (the current approach can easily be modified by passing an array of N >> cpu number -possibly including the same cpu many times-, instead of the >> current mask at creation time.). How would you describe which threads >> should be started or joined in your case? I guess an array of odp_thread >> ids would have to be given as well... or? >> > >> > I think the appraoch taken here is cleaner: each table is a group of >> threads which are created and joined at the same time. The helpers >> guarantees this consistency over such a group: the table size, the >> starting of all group threads and the joining of all group threads is done >> by the helper. >> > Yes, the application may have the need to create many such tables if it >> needs different groups of threads, but the consistency within each group >> is garanteed by the helper. >> > Besides, the need to have different tables for different group of >> threads does not necessarily make the application itself less clear: the >> pktio performance test, for instance, now defines two sets of threads: the >> TX threads and the RX threads. Is that worse than having a single table >> of threads and letting the application try to distinguish them by some >> other means (the cpu mask in your approach, as long as this works...) >> > >> > Your approach removes the need to have many tables within the >> application, indeed, but the consistency (size of table, number of started >> threads and number of joined threads) has to be maintained by the caller. >> > My approach lets the helper maintain the consistency within a table and >> force the application to separate different thread categories in different >> tables... >> > >> > I still think my approach is more consistent. Hope I can convince you :- >> ) >> > >> > Have a nice week-end, >> > >> > Christophe. >> > >> > >> > Of course, I had a bug in my example above and missed the index into the >> table... Logic is the same we already have in direct pthread/process calls >> (provides the same flexibility). >> > >> > odph_odpthread_tbl_t *thr_tbl; >> > int num = 5; >> > odp_cpumask_t cpumask; >> > >> > thr_tbl = odph_odpthread_tbl_alloc(num); >> > >> > // create first >> > odp_cpumask_zero(&cpumask); >> > odp_cpumask_set(&cpumask, 1); >> > odph_odpthreads_create(&thr_tbl[0], &cpumask, ...); >> > >> > ... >> > >> > // create second >> > odp_cpumask_zero(&cpumask); >> > odp_cpumask_set(&cpumask, 2); >> > odph_odpthreads_create(&thr_tbl[1], &cpumask, ...); >> > >> > ... >> > >> > // create three more >> > odp_cpumask_zero(&cpumask); >> > odp_cpumask_set(&cpumask, 3); >> > odp_cpumask_set(&cpumask, 4); >> > odp_cpumask_set(&cpumask, 5); >> > odph_odpthreads_create(&thr_tbl[2], &cpumask, ...); >> > >> > ... >> > >> > // wait for the first two >> > odph_odpthreads_join(&thr_tbl[0], 2); >> > >> > // wait for the rest >> > odph_odpthreads_join(&thr_tbl[2], 3); >> > >> > odph_odpthread_tbl_free(thr_tbl); >> > >> > >> > -Petri >> > That works, but is it really better? The applications are then forced to >> handle implicit sub tables in this single table (making sure those entries >> which have to be joined together are contiguous in the table). The >> application has to handle that by itself, remembering at which index each >> sub section starts and how many threads they includes. and allocating >> enough room for all at start. >> > >> > The approach I took delegates all this to the helpers: your sub-tables >> are forced into visibility (as separate table). Within each of these, the >> helpers maintain consistency. >> > >> > I still don't see what makes your approach better. >> > >> > Christophe. >> > >> > >> > >> > No HTML mails, please ... the list converts HTML to plain text, but >> direct HTML mail to me still messes up my client. >> >> Sorry. Hope this is better now. > > It's better. Thanks. > >> >> > >> > >> > This approach is more flexible - application can decide how to manage >> the table(s). If application wants, it can also create N tables and save >> those N pointers (into another table). >> > >> > >> > odph_odpthread_tbl_t *thr_tbl[3]; >> > odp_cpumask_t cpumask; >> > >> > thr_tbl[0] = odph_odpthread_tbl_alloc(1); >> > thr_tbl[1] = odph_odpthread_tbl_alloc(1); >> > thr_tbl[2] = odph_odpthread_tbl_alloc(3); >> > >> > >> > // create first >> > odp_cpumask_zero(&cpumask); >> > odp_cpumask_set(&cpumask, 1); >> > odph_odpthreads_create(thr_tbl[0], &cpumask, ...); >> > >> > ... >> > >> > // create second >> > odp_cpumask_zero(&cpumask); >> > odp_cpumask_set(&cpumask, 2); >> > odph_odpthreads_create(thr_tbl[1], &cpumask, ...); >> > >> > ... >> > >> > // create three more >> > odp_cpumask_zero(&cpumask); >> > odp_cpumask_set(&cpumask, 3); >> > odp_cpumask_set(&cpumask, 4); >> > odp_cpumask_set(&cpumask, 5); >> > odph_odpthreads_create(thr_tbl[2], &cpumask, ...); >> > >> > ... >> > >> > // wait for the first >> > odph_odpthreads_join(thr_tbl[0], 1); >> > >> > // wait for the second >> > odph_odpthreads_join(thr_tbl[1], 1); >> > >> > // wait for the rest >> > odph_odpthreads_join(thr_tbl[2], 3); >> > >> > odph_odpthread_tbl_free(thr_tbl[0]); >> > odph_odpthread_tbl_free(thr_tbl[1]); >> > odph_odpthread_tbl_free(thr_tbl[2]); >> > >> > >> > I don't see why inflexibility would be better than flexibility here. >> >> confused. What is inflexible? Using my original approach, the >> application is given the chance to delegate the table consistency to >> the helpers. If it doesn't want to do so, it can create N tables of 1 >> entry, and handle things manually. There are actually examples of both >> ways in the different tests. >> >> Your approach forces the manual approach only, as far as I can see. >> >> Any other voices to comment on this? Yi? Others? >> >> Christophe. > > > Your create function is actually a thread group alloc/create/start. It forces to start and stop (join) a group threads as one entity. If an application does not know (in advance) which thread will be terminated first, etc it needs to create every thread into its own "table" (group), which moves the "thread handle table" back to the application side. Correct. The helper handles the consistency within such a group. If the app wants to keep control, it can create tablesd of size 1: The app has the choice, hence the flexibility :-) > > It's better to output all handles (or pointers) to application defined location, so application logic does not have to change when thread create/join sequence changes (e.g. between different test runs) > > thr_hdl[MAX_THR]; > > // create one by one > odph_odpthreads_create(&thr_hdl[0], ...); > odph_odpthreads_create(&thr_hdl[1], ...); > odph_odpthreads_create(&thr_hdl[2], ...); > > // join one by one in reverse order > odph_odpthreads_join(&thr_tbl[2], 1); > odph_odpthreads_join(&thr_tbl[1], 1); > odph_odpthreads_join(&thr_tbl[0], 1); > > > ... works with the same logic as ... > > // create first two > odph_odpthreads_create(&thr_hdl[0..1], ...); > > // create third one > odph_odpthreads_create(&thr_hdl[2], ...); > > // join last two threads > odph_odpthreads_join(&thr_tbl[1], 2); > > // join the first thread > odph_odpthreads_join(&thr_tbl[0], 1); > Sure, when the app does not want to use the grouping provided by my approach, tables of size 1 is not of much use. Having said that, looking at what we have done so far shows that in many case application do "thread" many times the same thing, just to benefit of the speed of parallel cpus. I assume that with your approach, the name odph_odpthread_tbl_t is inappropriate and should be called odph_odpthread_t, as before, as the application really handles the table. Your suggestion is to abstract the odph_odpthread_t (as a void*) and keep the old interface unchanged. At least, I think we understand fully both approaches. I guess I did not really succeed to convince you. Christophe > > -Petri >
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..405ae23 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_impl_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_impl_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_impl_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,20 +412,30 @@ int odph_odpthreads_create(odph_odpthread_t *thread_tbl, int num; int cpu_count; int cpu; + _odph_odpthread_impl_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) { ODPH_ERR("Invalid number of odpthreads:%d" " (%d cores available)\n", num, cpu_count); + *thread_tbl_ptr = NULL; + return -1; + } + + thread_tbl = malloc(num * sizeof(_odph_odpthread_impl_t)); + *thread_tbl_ptr = (void *)thread_tbl; + + if (!thread_tbl) { + ODPH_ERR("malloc failed!"); return -1; } + memset(thread_tbl, 0, num * sizeof(_odph_odpthread_impl_t)); + cpu = odp_cpumask_first(mask); for (i = 0; i < num; i++) { if (!helper_options.proc) { @@ -420,8 +460,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_impl_t *thread_tbl; pid_t pid; int i = 0; int terminated = 0; @@ -430,6 +471,13 @@ int odph_odpthreads_join(odph_odpthread_t *thread_tbl) int ret; int retval = 0; + thread_tbl = (_odph_odpthread_impl_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 +537,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> --- since V2: - malloc return value check in odph_odpthreads_create() since V1: -_odph_odpthread_t changed to _odph_odpthread_impl_t (Hi) - full init of the odph_odpthread_tbl_t by odph_odpthreads_create() even in case of error. (Hi) 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 | 67 +++++++++++++++++++++++++++---- 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, 99 insertions(+), 95 deletions(-)