On 04/29 13:29:03, Christophe Milard wrote:
> On 29 April 2016 at 00:48, Brian Brooks <[email protected]> wrote:
>
> > On 04/28 17:18:36, Christophe Milard wrote:
> > > Since v3:
> > > -fixed rebase error (Christophe)
> > > -rebased
> >
> > Thanks for the rebase. test_in_process_mode_v4 merged cleanly into
> > origin/master and build and tests PASS.
> >
> > I've given this a look, and it appears we're headed in the right direction.
> >
> > > diff --git a/example/classifier/odp_classifier.c
> > b/example/classifier/odp_classifier.c
> > > index a477e23..4057457 100644
> > > --- a/example/classifier/odp_classifier.c
> > > +++ b/example/classifier/odp_classifier.c
> > > @@ -815,10 +802,16 @@ static void parse_args(int argc, char *argv[],
> > appl_args_t *appl_args)
> > > {NULL, 0, NULL, 0}
> > > };
> > >
> > > + static const char *shortopts = "+c:t:i:p:m:t:h";
> > > +
> > > + /* let helper collect its own arguments (e.g. --odph_proc) */
> > > + odph_parse_options(argc, argv, shortopts, longopts);
> > > +
> > > + opterr = 0; /* do not issue errors on helper options */
> >
> > Please use the default behavior of opterr _or_ add the case statement for
> > '?'
> > when appropriate.
> >
>
> I do think we need opterr=0, sadly: when the test or example (such as the
> classifier here) parses it options, the command line options being parsed
> still contains all options, i.e. both the option meant for the classifier
> itself, and the option meant to the "other layers", (here the helper). If
> opt_err is left to 1, the classifier's call to get_opt() will issue errors
> when hitting the helper options, even if we have a "?" in the switch, as
> far as I understand.
> So the mechanism taken here is: the caller (e.g. classifier) passes its
> list of option to the helper which builds the complete list of options by
> merging it own (helper) option list to the caller options. Then the helper
> parse this complete list (with default opterr=1), i.e. reacting to unknown
> options (not being in the merged list) and also picking up its own option
> semantic, of course. After this the caller (here classifier) parse the
> options again, picking up its own options only. but should not react on the
> helper's otpions.
> I Actually looked at removing the options from argv in the helpers, but
> this turned up to be quite tricky as well.
> parse_args() seem to be better at spitting command line option in different
> owner, but is is not POSIX. not sure we want to insert that kind of
> dependency for so little.
OK, this should not block a contribution.
> > > diff --git a/helper/include/odp/helper/linux.h
> > b/helper/include/odp/helper/linux.h
> > > index 7a6504f..a9ec90a 100644
> > > --- a/helper/include/odp/helper/linux.h
> > > +++ b/helper/include/odp/helper/linux.h
> > > @@ -25,111 +25,146 @@ extern "C" {
> > > #include <odp_api.h>
> > >
> > > #include <pthread.h>
> > > +#include <getopt.h>
> >
> > Please consider migrating CLI parsing via C library to a more appropriate
> > location.
> >
>
> Not sure I understand this: The goal is to have the helpers parsing their
> own options here. Maybe my comment above makes it clearer. otherwise please
> explain what you meant.
I see. I'll attempt to explain further.
Helper linux.[ch] files contain code for both threading and arg parsing. First,
because threading and arg parsing are separate functional pieces of code they
_should_ belong in separate files. Second, the linux.[ch] filename is
misleading. Two simple examples:
lib/thread.h
lib/thread.c
lib/thread_posix.c
lib/thread_linux.c
include/thread.h
lib/thread.c
lib/posix/thread.c
Many ways, but the point is that common platform-independent code contained
in one place, and abstraction occurs 'outward' or 'deeper' in the filename
or directory structure.
The filename and directory path corresponds to the code inside.
helper/include/odp/helper/linux.h <- CLI code goes here. Too subjective?
> >
> > > #include <sys/types.h>
> > >
> > > -/** Thread parameter for Linux pthreads and processes */
> > > +/** odpthread linux type: whether an ODP thread is a linux thread or
> > process */
> > > +typedef enum odph_odpthread_linuxtype_e {
> > > + NOT_STARTED = 0,
> > > + PROCESS,
> > > + PTHREAD
> > > +} odph_odpthread_linuxtype_t;
> >
> > Please use more descriptive identifiers in enums and consider a MAX if
> > used for iteration.
> >
>
> Any suggestion to any better names? What is meant here is: either the
> odpthread has never been started (in which case it is neither a linux
> process or a linux thread), or it is started as a linux process, or it is
> started as a linux pthread. There are not used in iteration.
> Maybe the confusion comes from what an odp thread is. I am not very keen on
> the naming, but despite its name, an ODP thread is just a concurrent
> execution task. On linux these can be implemented as pthreads or processes.
> The name "odp tasks" would in my eyes be better (as not as correlated to
> any specific implementation), but that was not accepted.
First, this enum conflates ODP thread type & runtime state. Second, if an
ODP thread is a concurrent execution task, why not name it that? I support
this naming.
ODPH_ODPTHREAD_PROCESS <- what is 'thread process'?
ODPH_ODPTHREAD_THREAD <- what is 'thread thread'?
<- don't forget 'thread zombie'
Excruciatingly dumb, yet simple, example.. _not_ to be confused with real code!
enum {
ODP_TASK_TYPE_THREAD,
ODP_TASK_TYPE_PROCESS
};
enum {
ODP_TASK_STATE_INIT,
ODP_TASK_STATE_RUNNING,
ODP_TASK_STATE_IDLE,
ODP_TASK_STATE_TERMINATED
};
struct odp_task {
union {
pid_t pid;
pthread_t thread_id;
};
int type;
int state;
};
Actually, please disregard this subjective example.
> > > -/** Linux process state information */
> > > +/** Linux odpthread state information, used both in process or thread
> > mode */
> > > typedef struct {
> > > - pid_t pid; /**< Process ID */
> > > - int cpu; /**< CPU ID */
> > > - int status; /**< Process state change status */
> > > -} odph_linux_process_t;
> > > + odph_odpthread_start_args_t start_args;
> > > + int cpu; /**< CPU ID */
> > > + int last; /**< true if last table
> > entry */
> > > + union {
> > > + struct { /* for thread implementation */
> > > + pthread_t thread; /**< Pthread ID */
> >
> > thread_id
> >
>
> Ok. will take that in v5.
>
>
> >
> > > + pthread_attr_t attr; /**< Pthread attributes */
> >
> > If threading affinity is determined at creation time, and never changes,
> > please
> > consider reducing the lifetime of this variable.
> >
>
> I am not sure what to do here: I don't really understand why this
> particular member is special here. The attribute themselves are
> created/destroyed at thread creation/destruction time.This is just a
> typedef, but indeed these structures (including all their members) are
> today staticaly allocated in tests. I have not changed that from the old
> code. I do not think this specific member is special and should be
> separated from this structure: it really belongs to it.
>
> What should be done, I think, is reducing the lifetime of the whole
> structure (allocating then at task/process creation time and freeing them
> at join time.).
> This can be addressed in another patch.
OK
> > > @@ -21,218 +21,368 @@
> > > #include <odp/helper/linux.h>
> > > #include "odph_debug.h"
> > >
> > > -static void *odp_run_start_routine(void *arg)
> > > +static struct {
> > > + int proc; /* true when process mode is required */
> > > + int thrd; /* true when thread mode is required */
> > > +} helper_options;
> >
> > This is a boolean. Please avoid the use of global state for code like this.
> >
>
> bool: bool is C99 only and the usage of int as bool seems well spread
> within the code, but there spots with bool as well, I have to admit. But
> Getopt() expects an int* as third member of its option structure... do you
> really think I should use bool and then cast it to int for Getopt()?. My
> feeling is it is as good with int straight away
> global: isn't this the best solution to avoid polluting the caller space?
> (as main belongs to another space and you want the options to be visible
> for all the helpers. Any better approach is welcome
What I mean to say is that if an ODP thread is either a thread or a process,
this state does not need to be represented by two integers. And, this state
should not be global if it is directly tied to the lifetime of an ODP thread.
Further down in your patch series, there is a change to flip between spawning
a thread or a process (every other ...) if CLI sets both thread and process
mode. This is _not_ subjective, but if it lays the foundation for further work,
then please carry on.
> >
> > > +/*
> > > + * wrapper for odpthreads, either implemented as linux threads or
> > processes.
> > > + * (in process mode, if start_routine returns NULL, the process return
> > 1.
> > > + */
> > > +static void *odpthread_run_start_routine(void *arg)
> > > {
> > > - odph_linux_thr_params_t *thr_params = arg;
> > > + int status;
> > > + int ret;
> > > + odph_odpthread_params_t *thr_params;
> > > +
> > > + odph_odpthread_start_args_t *start_args = arg;
> > > +
> > > + thr_params = &start_args->thr_params;
> > >
> > > /* ODP thread local init */
> > > if (odp_init_local(thr_params->instance, thr_params->thr_type)) {
> > > ODPH_ERR("Local init failed\n");
> > > + if (start_args->linuxtype == PROCESS)
> > > + exit(-1);
> > > return NULL;
> > > }
> > >
> > > - void *ret_ptr = thr_params->start(thr_params->arg);
> > > - int ret = odp_term_local();
> > > + /* debug: */
> > > + printf("helper: ODP %s thread started as linux %s. (pid=%d)\n",
> > > + thr_params->thr_type == ODP_THREAD_WORKER ? "worker" :
> > "control",
> > > + (start_args->linuxtype == PTHREAD) ? "pthread" : "process",
> > > + (int)getpid());
> >
> > Please use appropriate logging facilities instead of printf().
> >
>
> fixed in V5
>
>
> >
> > > + status = thr_params->start(thr_params->arg);
> > > + ret = odp_term_local();
> > >
> > > if (ret < 0)
> > > ODPH_ERR("Local term failed\n");
> > > else if (ret == 0 && odp_term_global(thr_params->instance))
> > > ODPH_ERR("Global term failed\n");
> > >
> > > - return ret_ptr;
> > > + /* for process implementation of odp threads, just return
> > status... */
> > > + if (start_args->linuxtype == PROCESS)
> > > + exit(status);
> > > +
> > > + /* threads implementation return void* pointers: cast status to
> > that. */
> >
> > Please avoid explicit cast.
> >
>
> Any other way to transform an int to a pointer which is what we need to do
> to be able to run the same code for processes (returning an int status) and
> a thread (returning a void*) ?
It is possible. But, it is faster to do the code. So, too subjective for this
review. Please disregard.
> >
> > > + return (void *)(long)status;
> > > }
> >
> > This review is 1% finished.
> >
>
> Thanks for looking at it anyway: I guess I should wait for V5...?
> I have fixed those things I said V5 on. other topics I discussed have not
> been changed, so either confirm that you understand and agree or come with
> suggestions.
> Once again many hx
>
> /Christophe
_______________________________________________
lng-odp mailing list
[email protected]
https://lists.linaro.org/mailman/listinfo/lng-odp
