Messages in this thread | | | Subject | Re: [RFC PATCH v2 03/10] perf workqueue: add threadpool start and stop functions | From | Riccardo Mancini <> | Date | Mon, 09 Aug 2021 12:35:16 +0200 |
| |
Hi,
On Fri, 2021-08-06 at 19:43 -0700, Namhyung Kim wrote: > On Fri, Jul 30, 2021 at 8:34 AM Riccardo Mancini <rickyman7@gmail.com> wrote: > > > > This patch adds the start and stop functions, alongside the thread > > function. > > Each thread will run until a stop signal is received. > > Furthermore, start and stop are added to the test. > > > > Thread management is based on the prototype from Alexey: > > https://lore.kernel.org/lkml/cover.1625227739.git.alexey.v.bayduraev@linux.intel.com/ > > > > Suggested-by: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com> > > Signed-off-by: Riccardo Mancini <rickyman7@gmail.com> > > --- > > [SNIP] > > @@ -93,6 +134,130 @@ static void threadpool_entry__close_pipes(struct > > threadpool_entry *thread) > > } > > } > > > > +/** > > + * threadpool__wait_thread - receive ack from thread > > + * > > + * NB: call only from main thread! > > + */ > > +static int threadpool__wait_thread(struct threadpool_entry *thread) > > If you wanted to differentiate APIs for main thread, I think it's better > to pass the pool struct (according to the name) and the index like: > > int threadpool__wait_thread(struct threadpool *pool, int idx) > > Then it can get a pointer to the entry easily.
Agree. I also need to more clearly separate those APIs too.
> > > +{ > > + int res; > > + enum threadpool_msg msg = THREADPOOL_MSG__UNDEFINED; > > + > > + res = readn(thread->pipes.ack[0], &msg, sizeof(msg)); > > + if (res < 0) { > > + pr_debug2("threadpool: failed to recv msg from tid=%d: > > %s\n", > > + thread->tid, strerror(errno)); > > + return -THREADPOOL_ERROR__READPIPE; > > + } > > + if (msg != THREADPOOL_MSG__ACK) { > > + pr_debug2("threadpool: received unexpected msg from tid=%d: > > %s\n", > > + thread->tid, threadpool_msg_tags[msg]); > > + return -THREADPOOL_ERROR__INVALIDMSG; > > + } > > + > > + pr_debug2("threadpool: received ack from tid=%d\n", thread->tid); > > + > > + return 0; > > +} > > + > > +/** > > + * threadpool__terminate_thread - send stop signal to thread and wait for > > ack > > + * > > + * NB: call only from main thread! > > + */ > > +static int threadpool__terminate_thread(struct threadpool_entry *thread) > > Ditto.
ack
> > > +{ > > + int res; > > + enum threadpool_msg msg = THREADPOOL_MSG__STOP; > > + > > + res = writen(thread->pipes.cmd[1], &msg, sizeof(msg)); > > + if (res < 0) { > > + pr_debug2("threadpool: error sending stop msg to tid=%d: > > %s\n", > > + thread->tid, strerror(errno)); > > + return -THREADPOOL_ERROR__WRITEPIPE; > > + } > > + > > + return threadpool__wait_thread(thread); > > +} > > + > > [SNIP] > > @@ -161,12 +326,30 @@ struct threadpool *threadpool__new(int n_threads) > > * > > * Buffer size should be at least THREADPOOL_STRERR_BUFSIZE bytes. > > */ > > -int threadpool__strerror(struct threadpool *pool __maybe_unused, int err, > > char *buf, size_t size) > > +int threadpool__strerror(struct threadpool *pool, int err, char *buf, > > size_t size) > > { > > char sbuf[STRERR_BUFSIZE], *emsg; > > + const char *status_str, *errno_str; > > > > - emsg = str_error_r(err, sbuf, sizeof(sbuf)); > > - return scnprintf(buf, size, "Error: %s.\n", emsg); > > + status_str = IS_ERR_OR_NULL(pool) ? "error" : > > threadpool_status_tags[pool->status]; > > + > > + switch (err) { > > + case -THREADPOOL_ERROR__SIGPROCMASK: > > + case -THREADPOOL_ERROR__READPIPE: > > + case -THREADPOOL_ERROR__WRITEPIPE: > > + emsg = str_error_r(errno, sbuf, sizeof(sbuf)); > > + errno_str = threadpool_errno_str[-err- > > THREADPOOL_ERROR__OFFSET]; > > + return scnprintf(buf, size, "%s: %s.\n", errno_str, emsg); > > + case -THREADPOOL_ERROR__INVALIDMSG: > > + errno_str = threadpool_errno_str[-err- > > THREADPOOL_ERROR__OFFSET]; > > + return scnprintf(buf, size, "%s.\n", errno_str); > > + case -THREADPOOL_ERROR__NOTALLOWED: > > + return scnprintf(buf, size, "%s (%s).\n", > > + threadpool_errno_str[-err], status_str); > > s/-err/-err-THREADPOOL_ERROR__OFFSET/ ? > > It'd be nice if you calculate the index once.
agreed
> > > + default: > > + emsg = str_error_r(err, sbuf, sizeof(sbuf)); > > I'm confused whether the 'err' is negative or positive?
It's negative, so it needs to be inverted in this case.
Thanks, Riccardo
> > Thanks, > Namhyung > > > > + return scnprintf(buf, size, "Error: %s", emsg); > > + } > > } > >
| |