Re: [PATCH v4 10/24] crypto: x86/poly - limit FPU preemption

From: Peter Lafreniere <peter@n8pjl.ca>
To: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "Elliott, Robert (Servers)" <elliott@hpe.com>,
	"Jason A. Donenfeld" <Jason@zx2c4.com>,
	"davem@davemloft.net" <davem@davemloft.net>,
	"tim.c.chen@linux.intel.com" <tim.c.chen@linux.intel.com>,
	"ap420073@gmail.com" <ap420073@gmail.com>,
	"ardb@kernel.org" <ardb@kernel.org>,
	"David.Laight@aculab.com" <David.Laight@aculab.com>,
	"ebiggers@kernel.org" <ebiggers@kernel.org>,
	"linux-crypto@vger.kernel.org" <linux-crypto@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v4 10/24] crypto: x86/poly - limit FPU preemption
Date: Tue, 06 Dec 2022 14:03:31 +0000	[thread overview]
Message-ID: <5TnEjVPNm7Eyw-GH7C0LeJJvgRSpOLb2NUshnG407s3TGTXL1lq4RpsoAMTpVGKWk7tVxDI5f2G9aH6lDbATR6QqXXkE7q54o7TUzO91ibI=@n8pjl.ca> (raw)
In-Reply-To: <Y47El6TRitHm7Xz9@gondor.apana.org.au>

> > > BTW, just a minor nit but you can delete the cond_resched() call
> > > because kernel_fpu_end()/preempt_enable() will do it anyway.
> > 
> > That happens under
> > CONFIG_PREEMPTION=y
> > (from include/Linux/preempt.h and arch/x86/include/asm/preempt.h)
> > 
> > Is calling cond_resched() still helpful if that is not the configuration?
> 
> 
> Perhaps, but then again perhaps if preemption is off, maybe we
> shouldn't even bother with the 4K split. Were the initial
> warnings with or without preemption?
> 
> Personally I don't really care since I always use preemption.
> 
> The PREEMPT Kconfigs do provide a bit of nuance with the split
> between PREEMPT_NONE vs. PREEMPT_VOLUNTARY. But perhaps that is
> just overkill for our situation.

I was thinking about this a few days ago, and my 2¢ is that it's 
probably best to not preempt the kernel in the middle of a crypto 
operation under PREEMPT_VOLUNTARY. We're already not preempting during 
these operations, and there haven't been complaints of excessive latency 
because of these crypto operations.

If we skip the kernel_fpu_{begin,end} pair when not under 
CONFIG_PREEMPT, we'll save a significant cycle count that is wasted 
currently. See Elliot Robert's numbers on conditional begin/end in sha 
to see the benefits of not saving/restoring unnecessarily: "10% of the 
CPU cycles spent making the [kernel_fpu_{begin,end}] calls".

> I'll leave it to you to decide :)

One extra thought: commit 827ee47: "crypto: x86 - add some helper macros 
for ECB and CBC modes" makes a mention of fpu save/restore being done 
lazily. I don't know the details, so would that change this discussion?

Thanks for listening,

Peter Lafreniere <peter@n8pjl.ca>