On 12 May 2016 at 13:33, Savolainen, Petri (Nokia - FI/Espoo) < [email protected]> wrote:
> > > > > *From:* lng-odp [mailto:[email protected]] *On Behalf Of > *Christophe > Milard > *Sent:* Thursday, May 12, 2016 2:14 PM > *To:* Savolainen, Petri (Nokia - FI/Espoo) <[email protected]> > *Cc:* [email protected] > *Subject:* Re: [lng-odp] [PATCHv6 05/38] helpers: linux: creating > functions to handle odpthreads > > > > > > > > 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? > > > > > > There is no need for _*helper*_ to setup every second thread as process > and every second as pthread. If an application needs mixed mode, it can set > it up without a helper. We just need basic stuff working first (all > processes). More flexible setup would need more params: “create 1 process > with 3 additional pthread, and a another 2 processes with 2 additional > pthreads each, and …” > But wasn't the point of this patch series to be able to run test/examples/perf test ... in all suported modes? > > > > > > > > > > > + 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. > > > > > > The point is that any helper doxygen documentation does not say that > odph_parse_options() > must be called always - also when application itself does not have any > command line options. Also is odph_parse_options() call required if you > don’t use odph_odpthreads_create(), but only other helpers calls (e.g. > odph_ipv4_csum_valid())… > > > > Optimally, all helper options have well defined default values and > application does not need to call odph_parse_options(), if it’s happy with > the defaults. > This defeat the purpose of the series: this patch series becomes useless if tests... just select one mode, as the idea here was to have the possibility to run the test in all supported modes. Christophe. > > > -Petri > > > > > > > > > > > > > > > -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
