On 29 April 2016 at 00:48, Brian Brooks <brian.bro...@linaro.org> wrote:
> On 04/28 17:18:36, Christophe Milard wrote:
> > Since v3:
> > -fixed rebase error (Christophe)
> > -rebased
>
> Thanks for the rebase. test_in_process_mode_v4 merged cleanly into
> origin/master and build and tests PASS.
>
> I've given this a look, and it appears we're headed in the right direction.
>
> > diff --git a/example/classifier/odp_classifier.c
> b/example/classifier/odp_classifier.c
> > index a477e23..4057457 100644
> > --- a/example/classifier/odp_classifier.c
> > +++ b/example/classifier/odp_classifier.c
> > @@ -815,10 +802,16 @@ static void parse_args(int argc, char *argv[],
> appl_args_t *appl_args)
> > {NULL, 0, NULL, 0}
> > };
> >
> > + static const char *shortopts = "+c:t:i:p:m:t:h";
> > +
> > + /* let helper collect its own arguments (e.g. --odph_proc) */
> > + odph_parse_options(argc, argv, shortopts, longopts);
> > +
> > + opterr = 0; /* do not issue errors on helper options */
>
> Please use the default behavior of opterr _or_ add the case statement for
> '?'
> when appropriate.
>
I do think we need opterr=0, sadly: when the test or example (such as the
classifier here) parses it options, the command line options being parsed
still contains all options, i.e. both the option meant for the classifier
itself, and the option meant to the "other layers", (here the helper). If
opt_err is left to 1, the classifier's call to get_opt() will issue errors
when hitting the helper options, even if we have a "?" in the switch, as
far as I understand.
So the mechanism taken here is: the caller (e.g. classifier) passes its
list of option to the helper which builds the complete list of options by
merging it own (helper) option list to the caller options. Then the helper
parse this complete list (with default opterr=1), i.e. reacting to unknown
options (not being in the merged list) and also picking up its own option
semantic, of course. After this the caller (here classifier) parse the
options again, picking up its own options only. but should not react on the
helper's otpions.
I Actually looked at removing the options from argv in the helpers, but
this turned up to be quite tricky as well.
parse_args() seem to be better at spitting command line option in different
owner, but is is not POSIX. not sure we want to insert that kind of
dependency for so little.
>
> > while (1) {
> > - opt = getopt_long(argc, argv, "+c:t:i:p:m:t:h",
> > - longopts, &long_index);
> > + opt = getopt_long(argc, argv, shortopts,
> > + longopts, &long_index);
> >
> > if (opt == -1)
> > break; /* No more options */
> > diff --git a/example/l2fwd_simple/odp_l2fwd_simple.c
> b/example/l2fwd_simple/odp_l2fwd_simple.c
> > index 45bb9b1..7b67705 100644
> > --- a/example/l2fwd_simple/odp_l2fwd_simple.c
> > +++ b/example/l2fwd_simple/odp_l2fwd_simple.c
> > @@ -116,13 +117,33 @@ int main(int argc, char **argv)
> > odp_pool_t pool;
> > odp_pool_param_t params;
> > odp_cpumask_t cpumask;
> > - odph_linux_pthread_t thd;
> > + odph_odpthread_t thd;
> > odp_instance_t instance;
> > - odph_linux_thr_params_t thr_params;
> > + odph_odpthread_params_t thr_params;
> > + int rc = 0;
> > + int opt;
> > + int long_index;
> > +
> > + static const struct option longopts[] = { {NULL, 0, NULL, 0} };
> > + static const char *shortopts = "";
> > +
> > + /* let helper collect its own arguments (e.g. --odph_proc) */
> > + odph_parse_options(argc, argv, shortopts, longopts);
> > +
> > + /*
> > + * parse own options: currentely none, but this will move optind
> > + * to the first non-option argument. (in case there where helprt
> args)
> > + */
>
> I suppose this is OK given the pre-existing arg handling.
>
> > + opterr = 0; /* do not issue errors on helper options */
> > + while (!rc) {
>
> Please use a simpler loop, e.g. for (;;) or while (1)
>
Of course, there is not much left for rc there! will be addressed in v5
>
> > + opt = getopt_long(argc, argv, shortopts, longopts,
> &long_index);
> > + if (-1 == opt)
> > + break; /* No more options */
> > + }
> >
> > - if (argc != 5 ||
> > - odph_eth_addr_parse(&global.dst, argv[3]) != 0 ||
> > - odph_eth_addr_parse(&global.src, argv[4]) != 0) {
> > + if (argc != optind + 4 ||
> > + odph_eth_addr_parse(&global.dst, argv[optind + 2]) != 0 ||
> > + odph_eth_addr_parse(&global.src, argv[optind + 3]) != 0) {
> > printf("Usage: odp_l2fwd_simple eth0 eth1
> 01:02:03:04:05:06"
> > " 07:08:09:0a:0b:0c\n");
> > printf("Where eth0 and eth1 are the used interfaces"
> > diff --git a/helper/include/odp/helper/linux.h
> b/helper/include/odp/helper/linux.h
> > index 7a6504f..a9ec90a 100644
> > --- a/helper/include/odp/helper/linux.h
> > +++ b/helper/include/odp/helper/linux.h
> > @@ -25,111 +25,146 @@ extern "C" {
> > #include <odp_api.h>
> >
> > #include <pthread.h>
> > +#include <getopt.h>
>
> Please consider migrating CLI parsing via C library to a more appropriate
> location.
>
Not sure I understand this: The goal is to have the helpers parsing their
own options here. Maybe my comment above makes it clearer. otherwise please
explain what you meant.
>
> > #include <sys/types.h>
> >
> > -/** Thread parameter for Linux pthreads and processes */
> > +/** odpthread linux type: whether an ODP thread is a linux thread or
> process */
> > +typedef enum odph_odpthread_linuxtype_e {
> > + NOT_STARTED = 0,
> > + PROCESS,
> > + PTHREAD
> > +} odph_odpthread_linuxtype_t;
>
> Please use more descriptive identifiers in enums and consider a MAX if
> used for iteration.
>
Any suggestion to any better names? What is meant here is: either the
odpthread has never been started (in which case it is neither a linux
process or a linux thread), or it is started as a linux process, or it is
started as a linux pthread. There are not used in iteration.
Maybe the confusion comes from what an odp thread is. I am not very keen on
the naming, but despite its name, an ODP thread is just a concurrent
execution task. On linux these can be implemented as pthreads or processes.
The name "odp tasks" would in my eyes be better (as not as correlated to
any specific implementation), but that was not accepted.
>
> > +/** odpthread parameters for odp threads (pthreads and processes) */
> > typedef struct {
> > - void *(*start)(void *); /**< Thread entry point function */
> > + int (*start)(void *); /**< Thread entry point function */
> > void *arg; /**< Argument for the function */
> > odp_thread_type_t thr_type; /**< ODP thread type */
> > odp_instance_t instance; /**< ODP instance handle */
> > -} odph_linux_thr_params_t;
> > +} odph_odpthread_params_t;
> >
> > -/** Linux pthread state information */
> > +/** The odpthread starting arguments, used both in process or thread
> mode */
> > typedef struct {
> > - pthread_t thread; /**< Pthread ID */
> > - pthread_attr_t attr; /**< Pthread attributes */
> > - int cpu; /**< CPU ID */
> > - /** Copy of thread params */
> > - odph_linux_thr_params_t thr_params;
> > -} odph_linux_pthread_t;
> > -
> > + odph_odpthread_linuxtype_t linuxtype;
> > + odph_odpthread_params_t thr_params; /*copy of thread start
> parameter*/
> > +} odph_odpthread_start_args_t;
> >
> > -/** Linux process state information */
> > +/** Linux odpthread state information, used both in process or thread
> mode */
> > typedef struct {
> > - pid_t pid; /**< Process ID */
> > - int cpu; /**< CPU ID */
> > - int status; /**< Process state change status */
> > -} odph_linux_process_t;
> > + odph_odpthread_start_args_t start_args;
> > + int cpu; /**< CPU ID */
> > + int last; /**< true if last table
> entry */
> > + union {
> > + struct { /* for thread implementation */
> > + pthread_t thread; /**< Pthread ID */
>
> thread_id
>
Ok. will take that in v5.
>
> > + pthread_attr_t attr; /**< Pthread attributes */
>
> If threading affinity is determined at creation time, and never changes,
> please
> consider reducing the lifetime of this variable.
>
I am not sure what to do here: I don't really understand why this
particular member is special here. The attribute themselves are
created/destroyed at thread creation/destruction time.This is just a
typedef, but indeed these structures (including all their members) are
today staticaly allocated in tests. I have not changed that from the old
code. I do not think this specific member is special and should be
separated from this structure: it really belongs to it.
What should be done, I think, is reducing the lifetime of the whole
structure (allocating then at task/process creation time and freeing them
at join time.).
This can be addressed in another patch.
>
> > + };
> > + struct { /* for process implementation */
> > + pid_t pid; /**< Process ID */
> > + int status; /**< Process state chge
> status*/
> > + };
> > + };
>
> Please consider avoiding the use of anonymous structures within an
> anonymous
> union as it makes the code harder to read.
>
will be fixed in v5. (inherited from old code)
>
> > +} odph_odpthread_t;
> >
> > /**
> > - * Creates and launches pthreads
> > + * Creates and launches odpthreads (as linux threads or processes)
> > *
> > * Creates, pins and launches threads to separate CPU's based on the
> cpumask.
> > *
> > - * @param[out] pthread_tbl Table of pthread state information records.
> Table
> > - * must have at least as many entries as there
> are
> > - * CPUs in the CPU mask.
> > - * @param mask CPU mask
> > - * @param thr_params Linux helper thread parameters
> > + * @param thread_tbl Thread table
> > + * @param mask CPU mask
> > + * @param start_routine Thread start function
> > + * @param arg Thread argument
> > + * @param thr_type Thread type
>
> Please document thr_params and remove start_routine, arg, thr_type
>
Ooops! That passed through the rebase. Will fix in V5. Thx!
>
> > *
> > * @return Number of threads created
> > */
> > -int odph_linux_pthread_create(odph_linux_pthread_t *pthread_tbl,
> > - const odp_cpumask_t *mask,
> > - const odph_linux_thr_params_t *thr_params);
> > +int odph_odpthreads_create(odph_odpthread_t *thread_tbl,
> > + const odp_cpumask_t *mask,
> > + const odph_odpthread_params_t *thr_params);
> >
> > /**
> > - * Waits pthreads to exit
> > + * Waits odpthreads (as linux threads or processes) to exit.
> > *
> > - * Returns when all threads have been exit.
> > + * Returns when all odpthreads have terminated.
> > *
> > * @param thread_tbl Thread table
> > - * @param num Number of threads to create
> > - *
> > - */
> > -void odph_linux_pthread_join(odph_linux_pthread_t *thread_tbl, int num);
> > -
> > -
> > -/**
> > - * Fork a process
> > - *
> > - * Forks and sets CPU affinity for the child process. Ignores 'start'
> and 'arg'
> > - * thread parameters.
> > - *
> > - * @param[out] proc Pointer to process state info (for output)
> > - * @param cpu Destination CPU for the child process
> > - * @param thr_params Linux helper thread parameters
> > + * @return The number of joined threads or -1 on error.
> > + * (error occurs if any of the start_routine return non-zero or if
> > + * the thread join/process wait itself failed -e.g. as the result of a
> kill)
> >
> > diff --git a/helper/linux.c b/helper/linux.c
> > index 24e243b..dd6ab8b 100644
> > --- a/helper/linux.c
> > +++ b/helper/linux.c
> > @@ -1,4 +1,4 @@
> > -/* Copyright (c) 2013, Linaro Limited
> > +/* Copyright (c) 2013, Linaro Limited
>
> Please revert this change
>
Wonder how I did that. fixed for V5.
>
> > * All rights reserved.
> > *
> > * SPDX-License-Identifier: BSD-3-Clause
> > @@ -21,218 +21,368 @@
> > #include <odp/helper/linux.h>
> > #include "odph_debug.h"
> >
> > -static void *odp_run_start_routine(void *arg)
> > +static struct {
> > + int proc; /* true when process mode is required */
> > + int thrd; /* true when thread mode is required */
> > +} helper_options;
>
> This is a boolean. Please avoid the use of global state for code like this.
>
bool: bool is C99 only and the usage of int as bool seems well spread
within the code, but there spots with bool as well, I have to admit. But
Getopt() expects an int* as third member of its option structure... do you
really think I should use bool and then cast it to int for Getopt()?. My
feeling is it is as good with int straight away
global: isn't this the best solution to avoid polluting the caller space?
(as main belongs to another space and you want the options to be visible
for all the helpers. Any better approach is welcome
>
> > +/*
> > + * wrapper for odpthreads, either implemented as linux threads or
> processes.
> > + * (in process mode, if start_routine returns NULL, the process return
> 1.
> > + */
> > +static void *odpthread_run_start_routine(void *arg)
> > {
> > - odph_linux_thr_params_t *thr_params = arg;
> > + int status;
> > + int ret;
> > + odph_odpthread_params_t *thr_params;
> > +
> > + odph_odpthread_start_args_t *start_args = arg;
> > +
> > + thr_params = &start_args->thr_params;
> >
> > /* ODP thread local init */
> > if (odp_init_local(thr_params->instance, thr_params->thr_type)) {
> > ODPH_ERR("Local init failed\n");
> > + if (start_args->linuxtype == PROCESS)
> > + exit(-1);
> > return NULL;
> > }
> >
> > - void *ret_ptr = thr_params->start(thr_params->arg);
> > - int ret = odp_term_local();
> > + /* debug: */
> > + printf("helper: ODP %s thread started as linux %s. (pid=%d)\n",
> > + thr_params->thr_type == ODP_THREAD_WORKER ? "worker" :
> "control",
> > + (start_args->linuxtype == PTHREAD) ? "pthread" : "process",
> > + (int)getpid());
>
> Please use appropriate logging facilities instead of printf().
>
fixed in V5
>
> > + status = thr_params->start(thr_params->arg);
> > + ret = odp_term_local();
> >
> > if (ret < 0)
> > ODPH_ERR("Local term failed\n");
> > else if (ret == 0 && odp_term_global(thr_params->instance))
> > ODPH_ERR("Global term failed\n");
> >
> > - return ret_ptr;
> > + /* for process implementation of odp threads, just return
> status... */
> > + if (start_args->linuxtype == PROCESS)
> > + exit(status);
> > +
> > + /* threads implementation return void* pointers: cast status to
> that. */
>
> Please avoid explicit cast.
>
Any other way to transform an int to a pointer which is what we need to do
to be able to run the same code for processes (returning an int status) and
a thread (returning a void*) ?
>
> > + return (void *)(long)status;
> > }
>
> This review is 1% finished.
>
Thanks for looking at it anyway: I guess I should wait for V5...?
I have fixed those things I said V5 on. other topics I discussed have not
been changed, so either confirm that you understand and agree or come with
suggestions.
Once again many hx
/Christophe
_______________________________________________
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp
