LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Namhyung Kim <namhyung@kernel.org>
To: Riccardo Mancini <rickyman7@gmail.com>
Cc: Arnaldo Carvalho de Melo <acme@kernel.org>,
	Ian Rogers <irogers@google.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Ingo Molnar <mingo@redhat.com>,
	Mark Rutland <mark.rutland@arm.com>, Jiri Olsa <jolsa@redhat.com>,
	linux-kernel <linux-kernel@vger.kernel.org>,
	linux-perf-users <linux-perf-users@vger.kernel.org>,
	Alexey Bayduraev <alexey.v.bayduraev@linux.intel.com>
Subject: Re: [RFC PATCH 01/10] perf workqueue: threadpool creation and destruction
Date: Mon, 19 Jul 2021 12:39:33 -0700	[thread overview]
Message-ID: <CAM9d7cggQkX=LpaOu+zJCJfPqsj0RUvECTDiU2dJpW-XqY5D8g@mail.gmail.com> (raw)
In-Reply-To: <b7aa58d750f5919f30877fe2f9878698f3cddbc4.camel@gmail.com>

On Fri, Jul 16, 2021 at 6:36 AM Riccardo Mancini <rickyman7@gmail.com> wrote:
>
> Hi Namhyung,
> thanks for the review.
>
> On Thu, 2021-07-15 at 16:29 -0700, Namhyung Kim wrote:
> > 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
> <SNIP>
> > > > +
> > > > +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?
>
> Yes.
>
> > Why do we need it?
>
> My idea is to separate the workqueue from the actual implementation of the
> threads. This way, when the function executing on the threadpool ends, the
> threads are kept alive to execute new work.
> By adding this additional layer of abstraction, we can achieve more flexibility.
> For example, the use-case I have in mind is to recycle the same threadpool for
> both Alexey's threaded trace and the workqueue.
> I don't think this could be easily achieved with just the workqueue since the
> perf-record threads are not just a task that needs to be executed by they have
> specific affinities to be respected.
>
> What are your thoughts?

I'm fine with the separation of work(queue) and thread-pool.

I thought the backing thread-pool is general and can handle
multiple works at the same time.

The work queue should keep track of works it submitted
and their status.  We can have multiple workqueues
sharing a single thread pool.


>
> >
> >
> > > > +     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.
>
> Agreed.
>
> >
> >
> > > > +     } 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.  :)
>
> Also, worker internally conflicts with the workqueue's worker, which runs on a
> (threadpool-)thread.
> Another name I had in mind is pool_thread to prevent having too many 'thread' in
> the name, but it might be confusing.
> I think threadpool_entry is fine.
>
> I have another question.
> In general, when should I use zfree instead of free?

I think the zfree is generally preferable to free.
Especially if the pointer can be accessed after free.

Thanks,
Namhyung

  reply	other threads:[~2021-07-19 20:18 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-13 12:11 [RFC PATCH 00/10] perf: add workqueue library and use it in synthetic-events Riccardo Mancini
2021-07-13 12:11 ` [RFC PATCH 01/10] perf workqueue: threadpool creation and destruction Riccardo Mancini
2021-07-14 14:16   ` Arnaldo Carvalho de Melo
2021-07-15 16:31     ` Riccardo Mancini
2021-07-15 20:48       ` Arnaldo Carvalho de Melo
2021-07-15 23:29     ` Namhyung Kim
2021-07-16 13:36       ` Riccardo Mancini
2021-07-19 19:39         ` Namhyung Kim [this message]
2021-07-13 12:11 ` [RFC PATCH 02/10] perf tests: add test for workqueue Riccardo Mancini
2021-07-14 15:10   ` Arnaldo Carvalho de Melo
2021-07-15 16:33     ` Riccardo Mancini
2021-07-13 12:11 ` [RFC PATCH 03/10] perf workqueue: add threadpool start and stop functions Riccardo Mancini
2021-07-14 15:15   ` Arnaldo Carvalho de Melo
2021-07-15 16:42     ` Riccardo Mancini
2021-07-15 20:43       ` Arnaldo Carvalho de Melo
2021-07-15 23:48   ` Namhyung Kim
2021-07-16 13:53     ` Riccardo Mancini
2021-07-16 16:29       ` Arnaldo Carvalho de Melo
2021-07-13 12:11 ` [RFC PATCH 04/10] perf workqueue: add threadpool execute and wait functions Riccardo Mancini
2021-07-15 23:56   ` Namhyung Kim
2021-07-16 13:55     ` Riccardo Mancini
2021-07-13 12:11 ` [RFC PATCH 05/10] perf workqueue: add sparse annotation header Riccardo Mancini
2021-07-13 12:11 ` [RFC PATCH 06/10] perf workqueue: introduce workqueue struct Riccardo Mancini
2021-07-14 15:22   ` Arnaldo Carvalho de Melo
2021-07-15 16:49     ` Riccardo Mancini
2021-07-15 20:47       ` Arnaldo Carvalho de Melo
2021-07-13 12:11 ` [RFC PATCH 07/10] perf workqueue: implement worker thread and management Riccardo Mancini
2021-07-13 12:11 ` [RFC PATCH 08/10] perf workqueue: add queue_work and flush_workqueue functions Riccardo Mancini
2021-07-13 12:11 ` [RFC PATCH 09/10] perf workqueue: add utility to execute a for loop in parallel Riccardo Mancini
2021-07-13 12:11 ` [RFC PATCH 10/10] perf synthetic-events: use workqueue parallel_for Riccardo Mancini
2021-07-13 19:14 ` [RFC PATCH 00/10] perf: add workqueue library and use it in synthetic-events Arnaldo Carvalho de Melo
2021-07-19 21:13 ` Jiri Olsa
2021-07-22 16:15   ` Riccardo Mancini

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAM9d7cggQkX=LpaOu+zJCJfPqsj0RUvECTDiU2dJpW-XqY5D8g@mail.gmail.com' \
    --to=namhyung@kernel.org \
    --cc=acme@kernel.org \
    --cc=alexey.v.bayduraev@linux.intel.com \
    --cc=irogers@google.com \
    --cc=jolsa@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-perf-users@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=mingo@redhat.com \
    --cc=peterz@infradead.org \
    --cc=rickyman7@gmail.com \
    --subject='Re: [RFC PATCH 01/10] perf workqueue: threadpool creation and destruction' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).