On 12 May 2016 at 12:28, Savolainen, Petri (Nokia - FI/Espoo) <
[email protected]> wrote:
>
> > +int odph_odpthreads_create(odph_odpthread_t *thread_tbl,
> > + const odp_cpumask_t *mask,
> > + const odph_odpthread_params_t *thr_params)
> > +{
> > + int i;
> > + int num;
> > + int cpu_count;
> > + int cpu;
> > +
> > + 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);
> > + return -1;
> > + }
> > +
> > + cpu = odp_cpumask_first(mask);
> > + for (i = 0; i < num; i++) {
> > + /*
> > + * Thread mode by default, or if both thread and proc mode
> > + * are required each second odpthread is a linux thread.
> > + */
>
> This is a weird logic. All threads should be one type (when you have
> boolean cmd line params for thread type). Pthread should be the default, so
> all threads are pthreads if user don't give any param. It's an error if
> user gives both.
>
>
>
> This follows a requirement from you that the linux ODP implementation
> should support a mix of both ODP threads implemented as pthread and process
> at the same time. I felt that a "--odph_mixed" option is not more clear
> than specifying both the things we want, i.e. processes and thread.
>
> Any better suggestion?
>
>
>
> You’d need more detailed information (than Boolean) for that advanced use
> case. At this point, all pthreads or all processes is sufficient for
> odp-linux. I have not required implementation of thread/process mix, but
> required that ODP API does not prevent different threading models.
>
>
>
> Create all threads by default, all processes if helper_options.proc is
> true and report error is both are true.
>
>
>
> if (helper_options.proc && helper_options.thrd)
>
> return -1;
>
>
>
> if (helper_options.proc) {
>
> odph_linux_process_create();
>
> } else {
>
> odph_linux_thread_create();
>
> }
>
>
>
>
>
Now, I am getting very confused... I thought we agreed that support for
both process mode and thread mode at the same time was needed. I actually
remember asking that specific question at some arch call and the answer
being positive: And it made sense!: If ODP threads are OS objects created
by the application directly towards the OS, how would you expect ODP to
prevent an app to mix pthreads and fork?. If we cannot prevent it, don't we
have to support it?
Shall I really remove support for the mixed mode from the patch series?
>
>
>
>
> > + if ((!helper_options.proc) ||
> > + (helper_options.proc && helper_options.thrd && (i &
> 1))) {
> > + if (odph_linux_thread_create(&thread_tbl[i],
> > + cpu,
> > + thr_params))
> > + break;
> > + } else {
> > + if (odph_linux_process_create(&thread_tbl[i],
> > + cpu,
> > + thr_params))
> > + break;
> > + }
> > +
> > + cpu = odp_cpumask_next(mask, cpu);
> > + }
> > + thread_tbl[num - 1].last = 1;
> > +
> > + return i;
> > +}
> > +
> > +/*
> > + * wait for the odpthreads termination (linux processes and threads)
> > + */
> > +int odph_odpthreads_join(odph_odpthread_t *thread_tbl)
> > +{
> > + pid_t pid;
> > + int i = 0;
> > + int terminated = 0;
> > + int status = 0; /* child process return code (!=0 is error)
> > */
> > + void *thread_ret; /* "child" thread return code (NULL is
> error) */
> > + int ret;
> > + int retval = 0;
> > +
> > + /* joins linux threads or wait for processes */
> > + do {
> > + /* pthreads: */
> > + switch (thread_tbl[i].start_args.linuxtype) {
> > + case ODPTHREAD_PTHREAD:
> > + /* Wait thread to exit */
> > + ret = pthread_join(thread_tbl[i].thread.thread_id,
> > + &thread_ret);
> > + if (ret != 0) {
> > + ODPH_ERR("Failed to join thread from cpu
> #%d\n",
> > + thread_tbl[i].cpu);
> > + retval = -1;
> > + } else {
> > + terminated++;
> > + if (thread_ret != NULL)
> > + retval = -1;
> > + }
> > + pthread_attr_destroy(&thread_tbl[i].thread.attr);
> > + break;
> > +
> > + case ODPTHREAD_PROCESS:
> > +
> > + /* processes: */
> > + pid = waitpid(thread_tbl[i].proc.pid, &status, 0);
> > +
> > + if (pid < 0) {
> > + ODPH_ERR("waitpid() failed\n");
> > + retval = -1;
> > + break;
> > + }
> > +
> > + terminated++;
> > +
> > + /* Examine the child process' termination status */
> > + if (WIFEXITED(status) &&
> > + WEXITSTATUS(status) != EXIT_SUCCESS) {
> > + ODPH_ERR("Child exit status:%d (pid:%d)\n",
> > + WEXITSTATUS(status), (int)pid);
> > + retval = -1;
> > + }
> > + if (WIFSIGNALED(status)) {
> > + int signo = WTERMSIG(status);
> > +
> > + ODPH_ERR("Child term signo:%d - %s
> (pid:%d)\n",
> > + signo, strsignal(signo),
> (int)pid);
> > + retval = -1;
> > + }
> > + break;
> > +
> > + case ODPTHREAD_NOT_STARTED:
> > + ODPH_DBG("No join done on not started
> ODPthread.\n");
> > + break;
> > + default:
> > + ODPH_DBG("Invalid case statement value!\n");
> > + break;
> > + }
> > +
> > + } while (!thread_tbl[i++].last);
> > +
> > + return (retval < 0) ? retval : terminated;
> > +}
> > +
> > +/*
> > * Parse command line options to extract options affecting helpers.
> > */
> > int odph_parse_options(int argc, char *argv[])
> > @@ -247,9 +508,15 @@ int odph_parse_options(int argc, char *argv[])
> >
> > static struct option long_options[] = {
> > /* These options set a flag. */
> > + {"odph_proc", no_argument, &helper_options.proc, 1},
> > + {"odph_thread", no_argument, &helper_options.thrd, 1},
> > {0, 0, 0, 0}
> > };
> >
> > + /* defaults: */
> > + helper_options.proc = false;
> > + helper_options.thrd = false;
>
> Pthread should be the default option. We should not always require a cmd
> line param from user (if no param ==> pthread).
>
>
>
> We don't!. This structure reflects the options that getopts finds on the
> command line: Before parsing the command line, no options have been found!.
> The default IS pthread when no options are given.
>
>
>
> Christophe.
>
>
>
> Code would be more readable if global variables and their default values
> indicate what happens by default. Optimally, a number of threads could be
> created even without calling the parse function (with empty argv). I think
> the helper documentation does not require that odph_parse_options() is
> called before calling odph_odpthreads_create(), or does it (in some of the
> patches).
>
>
>
> -Petri
>
I think it is clear that option parsing should happen at the very
beginning... I guess it is normal behaviour: one doesn't want git to start
parsing the "--dry-run" option once half of the job is done... :-)
I don't expect the helper doc to say anything on this matter as this option
parsing feature is new in the patch series.
Both options are set to false here because:
--odph_proc means we want process mode.
--odph_thread means we want thread mode.
--odph_proc --odph_thread means we want both (mixed mode)
If I had helper_options.proc set to true by default, --odph_thread would
always imply mixed mode.
As mentionned the helper_options struct tells which option were found on
the command line. I can add a comment to clarify that.
If the mixed mode should be suppressed from the patch series, then I can
set helper_options.proc to anything.
Christophe.
>
>
>
>
>
>
>
>
> -Petri
>
>
>
>
> > +
> > while (1) {
> > /* getopt_long stores the option index here. */
> > int option_index = 0;
> > --
> > 2.5.0
> >
> > _______________________________________________
> > lng-odp mailing list
> > [email protected]
> > https://lists.linaro.org/mailman/listinfo/lng-odp
>
>
>
_______________________________________________
lng-odp mailing list
[email protected]
https://lists.linaro.org/mailman/listinfo/lng-odp
