Re: [PATCH v6 5/8] padata: downgrade padata_do_multithreaded to serial execution for non-SMP

[Gang Li]

On 2024/2/28 05:26, Daniel Jordan wrote:
> On Thu, Feb 22, 2024 at 10:04:18PM +0800, Gang Li wrote:
> > hugetlb parallelization depends on PADATA, and PADATA depends on SMP.
> >
> > PADATA consists of two distinct functionality: One part is
> > padata_do_multithreaded which disregards order and simply divides
> > tasks into several groups for parallel execution. Hugetlb
> > init parallelization depends on padata_do_multithreaded.
> >
> > The other part is composed of a set of APIs that, while handling data in
> > an out-of-order parallel manner, can eventually return the data with
> > ordered sequence. Currently Only `crypto/pcrypt.c` use them.
> >
> > All users of PADATA of non-SMP case currently only use
> > padata_do_multithreaded. It is easy to implement a serial one in
> > include/linux/padata.h. And it is not necessary to implement another
> > functionality unless the only user of crypto/pcrypt.c does not depend on
> > SMP in the future.
> >
> > Signed-off-by: Gang Li <ligang.bdlg@xxxxxxxxxxxxx>
> > Tested-by: Paul E. McKenney <paulmck@xxxxxxxxxx>
> > ---
> >   include/linux/padata.h | 12 ++++++++----
> >   1 file changed, 8 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/linux/padata.h b/include/linux/padata.h
> > index 8f418711351bc..0146daf344306 100644
> > --- a/include/linux/padata.h
> > +++ b/include/linux/padata.h
> > @@ -180,10 +180,6 @@ struct padata_instance {
> >   
> >   #ifdef CONFIG_PADATA
> >   extern void __init padata_init(void);
> > -#else
> > -static inline void __init padata_init(void) {}
> > -#endif
> > -
> >   extern struct padata_instance *padata_alloc(const char *name);
> >   extern void padata_free(struct padata_instance *pinst);
> >   extern struct padata_shell *padata_alloc_shell(struct padata_instance *pinst);
> > @@ -194,4 +190,12 @@ extern void padata_do_serial(struct padata_priv *padata);
> >   extern void __init padata_do_multithreaded(struct padata_mt_job *job);
> >   extern int padata_set_cpumask(struct padata_instance *pinst, int cpumask_type,
> >   			      cpumask_var_t cpumask);
> > +#else
> > +static inline void __init padata_init(void) {}
> > +static inline void __init padata_do_multithreaded(struct padata_mt_job *job)
> > +{
>
> An early return here for zero-sized jobs is consistent with the
> CONFIG_PADATA version and avoids hugetlb_pages_alloc_boot taking a lock
> and flushing the tlb when there's no work to do.

That's reasonable, thanks!

Since it's single-threaded, the lock isn't contested, but tlb does need
to be treated with caution.

> With that,
>
> Acked-by: Daniel Jordan <daniel.m.jordan@xxxxxxxxxx>

And thanks again.

> > +	job->thread_fn(job->start, job->start + job->size, job->fn_arg);
> > +}
> > +#endif
> > +
> >   #endif
> > --
> > 2.20.1