> +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();
}
> + 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
-Petri
> +
> while (1) {
> /* getopt_long stores the option index here. */
> int option_index = 0;
> --
> 2.5.0
>
> _______________________________________________
> lng-odp mailing list
> lng-odp@lists.linaro.org<mailto:lng-odp@lists.linaro.org>
> https://lists.linaro.org/mailman/listinfo/lng-odp
_______________________________________________
lng-odp mailing list
lng-odp@lists.linaro.org
https://lists.linaro.org/mailman/listinfo/lng-odp
