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