On 04/28 17:18:36, Christophe Milard wrote:
> Since v3:
> -fixed rebase error (Christophe)
> -rebased
Thanks for the rebase. test_in_process_mode_v4 merged cleanly into
origin/master and build and tests PASS.
I've given this a look, and it appears we're headed in the right direction.
> diff --git a/example/classifier/odp_classifier.c
> b/example/classifier/odp_classifier.c
> index a477e23..4057457 100644
> --- a/example/classifier/odp_classifier.c
> +++ b/example/classifier/odp_classifier.c
> @@ -815,10 +802,16 @@ static void parse_args(int argc, char *argv[],
> appl_args_t *appl_args)
> {NULL, 0, NULL, 0}
> };
>
> + static const char *shortopts = "+c:t:i:p:m:t:h";
> +
> + /* let helper collect its own arguments (e.g. --odph_proc) */
> + odph_parse_options(argc, argv, shortopts, longopts);
> +
> + opterr = 0; /* do not issue errors on helper options */
Please use the default behavior of opterr _or_ add the case statement for '?'
when appropriate.
> while (1) {
> - opt = getopt_long(argc, argv, "+c:t:i:p:m:t:h",
> - longopts, &long_index);
> + opt = getopt_long(argc, argv, shortopts,
> + longopts, &long_index);
>
> if (opt == -1)
> break; /* No more options */
> diff --git a/example/l2fwd_simple/odp_l2fwd_simple.c
> b/example/l2fwd_simple/odp_l2fwd_simple.c
> index 45bb9b1..7b67705 100644
> --- a/example/l2fwd_simple/odp_l2fwd_simple.c
> +++ b/example/l2fwd_simple/odp_l2fwd_simple.c
> @@ -116,13 +117,33 @@ int main(int argc, char **argv)
> odp_pool_t pool;
> odp_pool_param_t params;
> odp_cpumask_t cpumask;
> - odph_linux_pthread_t thd;
> + odph_odpthread_t thd;
> odp_instance_t instance;
> - odph_linux_thr_params_t thr_params;
> + odph_odpthread_params_t thr_params;
> + int rc = 0;
> + int opt;
> + int long_index;
> +
> + static const struct option longopts[] = { {NULL, 0, NULL, 0} };
> + static const char *shortopts = "";
> +
> + /* let helper collect its own arguments (e.g. --odph_proc) */
> + odph_parse_options(argc, argv, shortopts, longopts);
> +
> + /*
> + * parse own options: currentely none, but this will move optind
> + * to the first non-option argument. (in case there where helprt args)
> + */
I suppose this is OK given the pre-existing arg handling.
> + opterr = 0; /* do not issue errors on helper options */
> + while (!rc) {
Please use a simpler loop, e.g. for (;;) or while (1)
> + opt = getopt_long(argc, argv, shortopts, longopts, &long_index);
> + if (-1 == opt)
> + break; /* No more options */
> + }
>
> - if (argc != 5 ||
> - odph_eth_addr_parse(&global.dst, argv[3]) != 0 ||
> - odph_eth_addr_parse(&global.src, argv[4]) != 0) {
> + if (argc != optind + 4 ||
> + odph_eth_addr_parse(&global.dst, argv[optind + 2]) != 0 ||
> + odph_eth_addr_parse(&global.src, argv[optind + 3]) != 0) {
> printf("Usage: odp_l2fwd_simple eth0 eth1 01:02:03:04:05:06"
> " 07:08:09:0a:0b:0c\n");
> printf("Where eth0 and eth1 are the used interfaces"
> diff --git a/helper/include/odp/helper/linux.h
> b/helper/include/odp/helper/linux.h
> index 7a6504f..a9ec90a 100644
> --- a/helper/include/odp/helper/linux.h
> +++ b/helper/include/odp/helper/linux.h
> @@ -25,111 +25,146 @@ extern "C" {
> #include <odp_api.h>
>
> #include <pthread.h>
> +#include <getopt.h>
Please consider migrating CLI parsing via C library to a more appropriate
location.
> #include <sys/types.h>
>
> -/** Thread parameter for Linux pthreads and processes */
> +/** odpthread linux type: whether an ODP thread is a linux thread or process
> */
> +typedef enum odph_odpthread_linuxtype_e {
> + NOT_STARTED = 0,
> + PROCESS,
> + PTHREAD
> +} odph_odpthread_linuxtype_t;
Please use more descriptive identifiers in enums and consider a MAX if
used for iteration.
> +/** odpthread parameters for odp threads (pthreads and processes) */
> typedef struct {
> - void *(*start)(void *); /**< Thread entry point function */
> + int (*start)(void *); /**< Thread entry point function */
> void *arg; /**< Argument for the function */
> odp_thread_type_t thr_type; /**< ODP thread type */
> odp_instance_t instance; /**< ODP instance handle */
> -} odph_linux_thr_params_t;
> +} odph_odpthread_params_t;
>
> -/** Linux pthread state information */
> +/** The odpthread starting arguments, used both in process or thread mode */
> typedef struct {
> - pthread_t thread; /**< Pthread ID */
> - pthread_attr_t attr; /**< Pthread attributes */
> - int cpu; /**< CPU ID */
> - /** Copy of thread params */
> - odph_linux_thr_params_t thr_params;
> -} odph_linux_pthread_t;
> -
> + odph_odpthread_linuxtype_t linuxtype;
> + odph_odpthread_params_t thr_params; /*copy of thread start parameter*/
> +} odph_odpthread_start_args_t;
>
> -/** Linux process state information */
> +/** Linux odpthread state information, used both in process or thread mode */
> typedef struct {
> - pid_t pid; /**< Process ID */
> - int cpu; /**< CPU ID */
> - int status; /**< Process state change status */
> -} odph_linux_process_t;
> + odph_odpthread_start_args_t start_args;
> + int cpu; /**< CPU ID */
> + int last; /**< true if last table entry */
> + union {
> + struct { /* for thread implementation */
> + pthread_t thread; /**< Pthread ID */
thread_id
> + pthread_attr_t attr; /**< Pthread attributes */
If threading affinity is determined at creation time, and never changes, please
consider reducing the lifetime of this variable.
> + };
> + struct { /* for process implementation */
> + pid_t pid; /**< Process ID */
> + int status; /**< Process state chge status*/
> + };
> + };
Please consider avoiding the use of anonymous structures within an anonymous
union as it makes the code harder to read.
> +} odph_odpthread_t;
>
> /**
> - * Creates and launches pthreads
> + * Creates and launches odpthreads (as linux threads or processes)
> *
> * Creates, pins and launches threads to separate CPU's based on the cpumask.
> *
> - * @param[out] pthread_tbl Table of pthread state information records. Table
> - * must have at least as many entries as there are
> - * CPUs in the CPU mask.
> - * @param mask CPU mask
> - * @param thr_params Linux helper thread parameters
> + * @param thread_tbl Thread table
> + * @param mask CPU mask
> + * @param start_routine Thread start function
> + * @param arg Thread argument
> + * @param thr_type Thread type
Please document thr_params and remove start_routine, arg, thr_type
> *
> * @return Number of threads created
> */
> -int odph_linux_pthread_create(odph_linux_pthread_t *pthread_tbl,
> - const odp_cpumask_t *mask,
> - const odph_linux_thr_params_t *thr_params);
> +int odph_odpthreads_create(odph_odpthread_t *thread_tbl,
> + const odp_cpumask_t *mask,
> + const odph_odpthread_params_t *thr_params);
>
> /**
> - * Waits pthreads to exit
> + * Waits odpthreads (as linux threads or processes) to exit.
> *
> - * Returns when all threads have been exit.
> + * Returns when all odpthreads have terminated.
> *
> * @param thread_tbl Thread table
> - * @param num Number of threads to create
> - *
> - */
> -void odph_linux_pthread_join(odph_linux_pthread_t *thread_tbl, int num);
> -
> -
> -/**
> - * Fork a process
> - *
> - * Forks and sets CPU affinity for the child process. Ignores 'start' and
> 'arg'
> - * thread parameters.
> - *
> - * @param[out] proc Pointer to process state info (for output)
> - * @param cpu Destination CPU for the child process
> - * @param thr_params Linux helper thread parameters
> + * @return The number of joined threads or -1 on error.
> + * (error occurs if any of the start_routine return non-zero or if
> + * the thread join/process wait itself failed -e.g. as the result of a kill)
>
> diff --git a/helper/linux.c b/helper/linux.c
> index 24e243b..dd6ab8b 100644
> --- a/helper/linux.c
> +++ b/helper/linux.c
> @@ -1,4 +1,4 @@
> -/* Copyright (c) 2013, Linaro Limited
> +/* Copyright (c) 2013, Linaro Limited
Please revert this change
> * All rights reserved.
> *
> * SPDX-License-Identifier: BSD-3-Clause
> @@ -21,218 +21,368 @@
> #include <odp/helper/linux.h>
> #include "odph_debug.h"
>
> -static void *odp_run_start_routine(void *arg)
> +static struct {
> + int proc; /* true when process mode is required */
> + int thrd; /* true when thread mode is required */
> +} helper_options;
This is a boolean. Please avoid the use of global state for code like this.
> +/*
> + * wrapper for odpthreads, either implemented as linux threads or processes.
> + * (in process mode, if start_routine returns NULL, the process return 1.
> + */
> +static void *odpthread_run_start_routine(void *arg)
> {
> - odph_linux_thr_params_t *thr_params = arg;
> + int status;
> + int ret;
> + odph_odpthread_params_t *thr_params;
> +
> + odph_odpthread_start_args_t *start_args = arg;
> +
> + thr_params = &start_args->thr_params;
>
> /* ODP thread local init */
> if (odp_init_local(thr_params->instance, thr_params->thr_type)) {
> ODPH_ERR("Local init failed\n");
> + if (start_args->linuxtype == PROCESS)
> + exit(-1);
> return NULL;
> }
>
> - void *ret_ptr = thr_params->start(thr_params->arg);
> - int ret = odp_term_local();
> + /* debug: */
> + printf("helper: ODP %s thread started as linux %s. (pid=%d)\n",
> + thr_params->thr_type == ODP_THREAD_WORKER ? "worker" : "control",
> + (start_args->linuxtype == PTHREAD) ? "pthread" : "process",
> + (int)getpid());
Please use appropriate logging facilities instead of printf().
> + status = thr_params->start(thr_params->arg);
> + ret = odp_term_local();
>
> if (ret < 0)
> ODPH_ERR("Local term failed\n");
> else if (ret == 0 && odp_term_global(thr_params->instance))
> ODPH_ERR("Global term failed\n");
>
> - return ret_ptr;
> + /* for process implementation of odp threads, just return status... */
> + if (start_args->linuxtype == PROCESS)
> + exit(status);
> +
> + /* threads implementation return void* pointers: cast status to that. */
Please avoid explicit cast.
> + return (void *)(long)status;
> }
This review is 1% finished.
_______________________________________________
lng-odp mailing list
[email protected]
https://lists.linaro.org/mailman/listinfo/lng-odp
