Re: [RFC PATCH v2 01/10] perf workqueue: threadpool creation and destruction

[Namhyung Kim]

On Mon, Aug 9, 2021 at 3:30 AM Riccardo Mancini <rickyman7@xxxxxxxxx> wrote:
> On Fri, 2021-08-06 at 19:24 -0700, Namhyung Kim wrote:
> > > +
> > > +/**
> > > + * threadpool__strerror - print message regarding given @err in @pool
> > > + *
> > > + * 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)
> > > +{
> > > +       char sbuf[STRERR_BUFSIZE], *emsg;
> > > +
> > > +       emsg = str_error_r(err, sbuf, sizeof(sbuf));
> > > +       return scnprintf(buf, size, "Error: %s.\n", emsg);
> > > +}
> > > +
> > > +/**
> > > + * threadpool__new_strerror - print message regarding @err_ptr
> > > + *
> > > + * Buffer size should be at least THREADPOOL_STRERR_BUFSIZE bytes.
> > > + */
> > > +int threadpool__new_strerror(struct threadpool *err_ptr, char *buf, size_t
> > > size)
> > > +{
> > > +       return threadpool__strerror(err_ptr, PTR_ERR(err_ptr), buf, size);
> > > +}
> >
> > Why two different functions?
>
> Since when new fails you don't have a err number, just an err_ptr so it's not
> very clear how to call threadpool__strerror. Therefore I made a wrapper to
> remove any ambiguity.

What do you mean by "when new fails"?

> >
> > > +
> > > +/**
> > > + * threadpool__delete - free the @pool and all its resources
> > > + */
> > > +void threadpool__delete(struct threadpool *pool)
> > > +{
> > > +       int t;
> > > +
> > > +       if (IS_ERR_OR_NULL(pool))
> > > +               return;
> > > +
> > > +       WARN_ON(pool->status != THREADPOOL_STATUS__STOPPED
> > > +               && pool->status != THREADPOOL_STATUS__ERROR);
> > > +
> > > +       for (t = 0; t < pool->nr_threads; t++)
> > > +               threadpool_entry__close_pipes(&pool->threads[t]);
> > > +
> > > +       zfree(&pool->threads);
> > > +       free(pool);
> > > +}
> > > +
> > > +/**
> > > + * threadpool__size - get number of threads in the threadpool
> > > + */
> > > +int threadpool__size(struct threadpool *pool)
> > > +{
> > > +       return pool->nr_threads;
> > > +}
> > > diff --git a/tools/perf/util/workqueue/threadpool.h
> > > b/tools/perf/util/workqueue/threadpool.h
> > > new file mode 100644
> > > index 0000000000000000..fb18aa32fb64f671
> > > --- /dev/null
> > > +++ b/tools/perf/util/workqueue/threadpool.h
> > > @@ -0,0 +1,30 @@
> > > +/* SPDX-License-Identifier: GPL-2.0 */
> > > +#ifndef __WORKQUEUE_THREADPOOL_H
> > > +#define __WORKQUEUE_THREADPOOL_H
> > > +
> > > +struct threadpool;
> > > +struct task_struct;
> >
> > You can just move the definition here.
> >
> > > +
> > > +typedef void (*task_func_t)(int tidx, struct task_struct *task);
> > > +
> > > +struct task_struct {
> > > +       task_func_t fn;
> > > +};
>
> I thought it was not allowed, since task_func_t refers to task_struct and
> viceversa.
> I will try to remove it if possible.

Oh, I missed that, sorry for the noise.

Thanks,
namhyung