Hi Brian
I have commented below, but it looks we need to talk. would you have time
after the linaro sync call today, for a HO?
On 29 April 2016 at 23:49, Brian Brooks <brian.bro...@linaro.org> wrote:
> On 04/29 13:29:03, Christophe Milard wrote:
> > 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.
>
> 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?
>
hmm,
To start with, I will keep using the term "ODP thread", even if I don't
like it more than you, because that is the accepted term here. It is
important we talk the same language, all of us. You are welcome to alias it
to "ODPtask" on your head, or argue for a name change (which I alreay
tried !), but we should be using the same accepted term everywhere, and
currently, it is "ODPthread".
I think our disagreement mostly comes from the fact that "helpers" is not a
very well defined thing. And interestingly, your comments match the
feelings I had when I started... There must be some good in them :-)
ODP is a strange thing when it comes to "ODPthreads": The problem being
that ODP tries not to redefine what the OS provides, but somehow needs to:
ODP provides mechanism to block odpthreads (e.g. barriers, scheduler), to
wake them up (scheduler, on event reception), but ODP tries not to define
how an ODP thread is implemented (which is supposed to be an OS matter).
This situation gives me a head-hake too! I have personally the feeling that
the odpthread creation will eventually have to be accepted as fully part of
ODP and that ODP will do whatever is needed towards the underlying OS. But
today the odpthread creation ends up either directly in the application
(calling the OS) or in the helpers (for those things most application will
keep doing).
So the distinction thread vs process makes sense within linux. For the
helper (for the time being), we are just creating ODP threads, so I do not
think this should be splitted into 2 files, even if the odpthread creation
itself may end-up calling two different OS system calls at the end.
This patch series tries to address a single issue: We have claimed that ODP
threads, on linux, could be either linux threads or linux processes. But
the process case has mostly been ignored and is very broken. Before this
patch series, the helper provided 2 sets of functions: one for creating
odpthreads as linux threads, and the other for creating the odpthreads as
linux processes. The latter set was in practical not used. so the "process
mode" that we claim to support was not tested.
This patch series just factorizes these 2 sets in one, letting the helper
decide whether odpthreads should be linux processes or linux threads (based
on options), hence giving the chance to all test/examples that only
previously ran odpthreads as linux threads to now run as linux threads as
well. (and mostly fails).
I personnaly feel that this will change in the future and that ODP threads
creation might well become a part of ODP.
I think we need a HO call :-)
Separating the command line parsing from the rest of the help makes more
sense to me. But as the only option being parsed are the two being
introduced here, I am not sure such a split makes sense now.
Any other opinions?
>
> > >
> > > > #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.
>
Yes and no: your are right in the sense that the name conflates 2 things:
the type,and the thread existance (not its state). Maybe it should be:
NONE, PROCESS and THREADS, "NONE" being for the odpthreads not being
started, and that are hence neither processes nor threads. Note that those
odpthreads not being started, are not going to be started either: This
just tells that this entry in the table corresponds to a odpthread whose
creation failed, and therefore no attempt to join/wait for the underlying
thread/process should be made. We are not talking about the usual process
state (running/wait/... state)
> ODPH_ODPTHREAD_PROCESS <- what is 'thread process'?
> ODPH_ODPTHREAD_THREAD <- what is 'thread thread'?
> <- don't forget 'thread zombie'
>
The ODPH prefix should be reserved for things belonging to the helper
interface. You will argue with reason that these are defined in the
interface definition file (linux.h). I have kept the same structure as
previously in this patch, keeping the table struct definition in "linux.h".
I think the table definition should actually be opaque to the user and
defined in linux.c (as it is local to it). I actually have a patch doing
that ready. So rather than prefixing private things with ODPH, they are
moved in linux.c.
But as said, even if this code looks new, it is actually a merge of two of
sructures (one for linux and one for threads) defined just above in the
same file.
I can howerver can it to ODPTHREAD_PROCESS and ODPTHREAD_PROCESS if yu
prefer.
>
> 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
> };
>
No: these are states for the OS. (and possibly for ODP in some future ?).
but not for the helpers.
> 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.
>
I agree this 2 ints really carries bool info, but as said getopt() expects
int*, not bool*.
And this structure should live as long as the application lives (its
lifetime is not connected to the odpthread lifetime): These flags tells
whether any odpthread beeing started -at any time- should be started as
linux process thread/process)
> 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.
>
Not sure what you mean here: we need to talk ! :-)
>
> > >
> > > > +/*
> > > > + * 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
>
/Christophe.
_______________________________________________
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp
