From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753234AbdK3PaB convert rfc822-to-8bit (ORCPT ); Thu, 30 Nov 2017 10:30:01 -0500 Received: from mail.kernel.org ([198.145.29.99]:36688 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752481AbdK3P34 (ORCPT ); Thu, 30 Nov 2017 10:29:56 -0500 DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5039421483 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=goodmis.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=rostedt@goodmis.org Date: Thu, 30 Nov 2017 10:29:48 -0500 From: Steven Rostedt To: Sebastian Andrzej Siewior Cc: linux-rt-users@vger.kernel.org, linux-kernel@vger.kernel.org, tglx@linutronix.de, Peter Zijlstra Subject: Re: [PATCH RT] crypto: limit more FPU-enabled sections Message-ID: <20171130102948.22d451cd@gandalf.local.home> In-Reply-To: <20171130142216.GB12606@linutronix.de> References: <20171130142216.GB12606@linutronix.de> X-Mailer: Claws Mail 3.14.0 (GTK+ 2.24.31; x86_64-pc-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 30 Nov 2017 15:22:17 +0100 Sebastian Andrzej Siewior wrote: > Those crypto drivers use SSE/AVX/… for their crypto work and in order to > do so in kernel they need to enable the "FPU" in kernel mode which > disables preemption. > There are two problems with the way they are used: > - the while loop which processes X bytes may create latency spikes and > should be avoided or limited. > - the cipher-walk-next part may allocate/free memory and may use > kmap_atomic(). > > The whole kernel_fpu_begin()/end() processing isn't probably that cheap. > It most likely makes sense to prcess as much of those as possible in one s/prcess/process/ > go. The new *_fpu_sched_rt() shedules only if a RT task is pending. > > Probably we should meassure the performance those ciphers in pure SW > mode and with this optimisations to see if it makes sense to keep them > for RT. > > Signed-off-by: Sebastian Andrzej Siewior > +static void camellia_fpu_end_rt(struct crypt_priv *ctx) > +{ > +#if CONFIG_PREEMPT_RT_FULL > + bool fpu_enabled = ctx->fpu_enabled; > + > + if (!fpu_enabled) > + return; > + camellia_fpu_end(fpu_enabled); > + ctx->fpu_enabled = false; > +#endif > +} > + > +static void camellia_fpu_sched_rt(struct crypt_priv *ctx) > +{ > +#if CONFIG_PREEMPT_RT_FULL > + bool fpu_enabled = ctx->fpu_enabled; > + > + if (!fpu_enabled || !tif_need_resched_now()) > + return; > + camellia_fpu_end(fpu_enabled); > + kernel_fpu_end(); > + /* schedule due to preemptible */ > + kernel_fpu_begin(); > +#endif > +} > + > +static void camellia_fpu_end_rt(struct crypt_priv *ctx) > +{ > +#if CONFIG_PREEMPT_RT_FULL > + bool fpu_enabled = ctx->fpu_enabled; > + > + if (!fpu_enabled) > + return; > + camellia_fpu_end(fpu_enabled); > + ctx->fpu_enabled = false; > +#endif > +} > + > +static void camellia_fpu_sched_rt(struct crypt_priv *ctx) > +{ > +#if CONFIG_PREEMPT_RT_FULL > + bool fpu_enabled = ctx->fpu_enabled; > + > + if (!fpu_enabled || !tif_need_resched_now()) > + return; > + camellia_fpu_end(fpu_enabled); I haven't looked deeply, but why does this call the camellia_fpu_end() but other *_fpu_sched_rt() do not call the equivalent? > + kernel_fpu_end(); > + /* schedule due to preemptible */ > + kernel_fpu_begin(); > +#endif > +} > + These are duplicate functions. Shouldn't they go into a header file? Also, they are very similar: static inline void camellia_fpu_end(bool fpu_enabled) { glue_fpu_end(fpu_enabled); } static inline void cast6_fpu_end(bool fpu_enabled) { glue_fpu_end(fpu_enabled); } static inline void serpent_fpu_end(bool fpu_enabled) { glue_fpu_end(fpu_enabled); } static inline void twofish_fpu_end(bool fpu_enabled) { glue_fpu_end(fpu_enabled); } -- Steve > > +static void cast6_fpu_end_rt(struct crypt_priv *ctx) > +{ > +#if CONFIG_PREEMPT_RT_FULL > + bool fpu_enabled = ctx->fpu_enabled; > + > + if (!fpu_enabled) > + return; > + cast6_fpu_end(fpu_enabled); > + ctx->fpu_enabled = false; > +#endif > +} > > +static void serpent_fpu_end_rt(struct crypt_priv *ctx) > +{ > +#if CONFIG_PREEMPT_RT_FULL > + bool fpu_enabled = ctx->fpu_enabled; > + > + if (!fpu_enabled) > + return; > + serpent_fpu_end(fpu_enabled); > + ctx->fpu_enabled = false; > +#endif > +} > + > +static void serpent_fpu_sched_rt(struct crypt_priv *ctx) > +{ > +#if CONFIG_PREEMPT_RT_FULL > + bool fpu_enabled = ctx->fpu_enabled; > + > + if (!fpu_enabled || !tif_need_resched_now()) > + return; > + kernel_fpu_end(); > + /* schedule due to preemptible */ > + kernel_fpu_begin(); > +#endif > +} > + > static void encrypt_callback(void *priv, u8 *srcdst, unsigned int nbytes) > > +static void serpent_fpu_end_rt(struct crypt_priv *ctx) > +{ > +#if CONFIG_PREEMPT_RT_FULL > + bool fpu_enabled = ctx->fpu_enabled; > + > + if (!fpu_enabled) > + return; > + serpent_fpu_end(fpu_enabled); > + ctx->fpu_enabled = false; > +#endif > +} > + > > diff --git a/arch/x86/crypto/serpent_sse2_glue.c b/arch/x86/crypto/serpent_sse2_glue.c > index ac0e831943f5..66fd2a51836f 100644 > --- a/arch/x86/crypto/serpent_sse2_glue.c > +++ b/arch/x86/crypto/serpent_sse2_glue.c > @@ -187,16 +187,28 @@ struct crypt_priv { > bool fpu_enabled; > }; > > +static void serpent_fpu_end_rt(struct crypt_priv *ctx) > +{ > +#if CONFIG_PREEMPT_RT_FULL > + bool fpu_enabled = ctx->fpu_enabled; > + > + if (!fpu_enabled) > + return; > + serpent_fpu_end(fpu_enabled); > + ctx->fpu_enabled = false; > +#endif > +} > + > > +static void twofish_fpu_end_rt(struct crypt_priv *ctx) > +{ > +#if CONFIG_PREEMPT_RT_FULL > + bool fpu_enabled = ctx->fpu_enabled; > + > + if (!fpu_enabled) > + return; > + twofish_fpu_end(fpu_enabled); > + ctx->fpu_enabled = false; > +#endif > +} > + > +static void twofish_fpu_sched_rt(struct crypt_priv *ctx) > +{ > +#if CONFIG_PREEMPT_RT_FULL > + bool fpu_enabled = ctx->fpu_enabled; > + > + if (!fpu_enabled || !tif_need_resched_now()) > + return; > + kernel_fpu_end(); > + /* schedule due to preemptible */ > + kernel_fpu_begin(); > +#endif > +} > + >