Re: [PATCH v1 1/1] crypto/pcrypt: Do not use isolated CPUs for callback

["Leonardo Brás" <leobras@redhat.com>]

On Wed, 2022-10-05 at 09:57 -0300, Marcelo Tosatti wrote:
> On Tue, Oct 04, 2022 at 03:25:37AM -0300, Leonardo Bras wrote:
> > Currently pcrypt_aead_init_tfm() will pick callback cpus (ctx->cb_cpu)
> > from any online cpus. Later padata_reorder() will queue_work_on() the
> > chosen cb_cpu.
> > 
> > This is undesired if the chosen cb_cpu is listed as isolated (i.e. using
> > isolcpus=... kernel parameter), since the work queued will interfere with
> > the workload on the isolated cpu.
> > 
> > Make sure isolated cpus are not used for pcrypt.
> > 
> > Signed-off-by: Leonardo Bras <leobras@redhat.com>
> > ---
> >  crypto/pcrypt.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> > 
> > diff --git a/crypto/pcrypt.c b/crypto/pcrypt.c
> > index 9d10b846ccf73..9017d08c91a8d 100644
> > --- a/crypto/pcrypt.c
> > +++ b/crypto/pcrypt.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/kobject.h>
> >  #include <linux/cpu.h>
> >  #include <crypto/pcrypt.h>
> > +#include <linux/sched/isolation.h>
> >  
> >  static struct padata_instance *pencrypt;
> >  static struct padata_instance *pdecrypt;
> > @@ -175,13 +176,16 @@ static int pcrypt_aead_init_tfm(struct crypto_aead *tfm)
> >  	struct pcrypt_instance_ctx *ictx = aead_instance_ctx(inst);
> >  	struct pcrypt_aead_ctx *ctx = crypto_aead_ctx(tfm);
> >  	struct crypto_aead *cipher;
> > +	struct cpumask non_isolated;
> > +
> > +	cpumask_and(&non_isolated, cpu_online_mask, housekeeping_cpumask(HK_TYPE_DOMAIN));
> 
> Since certain systems do not use isolcpus=domain, so please use a flag
> that is setup by nohz_full=, for example HK_FLAG_MISC:
> 
> static int __init housekeeping_nohz_full_setup(char *str)
> {
>         unsigned long flags;
> 
>         flags = HK_FLAG_TICK | HK_FLAG_WQ | HK_FLAG_TIMER | HK_FLAG_RCU |
>                 HK_FLAG_MISC | HK_FLAG_KTHREAD;
> 
>         return housekeeping_setup(str, flags);
> }
> __setup("nohz_full=", housekeeping_nohz_full_setup);

Oh, sure. 
Since we are talking about WorkQueues, I think it makes sense to pick
HK_FLAG_WQ. 

> 
> Also, shouldnt you use cpumask_t ?/

Yeah, I think so. 
I was quick to choose the 'struct cpumask' because all functions would operate
in this variable type, but yeah, I think it makes sense to have this variable
being opaque here.

> 
> Looks good otherwise.
> 
> Thanks!

Thank you for reviewing! 

Leo

> 
> 
> >  
> >  	cpu_index = (unsigned int)atomic_inc_return(&ictx->tfm_count) %
> > -		    cpumask_weight(cpu_online_mask);
> > +		    cpumask_weight(&non_isolated);
> >  
> > -	ctx->cb_cpu = cpumask_first(cpu_online_mask);
> > +	ctx->cb_cpu = cpumask_first(&non_isolated);
> >  	for (cpu = 0; cpu < cpu_index; cpu++)
> > -		ctx->cb_cpu = cpumask_next(ctx->cb_cpu, cpu_online_mask);
> > +		ctx->cb_cpu = cpumask_next(ctx->cb_cpu, &non_isolated);
> >  
> >  	cipher = crypto_spawn_aead(&ictx->spawn);
> >  
> > -- 
> > 2.37.3
> > 
> > 
>