Messages in this thread | | | From | Namhyung Kim <> | Date | Thu, 15 Jul 2021 16:29:59 -0700 | Subject | Re: [RFC PATCH 01/10] perf workqueue: threadpool creation and destruction |
| |
Hi Riccardo and Arnaldo,
On Wed, Jul 14, 2021 at 7:16 AM Arnaldo Carvalho de Melo <acme@kernel.org> wrote: > > Em Tue, Jul 13, 2021 at 02:11:12PM +0200, Riccardo Mancini escreveu: > > The workqueue library is made up by two components: > > - threadpool: handles the lifetime of the threads > > - workqueue: handles work distribution among the threads > > > > This first patch introduces the threadpool, starting from its creation > > and destruction functions. > > Thread management is based on the prototype from Alexey: > > https://lore.kernel.org/lkml/cover.1625227739.git.alexey.v.bayduraev@linux.intel.com/ > > > > Each thread in the threadpool executes the same function (aka task) > > with a different argument tidx. > > Threads use a pair of pipes to communicate with the main process. > > The threadpool is static (all threads will be spawned at the same time). > > Future work could include making it resizable and adding affinity support > > (as in Alexey prototype). > > > > Suggested-by: Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com> > > Signed-off-by: Riccardo Mancini <rickyman7@gmail.com> > > --- > > tools/perf/util/Build | 1 + > > tools/perf/util/workqueue/Build | 1 + > > tools/perf/util/workqueue/threadpool.c | 175 +++++++++++++++++++++++++ > > tools/perf/util/workqueue/threadpool.h | 19 +++ > > 4 files changed, 196 insertions(+) > > create mode 100644 tools/perf/util/workqueue/Build > > create mode 100644 tools/perf/util/workqueue/threadpool.c > > create mode 100644 tools/perf/util/workqueue/threadpool.h > > > > diff --git a/tools/perf/util/Build b/tools/perf/util/Build > > index 2d4fa13041789cd6..c7b09701661c869d 100644 > > --- a/tools/perf/util/Build > > +++ b/tools/perf/util/Build > > @@ -180,6 +180,7 @@ perf-$(CONFIG_LIBBABELTRACE) += data-convert-bt.o > > perf-y += data-convert-json.o > > > > perf-y += scripting-engines/ > > +perf-y += workqueue/ > > > > perf-$(CONFIG_ZLIB) += zlib.o > > perf-$(CONFIG_LZMA) += lzma.o > > diff --git a/tools/perf/util/workqueue/Build b/tools/perf/util/workqueue/Build > > new file mode 100644 > > index 0000000000000000..8b72a6cd4e2cba0d > > --- /dev/null > > +++ b/tools/perf/util/workqueue/Build > > @@ -0,0 +1 @@ > > +perf-y += threadpool.o > > diff --git a/tools/perf/util/workqueue/threadpool.c b/tools/perf/util/workqueue/threadpool.c > > new file mode 100644 > > index 0000000000000000..70c67569f956a3e2 > > --- /dev/null > > +++ b/tools/perf/util/workqueue/threadpool.c > > @@ -0,0 +1,175 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +#include <stdlib.h> > > +#include <stdio.h> > > +#include <unistd.h> > > +#include <errno.h> > > +#include <string.h> > > +#include "debug.h" > > +#include "asm/bug.h" > > +#include "threadpool.h" > > + > > +enum threadpool_status { > > + THREADPOOL_STATUS__STOPPED, /* no threads */ > > + THREADPOOL_STATUS__ERROR, /* errors */ > > + THREADPOOL_STATUS__MAX > > +}; > > + > > +struct threadpool_struct { > > Can this be just 'struct threadpool'? I think its descriptive enough: > > > + int nr_threads; /* number of threads in the pool */ > > + struct thread_struct *threads; /* array of threads in the pool */ > > + struct task_struct *current_task; /* current executing function */
Does this mean it can only have a single function to run? Why do we need it?
> > + enum threadpool_status status; /* current status of the pool */ > > +}; > > + > > +struct thread_struct { > > + int idx; /* idx of thread in pool->threads */ > > + pid_t tid; /* tid of thread */ > > + struct threadpool_struct *pool; /* parent threadpool */ > > + struct { > > + int from[2]; /* messages from thread (acks) */ > > + int to[2]; /* messages to thread (commands) */
It can be confusing if you think from the main thread. Maybe 'ack' and 'cmd' would be better.
> > + } pipes; > > +}; > > This one, since we have already a 'struct thread' in tools/perf, to > represent a PERF_RECORD_FORK, perhaps we can call it 'struct threadpool_entry'?
I think we can even use 'worker' instead of 'thread' but it requires huge renaming and conflicts so I won't insist on it strongly. :)
Thanks, Namhyung
| |