Received-SPF: pass (google.com: domain of linux-crypto-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <3d6d47035f8897542a4786eef5a6b8885f4caaf0.camel@redhat.com>
Subject: Re: [PATCH v1 1/1] crypto/pcrypt: Do not use isolated CPUs for
 callback
From:   Leonardo =?ISO-8859-1?Q?Br=E1s?= <leobras@redhat.com>
To:     Marcelo Tosatti <mtosatti@redhat.com>
Cc:     Steffen Klassert <steffen.klassert@secunet.com>,
        Herbert Xu <herbert@gondor.apana.org.au>,
        "David S. Miller" <davem@davemloft.net>,
        linux-crypto@vger.kernel.org, linux-kernel@vger.kernel.org
Date:   Tue, 11 Oct 2022 15:20:39 -0300
In-Reply-To: <b23b08274ccff99fb341ea272e968f72c2e289ce.camel@redhat.com>
References: <20221004062536.280712-1-leobras@redhat.com>
         <Yz1/TVUV+KnLvodg@fuller.cnet>
         <b23b08274ccff99fb341ea272e968f72c2e289ce.camel@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
User-Agent: Evolution 3.44.4 
MIME-Version: 1.0
Precedence: bulk

On Fri, 2022-10-07 at 18:42 -0300, Leonardo Br=C3=A1s wrote:
> 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.
> > >=20
> > > This is undesired if the chosen cb_cpu is listed as isolated (i.e. us=
ing
> > > isolcpus=3D... kernel parameter), since the work queued will interfer=
e with
> > > the workload on the isolated cpu.
> > >=20
> > > Make sure isolated cpus are not used for pcrypt.
> > >=20
> > > Signed-off-by: Leonardo Bras <leobras@redhat.com>
> > > ---
> > >  crypto/pcrypt.c | 10 +++++++---
> > >  1 file changed, 7 insertions(+), 3 deletions(-)
> > >=20
> > > 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>
> > > =20
> > >  static struct padata_instance *pencrypt;
> > >  static struct padata_instance *pdecrypt;
> > > @@ -175,13 +176,16 @@ static int pcrypt_aead_init_tfm(struct crypto_a=
ead *tfm)
> > >  	struct pcrypt_instance_ctx *ictx =3D aead_instance_ctx(inst);
> > >  	struct pcrypt_aead_ctx *ctx =3D crypto_aead_ctx(tfm);
> > >  	struct crypto_aead *cipher;
> > > +	struct cpumask non_isolated;
> > > +
> > > +	cpumask_and(&non_isolated, cpu_online_mask, housekeeping_cpumask(HK=
_TYPE_DOMAIN));
> >=20
> > Since certain systems do not use isolcpus=3Ddomain, so please use a fla=
g
> > that is setup by nohz_full=3D, for example HK_FLAG_MISC:
> >=20
> > static int __init housekeeping_nohz_full_setup(char *str)
> > {
> >         unsigned long flags;
> >=20
> >         flags =3D HK_FLAG_TICK | HK_FLAG_WQ | HK_FLAG_TIMER | HK_FLAG_R=
CU |
> >                 HK_FLAG_MISC | HK_FLAG_KTHREAD;
> >=20
> >         return housekeeping_setup(str, flags);
> > }
> > __setup("nohz_full=3D", housekeeping_nohz_full_setup);
>=20
> Oh, sure.=C2=A0
> Since we are talking about WorkQueues, I think it makes sense to pick
> HK_FLAG_WQ.=20
>=20
> >=20
> > Also, shouldnt you use cpumask_t ?/
>=20
> Yeah, I think so.=C2=A0
> I was quick to choose the 'struct cpumask' because all functions would op=
erate
> in this variable type, but yeah, I think it makes sense to have this vari=
able
> being opaque here.

In fact, it seems neither 'struct cpumask' nor 'cpumask_t' are recommended =
to be
used allocated in the stack, due to the large size it can get (up to 1kB).=
=C2=A0

At include/linux/cpumask.h we have:
'cpumask_var_t: struct cpumask for stack usage'
which should work better at least for init functions like this.

In other cases, I see 'static cpumask_t' being used to avoid the allocation
overhead, but it's probably due to the functions being called in very speci=
fic
scenarios. It could mean trouble if multiple cpus try to use it at once.

What do you recommend on it?

Best regards,
Leo

>=20
> >=20
> > Looks good otherwise.
> >=20
> > Thanks!
>=20
> Thank you for reviewing!=20
>=20
> Leo
>=20
> >=20
> >=20
> > > =20
> > >  	cpu_index =3D (unsigned int)atomic_inc_return(&ictx->tfm_count) %
> > > -		    cpumask_weight(cpu_online_mask);
> > > +		    cpumask_weight(&non_isolated);
> > > =20
> > > -	ctx->cb_cpu =3D cpumask_first(cpu_online_mask);
> > > +	ctx->cb_cpu =3D cpumask_first(&non_isolated);
> > >  	for (cpu =3D 0; cpu < cpu_index; cpu++)
> > > -		ctx->cb_cpu =3D cpumask_next(ctx->cb_cpu, cpu_online_mask);
> > > +		ctx->cb_cpu =3D cpumask_next(ctx->cb_cpu, &non_isolated);
> > > =20
> > >  	cipher =3D crypto_spawn_aead(&ictx->spawn);
> > > =20
> > > --=20
> > > 2.37.3
> > >=20
> > >=20
> >=20
>=20