crypto: limit more FPU-enabled sections
Subsystems (4)
PATCH 1/1 crypto: limit more FPU-enabled sections
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 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.
--- arch/x86/crypto/camellia_aesni_avx2_glue.c | 32 ++++++++++++++++++++++++++ arch/x86/crypto/camellia_aesni_avx_glue.c | 30 ++++++++++++++++++++++++ arch/x86/crypto/cast6_avx_glue.c | 21 +++++++++++++---- arch/x86/crypto/chacha20_glue.c | 9 ++++---- arch/x86/crypto/serpent_avx2_glue.c | 29 +++++++++++++++++++++++ arch/x86/crypto/serpent_avx_glue.c | 20 ++++++++++++---- arch/x86/crypto/serpent_sse2_glue.c | 20 ++++++++++++---- arch/x86/crypto/twofish_avx_glue.c | 37 ++++++++++++++++++++++++++++-- 8 files changed, 179 insertions(+), 19 deletions(-)
arch/x86/crypto/camellia_aesni_avx2_glue.c +32 -0
@@ -206,6 +206,32 @@ struct crypt_priv {bool fpu_enabled; }; +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 encrypt_callback (void *priv,u8 *srcdst,unsigned int nbytes) {const unsigned int bsize =CAMELLIA_BLOCK_SIZE ;
@@ -221,16 +247,19 @@ static void encrypt_callback(void *priv, u8 *srcdst, unsigned int nbytes) }if (nbytes >=CAMELLIA_AESNI_PARALLEL_BLOCKS * bsize) { +camellia_fpu_sched_rt (ctx);camellia_ecb_enc_16way (ctx->ctx, srcdst, srcdst); srcdst += bsize *CAMELLIA_AESNI_PARALLEL_BLOCKS ; nbytes -= bsize *CAMELLIA_AESNI_PARALLEL_BLOCKS ; }while (nbytes >=CAMELLIA_PARALLEL_BLOCKS * bsize) { +camellia_fpu_sched_rt (ctx);camellia_enc_blk_2way (ctx->ctx, srcdst, srcdst); srcdst += bsize *CAMELLIA_PARALLEL_BLOCKS ; nbytes -= bsize *CAMELLIA_PARALLEL_BLOCKS ; } +camellia_fpu_end_rt (ctx);for (i =0 ; i < nbytes / bsize; i++, srcdst += bsize)camellia_enc_blk (ctx->ctx, srcdst, srcdst);
@@ -251,16 +280,19 @@ static void decrypt_callback(void *priv, u8 *srcdst, unsigned int nbytes) }if (nbytes >=CAMELLIA_AESNI_PARALLEL_BLOCKS * bsize) { +camellia_fpu_sched_rt (ctx);camellia_ecb_dec_16way (ctx->ctx, srcdst, srcdst); srcdst += bsize *CAMELLIA_AESNI_PARALLEL_BLOCKS ; nbytes -= bsize *CAMELLIA_AESNI_PARALLEL_BLOCKS ; }while (nbytes >=CAMELLIA_PARALLEL_BLOCKS * bsize) { +camellia_fpu_sched_rt (ctx);camellia_dec_blk_2way (ctx->ctx, srcdst, srcdst); srcdst += bsize *CAMELLIA_PARALLEL_BLOCKS ; nbytes -= bsize *CAMELLIA_PARALLEL_BLOCKS ; } +camellia_fpu_end_rt (ctx);for (i =0 ; i < nbytes / bsize; i++, srcdst += bsize)camellia_dec_blk (ctx->ctx, srcdst, srcdst);
arch/x86/crypto/camellia_aesni_avx_glue.c +30 -0
@@ -210,6 +210,32 @@ struct crypt_priv {bool fpu_enabled; }; +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 encrypt_callback (void *priv,u8 *srcdst,unsigned int nbytes) {const unsigned int bsize =CAMELLIA_BLOCK_SIZE ;
@@ -225,10 +251,12 @@ static void encrypt_callback(void *priv, u8 *srcdst, unsigned int nbytes) }while (nbytes >=CAMELLIA_PARALLEL_BLOCKS * bsize) { +camellia_fpu_sched_rt (ctx);camellia_enc_blk_2way (ctx->ctx, srcdst, srcdst); srcdst += bsize *CAMELLIA_PARALLEL_BLOCKS ; nbytes -= bsize *CAMELLIA_PARALLEL_BLOCKS ; } +camellia_fpu_end_rt (ctx);for (i =0 ; i < nbytes / bsize; i++, srcdst += bsize)camellia_enc_blk (ctx->ctx, srcdst, srcdst);
@@ -249,10 +277,12 @@ static void decrypt_callback(void *priv, u8 *srcdst, unsigned int nbytes) }while (nbytes >=CAMELLIA_PARALLEL_BLOCKS * bsize) { +camellia_fpu_sched_rt (ctx);camellia_dec_blk_2way (ctx->ctx, srcdst, srcdst); srcdst += bsize *CAMELLIA_PARALLEL_BLOCKS ; nbytes -= bsize *CAMELLIA_PARALLEL_BLOCKS ; } +camellia_fpu_end_rt (ctx);for (i =0 ; i < nbytes / bsize; i++, srcdst += bsize)camellia_dec_blk (ctx->ctx, srcdst, srcdst);
arch/x86/crypto/cast6_avx_glue.c +16 -5
@@ -205,19 +205,30 @@ struct crypt_priv {bool fpu_enabled; }; +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 encrypt_callback (void *priv,u8 *srcdst,unsigned int nbytes) {const unsigned int bsize =CAST6_BLOCK_SIZE ;struct crypt_priv *ctx = priv;int i; - ctx->fpu_enabled =cast6_fpu_begin (ctx->fpu_enabled, nbytes); -if (nbytes == bsize *CAST6_PARALLEL_BLOCKS ) { + ctx->fpu_enabled =cast6_fpu_begin (ctx->fpu_enabled, nbytes);cast6_ecb_enc_8way (ctx->ctx, srcdst, srcdst); +cast6_fpu_end_rt (ctx);return ; } -for (i =0 ; i < nbytes / bsize; i++, srcdst += bsize)__cast6_encrypt (ctx->ctx, srcdst, srcdst); }
@@ -228,10 +239,10 @@ static void decrypt_callback(void *priv, u8 *srcdst, unsigned int nbytes)struct crypt_priv *ctx = priv;int i; - ctx->fpu_enabled =cast6_fpu_begin (ctx->fpu_enabled, nbytes); -if (nbytes == bsize *CAST6_PARALLEL_BLOCKS ) { + ctx->fpu_enabled =cast6_fpu_begin (ctx->fpu_enabled, nbytes);cast6_ecb_dec_8way (ctx->ctx, srcdst, srcdst); +cast6_fpu_end_rt (ctx);return ; }
arch/x86/crypto/chacha20_glue.c +5 -4
@@ -81,23 +81,24 @@ static int chacha20_simd(struct skcipher_request *req)crypto_chacha20_init (state, ctx, walk.iv); -kernel_fpu_begin (); -while (walk.nbytes >=CHACHA20_BLOCK_SIZE ) { +kernel_fpu_begin (); +chacha20_dosimd (state, walk.dst.virt.addr, walk.src.virt.addr,rounddown (walk.nbytes,CHACHA20_BLOCK_SIZE )); +kernel_fpu_end (); err =skcipher_walk_done (&walk, walk.nbytes %CHACHA20_BLOCK_SIZE ); }if (walk.nbytes) { +kernel_fpu_begin ();chacha20_dosimd (state, walk.dst.virt.addr, walk.src.virt.addr, walk.nbytes); +kernel_fpu_end (); err =skcipher_walk_done (&walk,0 ); } -kernel_fpu_end (); -return err; }
arch/x86/crypto/serpent_avx2_glue.c +29 -0
@@ -184,6 +184,31 @@ 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 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) {const unsigned int bsize =SERPENT_BLOCK_SIZE ;
@@ -199,10 +224,12 @@ static void encrypt_callback(void *priv, u8 *srcdst, unsigned int nbytes) }while (nbytes >=SERPENT_PARALLEL_BLOCKS * bsize) { +serpent_fpu_sched_rt (ctx);serpent_ecb_enc_8way_avx (ctx->ctx, srcdst, srcdst); srcdst += bsize *SERPENT_PARALLEL_BLOCKS ; nbytes -= bsize *SERPENT_PARALLEL_BLOCKS ; } +serpent_fpu_end_rt (ctx);for (i =0 ; i < nbytes / bsize; i++, srcdst += bsize)__serpent_encrypt (ctx->ctx, srcdst, srcdst);
@@ -223,10 +250,12 @@ static void decrypt_callback(void *priv, u8 *srcdst, unsigned int nbytes) }while (nbytes >=SERPENT_PARALLEL_BLOCKS * bsize) { +serpent_fpu_sched_rt (ctx);serpent_ecb_dec_8way_avx (ctx->ctx, srcdst, srcdst); srcdst += bsize *SERPENT_PARALLEL_BLOCKS ; nbytes -= bsize *SERPENT_PARALLEL_BLOCKS ; } +serpent_fpu_end_rt (ctx);for (i =0 ; i < nbytes / bsize; i++, srcdst += bsize)__serpent_decrypt (ctx->ctx, srcdst, srcdst);
arch/x86/crypto/serpent_avx_glue.c +16 -4
@@ -218,16 +218,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 encrypt_callback (void *priv,u8 *srcdst,unsigned int nbytes) {const unsigned int bsize =SERPENT_BLOCK_SIZE ;struct crypt_priv *ctx = priv;int i; - ctx->fpu_enabled =serpent_fpu_begin (ctx->fpu_enabled, nbytes); -if (nbytes == bsize *SERPENT_PARALLEL_BLOCKS ) { + ctx->fpu_enabled =serpent_fpu_begin (ctx->fpu_enabled, nbytes);serpent_ecb_enc_8way_avx (ctx->ctx, srcdst, srcdst); +serpent_fpu_end_rt (ctx);return ; }
@@ -241,10 +253,10 @@ static void decrypt_callback(void *priv, u8 *srcdst, unsigned int nbytes)struct crypt_priv *ctx = priv;int i; - ctx->fpu_enabled =serpent_fpu_begin (ctx->fpu_enabled, nbytes); -if (nbytes == bsize *SERPENT_PARALLEL_BLOCKS ) { + ctx->fpu_enabled =serpent_fpu_begin (ctx->fpu_enabled, nbytes);serpent_ecb_dec_8way_avx (ctx->ctx, srcdst, srcdst); +serpent_fpu_end_rt (ctx);return ; }
arch/x86/crypto/serpent_sse2_glue.c +16 -4
@@ -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 encrypt_callback (void *priv,u8 *srcdst,unsigned int nbytes) {const unsigned int bsize =SERPENT_BLOCK_SIZE ;struct crypt_priv *ctx = priv;int i; - ctx->fpu_enabled =serpent_fpu_begin (ctx->fpu_enabled, nbytes); -if (nbytes == bsize *SERPENT_PARALLEL_BLOCKS ) { + ctx->fpu_enabled =serpent_fpu_begin (ctx->fpu_enabled, nbytes);serpent_enc_blk_xway (ctx->ctx, srcdst, srcdst); +serpent_fpu_end_rt (ctx);return ; }
@@ -210,10 +222,10 @@ static void decrypt_callback(void *priv, u8 *srcdst, unsigned int nbytes)struct crypt_priv *ctx = priv;int i; - ctx->fpu_enabled =serpent_fpu_begin (ctx->fpu_enabled, nbytes); -if (nbytes == bsize *SERPENT_PARALLEL_BLOCKS ) { + ctx->fpu_enabled =serpent_fpu_begin (ctx->fpu_enabled, nbytes);serpent_dec_blk_xway (ctx->ctx, srcdst, srcdst); +serpent_fpu_end_rt (ctx);return ; }
arch/x86/crypto/twofish_avx_glue.c +35 -2
@@ -218,6 +218,31 @@ struct crypt_priv {bool fpu_enabled; }; +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 +} +static void encrypt_callback (void *priv,u8 *srcdst,unsigned int nbytes) {const unsigned int bsize =TF_BLOCK_SIZE ;
@@ -228,12 +253,16 @@ static void encrypt_callback(void *priv, u8 *srcdst, unsigned int nbytes)if (nbytes == bsize *TWOFISH_PARALLEL_BLOCKS ) {twofish_ecb_enc_8way (ctx->ctx, srcdst, srcdst); +twofish_fpu_end_rt (ctx);return ; } -for (i =0 ; i < nbytes / (bsize *3 ); i++, srcdst += bsize *3 ) +for (i =0 ; i < nbytes / (bsize *3 ); i++, srcdst += bsize *3 ) { +twofish_fpu_sched_rt (ctx);twofish_enc_blk_3way (ctx->ctx, srcdst, srcdst); + } +twofish_fpu_end_rt (ctx); nbytes %= bsize *3 ;for (i =0 ; i < nbytes / bsize; i++, srcdst += bsize)
@@ -250,11 +279,15 @@ static void decrypt_callback(void *priv, u8 *srcdst, unsigned int nbytes)if (nbytes == bsize *TWOFISH_PARALLEL_BLOCKS ) {twofish_ecb_dec_8way (ctx->ctx, srcdst, srcdst); +twofish_fpu_end_rt (ctx);return ; } -for (i =0 ; i < nbytes / (bsize *3 ); i++, srcdst += bsize *3 ) +for (i =0 ; i < nbytes / (bsize *3 ); i++, srcdst += bsize *3 ) { +twofish_fpu_sched_rt (ctx);twofish_dec_blk_3way (ctx->ctx, srcdst, srcdst); + } +twofish_fpu_end_rt (ctx); nbytes %= bsize *3 ;
Signature · 1 line
2.15.0
Quoted 3 lines · On 2017-11-30 15:22:17 [+0100], To [email protected] wrote:
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.
This is only x86. We have the same fun with kernel_neon_begin() on the arm/arm64 side…
Sebastian
PATCH 1/1 arm*: disable NEON in kernel mode
NEON in kernel mode is used by the crypto algorithms and raid6 code. While the raid6 code looks okay, the crypto algorithms do not: NEON is enabled on first invocation and may allocate/free/map memory before the NEON mode is disabled again.
This needs to be changed until it can be enabled.
On ARM NEON in kernel mode can be simply disabled. on ARM64 it needs to stay on due to possible EFI callbacks so here I disable each algorithm.
--- arch/arm/Kconfig | 2 +- arch/arm64/crypto/Kconfig | 20 ++++++++++---------- arch/arm64/crypto/crc32-ce-glue.c | 3 ++- 3 files changed, 13 insertions(+), 12 deletions(-)
arch/arm/Kconfig +1 -1
@@ -2164,7 +2164,7 @@ config NEON config KERNEL_MODE_NEON bool "Support for NEON in kernel mode" - depends on NEON && AEABI + depends on NEON && AEABI && !PREEMPT_RT_BASE help Say Y to include support for NEON in kernel mode.
arch/arm64/crypto/Kconfig +10 -10
@@ -19,19 +19,19 @@ config CRYPTO_SHA512_ARM64 config CRYPTO_SHA1_ARM64_CE tristate "SHA-1 digest algorithm (ARMv8 Crypto Extensions)" - depends on KERNEL_MODE_NEON + depends on KERNEL_MODE_NEON && !PREEMPT_RT_BASE select CRYPTO_HASH select CRYPTO_SHA1 config CRYPTO_SHA2_ARM64_CE tristate "SHA-224/SHA-256 digest algorithm (ARMv8 Crypto Extensions)" - depends on KERNEL_MODE_NEON + depends on KERNEL_MODE_NEON && !PREEMPT_RT_BASE select CRYPTO_HASH select CRYPTO_SHA256_ARM64 config CRYPTO_GHASH_ARM64_CE tristate "GHASH/AES-GCM using ARMv8 Crypto Extensions" - depends on KERNEL_MODE_NEON + depends on KERNEL_MODE_NEON && !PREEMPT_RT_BASE select CRYPTO_HASH select CRYPTO_GF128MUL select CRYPTO_AES
@@ -39,7 +39,7 @@ config CRYPTO_GHASH_ARM64_CE config CRYPTO_CRCT10DIF_ARM64_CE tristate "CRCT10DIF digest algorithm using PMULL instructions" - depends on KERNEL_MODE_NEON && CRC_T10DIF + depends on KERNEL_MODE_NEON && CRC_T10DIF && !PREEMPT_RT_BASE select CRYPTO_HASH config CRYPTO_CRC32_ARM64_CE
@@ -53,13 +53,13 @@ config CRYPTO_AES_ARM64 config CRYPTO_AES_ARM64_CE tristate "AES core cipher using ARMv8 Crypto Extensions" - depends on ARM64 && KERNEL_MODE_NEON + depends on ARM64 && KERNEL_MODE_NEON && !PREEMPT_RT_BASE select CRYPTO_ALGAPI select CRYPTO_AES_ARM64 config CRYPTO_AES_ARM64_CE_CCM tristate "AES in CCM mode using ARMv8 Crypto Extensions" - depends on ARM64 && KERNEL_MODE_NEON + depends on ARM64 && KERNEL_MODE_NEON && !PREEMPT_RT_BASE select CRYPTO_ALGAPI select CRYPTO_AES_ARM64_CE select CRYPTO_AES_ARM64
@@ -67,7 +67,7 @@ config CRYPTO_AES_ARM64_CE_CCM config CRYPTO_AES_ARM64_CE_BLK tristate "AES in ECB/CBC/CTR/XTS modes using ARMv8 Crypto Extensions" - depends on KERNEL_MODE_NEON + depends on KERNEL_MODE_NEON && !PREEMPT_RT_BASE select CRYPTO_BLKCIPHER select CRYPTO_AES_ARM64_CE select CRYPTO_AES_ARM64
@@ -75,7 +75,7 @@ config CRYPTO_AES_ARM64_CE_BLK config CRYPTO_AES_ARM64_NEON_BLK tristate "AES in ECB/CBC/CTR/XTS modes using NEON instructions" - depends on KERNEL_MODE_NEON + depends on KERNEL_MODE_NEON && !PREEMPT_RT_BASE select CRYPTO_BLKCIPHER select CRYPTO_AES_ARM64 select CRYPTO_AES
@@ -83,13 +83,13 @@ config CRYPTO_AES_ARM64_NEON_BLK config CRYPTO_CHACHA20_NEON tristate "NEON accelerated ChaCha20 symmetric cipher" - depends on KERNEL_MODE_NEON + depends on KERNEL_MODE_NEON && !PREEMPT_RT_BASE select CRYPTO_BLKCIPHER select CRYPTO_CHACHA20 config CRYPTO_AES_ARM64_BS tristate "AES in ECB/CBC/CTR/XTS modes using bit-sliced NEON algorithm" - depends on KERNEL_MODE_NEON + depends on KERNEL_MODE_NEON && !PREEMPT_RT_BASE select CRYPTO_BLKCIPHER select CRYPTO_AES_ARM64_NEON_BLK select CRYPTO_AES_ARM64
arch/arm64/crypto/crc32-ce-glue.c +2 -1
@@ -206,7 +206,8 @@ static struct shash_alg crc32_pmull_algs[] = { {static int __init crc32_pmull_mod_init (void ) { -if (IS_ENABLED (CONFIG_KERNEL_MODE_NEON ) && (elf_hwcap &HWCAP_PMULL )) { +if (IS_ENABLED (CONFIG_KERNEL_MODE_NEON ) && + !IS_ENABLED (CONFIG_PREEMPT_RT_BASE ) && (elf_hwcap &HWCAP_PMULL )) { crc32_pmull_algs[0 ].update = crc32_pmull_update; crc32_pmull_algs[1 ].update = crc32c_pmull_update;
Signature · 1 line
2.15.0
Re: [PATCH RT] arm*: disable NEON in kernel mode
+arm folks, to let you know
Quoted 131 lines · To linux-rt-users at vger.kernel.org
NEON in kernel mode is used by the crypto algorithms and raid6 code. While the raid6 code looks okay, the crypto algorithms do not: NEON is enabled on first invocation and may allocate/free/map memory before the NEON mode is disabled again.
This needs to be changed until it can be enabled.
On ARM NEON in kernel mode can be simply disabled. on ARM64 it needs to stay on due to possible EFI callbacks so here I disable each algorithm.Cc stable-rt at vger.kernel.orgSigned-off-by Sebastian Andrzej Siewior <[email protected]>--- arch/arm/Kconfig | 2 +- arch/arm64/crypto/Kconfig | 20 ++++++++++---------- arch/arm64/crypto/crc32-ce-glue.c | 3 ++- 3 files changed, 13 insertions(+), 12 deletions(-)arch/arm/Kconfig +1
-1@@ -2164,7 +2164,7 @@ config NEON config KERNEL_MODE_NEON bool "Support for NEON in kernel mode" - depends on NEON && AEABI + depends on NEON && AEABI && !PREEMPT_RT_BASE help Say Y to include support for NEON in kernel mode.arch/arm64/crypto/Kconfig +10
-10@@ -19,19 +19,19 @@ config CRYPTO_SHA512_ARM64 config CRYPTO_SHA1_ARM64_CE tristate "SHA-1 digest algorithm (ARMv8 Crypto Extensions)" - depends on KERNEL_MODE_NEON + depends on KERNEL_MODE_NEON && !PREEMPT_RT_BASE select CRYPTO_HASH select CRYPTO_SHA1 config CRYPTO_SHA2_ARM64_CE tristate "SHA-224/SHA-256 digest algorithm (ARMv8 Crypto Extensions)" - depends on KERNEL_MODE_NEON + depends on KERNEL_MODE_NEON && !PREEMPT_RT_BASE select CRYPTO_HASH select CRYPTO_SHA256_ARM64 config CRYPTO_GHASH_ARM64_CE tristate "GHASH/AES-GCM using ARMv8 Crypto Extensions" - depends on KERNEL_MODE_NEON + depends on KERNEL_MODE_NEON && !PREEMPT_RT_BASE select CRYPTO_HASH select CRYPTO_GF128MUL select CRYPTO_AES@@ -39,7 +39,7 @@ config CRYPTO_GHASH_ARM64_CE config CRYPTO_CRCT10DIF_ARM64_CE tristate "CRCT10DIF digest algorithm using PMULL instructions" - depends on KERNEL_MODE_NEON && CRC_T10DIF + depends on KERNEL_MODE_NEON && CRC_T10DIF && !PREEMPT_RT_BASE select CRYPTO_HASH config CRYPTO_CRC32_ARM64_CE@@ -53,13 +53,13 @@ config CRYPTO_AES_ARM64 config CRYPTO_AES_ARM64_CE tristate "AES core cipher using ARMv8 Crypto Extensions" - depends on ARM64 && KERNEL_MODE_NEON + depends on ARM64 && KERNEL_MODE_NEON && !PREEMPT_RT_BASE select CRYPTO_ALGAPI select CRYPTO_AES_ARM64 config CRYPTO_AES_ARM64_CE_CCM tristate "AES in CCM mode using ARMv8 Crypto Extensions" - depends on ARM64 && KERNEL_MODE_NEON + depends on ARM64 && KERNEL_MODE_NEON && !PREEMPT_RT_BASE select CRYPTO_ALGAPI select CRYPTO_AES_ARM64_CE select CRYPTO_AES_ARM64@@ -67,7 +67,7 @@ config CRYPTO_AES_ARM64_CE_CCM config CRYPTO_AES_ARM64_CE_BLK tristate "AES in ECB/CBC/CTR/XTS modes using ARMv8 Crypto Extensions" - depends on KERNEL_MODE_NEON + depends on KERNEL_MODE_NEON && !PREEMPT_RT_BASE select CRYPTO_BLKCIPHER select CRYPTO_AES_ARM64_CE select CRYPTO_AES_ARM64@@ -75,7 +75,7 @@ config CRYPTO_AES_ARM64_CE_BLK config CRYPTO_AES_ARM64_NEON_BLK tristate "AES in ECB/CBC/CTR/XTS modes using NEON instructions" - depends on KERNEL_MODE_NEON + depends on KERNEL_MODE_NEON && !PREEMPT_RT_BASE select CRYPTO_BLKCIPHER select CRYPTO_AES_ARM64 select CRYPTO_AES@@ -83,13 +83,13 @@ config CRYPTO_AES_ARM64_NEON_BLK config CRYPTO_CHACHA20_NEON tristate "NEON accelerated ChaCha20 symmetric cipher" - depends on KERNEL_MODE_NEON + depends on KERNEL_MODE_NEON && !PREEMPT_RT_BASE select CRYPTO_BLKCIPHER select CRYPTO_CHACHA20 config CRYPTO_AES_ARM64_BS tristate "AES in ECB/CBC/CTR/XTS modes using bit-sliced NEON algorithm" - depends on KERNEL_MODE_NEON + depends on KERNEL_MODE_NEON && !PREEMPT_RT_BASE select CRYPTO_BLKCIPHER select CRYPTO_AES_ARM64_NEON_BLK select CRYPTO_AES_ARM64arch/arm64/crypto/crc32-ce-glue.c +2
-1@@ -206,7 +206,8 @@ static struct shash_alg crc32_pmull_algs[] = { {static int __init crc32_pmull_mod_init (void ) { -if (IS_ENABLED (CONFIG_KERNEL_MODE_NEON ) && (elf_hwcap &HWCAP_PMULL )) { +if (IS_ENABLED (CONFIG_KERNEL_MODE_NEON ) && + !IS_ENABLED (CONFIG_PREEMPT_RT_BASE ) && (elf_hwcap &HWCAP_PMULL )) { crc32_pmull_algs[0 ].update = crc32_pmull_update; crc32_pmull_algs[1 ].update = crc32c_pmull_update;Signature · 1 line
2.15.0
Mark Rutland
reviewerRe: [PATCH RT] arm*: disable NEON in kernel mode
[Adding Ard, who wrote the NEON crypto code]
Quoted 7 lines · Sebastian Andrzej Siewior
+arm folks, to let you know
Quoted 4 lines · To linux-rt-users at vger.kernel.org
NEON in kernel mode is used by the crypto algorithms and raid6 code. While the raid6 code looks okay, the crypto algorithms do not: NEON is enabled on first invocation and may allocate/free/map memory before the NEON mode is disabled again.
Could you elaborate on why this is a problem?
I guess this is because kernel_neon_{begin,end}() disable preemption?
... is this specific to RT?
Thanks,
Mark.
Quoted 132 lines
Quoted 127 lines
This needs to be changed until it can be enabled.
On ARM NEON in kernel mode can be simply disabled. on ARM64 it needs to stay on due to possible EFI callbacks so here I disable each algorithm.Cc stable-rt at vger.kernel.orgSigned-off-by Sebastian Andrzej Siewior <[email protected]>--- arch/arm/Kconfig | 2 +- arch/arm64/crypto/Kconfig | 20 ++++++++++---------- arch/arm64/crypto/crc32-ce-glue.c | 3 ++- 3 files changed, 13 insertions(+), 12 deletions(-)arch/arm/Kconfig +1
-1@@ -2164,7 +2164,7 @@ config NEON config KERNEL_MODE_NEON bool "Support for NEON in kernel mode" - depends on NEON && AEABI + depends on NEON && AEABI && !PREEMPT_RT_BASE help Say Y to include support for NEON in kernel mode.arch/arm64/crypto/Kconfig +10
-10@@ -19,19 +19,19 @@ config CRYPTO_SHA512_ARM64 config CRYPTO_SHA1_ARM64_CE tristate "SHA-1 digest algorithm (ARMv8 Crypto Extensions)" - depends on KERNEL_MODE_NEON + depends on KERNEL_MODE_NEON && !PREEMPT_RT_BASE select CRYPTO_HASH select CRYPTO_SHA1 config CRYPTO_SHA2_ARM64_CE tristate "SHA-224/SHA-256 digest algorithm (ARMv8 Crypto Extensions)" - depends on KERNEL_MODE_NEON + depends on KERNEL_MODE_NEON && !PREEMPT_RT_BASE select CRYPTO_HASH select CRYPTO_SHA256_ARM64 config CRYPTO_GHASH_ARM64_CE tristate "GHASH/AES-GCM using ARMv8 Crypto Extensions" - depends on KERNEL_MODE_NEON + depends on KERNEL_MODE_NEON && !PREEMPT_RT_BASE select CRYPTO_HASH select CRYPTO_GF128MUL select CRYPTO_AES@@ -39,7 +39,7 @@ config CRYPTO_GHASH_ARM64_CE config CRYPTO_CRCT10DIF_ARM64_CE tristate "CRCT10DIF digest algorithm using PMULL instructions" - depends on KERNEL_MODE_NEON && CRC_T10DIF + depends on KERNEL_MODE_NEON && CRC_T10DIF && !PREEMPT_RT_BASE select CRYPTO_HASH config CRYPTO_CRC32_ARM64_CE@@ -53,13 +53,13 @@ config CRYPTO_AES_ARM64 config CRYPTO_AES_ARM64_CE tristate "AES core cipher using ARMv8 Crypto Extensions" - depends on ARM64 && KERNEL_MODE_NEON + depends on ARM64 && KERNEL_MODE_NEON && !PREEMPT_RT_BASE select CRYPTO_ALGAPI select CRYPTO_AES_ARM64 config CRYPTO_AES_ARM64_CE_CCM tristate "AES in CCM mode using ARMv8 Crypto Extensions" - depends on ARM64 && KERNEL_MODE_NEON + depends on ARM64 && KERNEL_MODE_NEON && !PREEMPT_RT_BASE select CRYPTO_ALGAPI select CRYPTO_AES_ARM64_CE select CRYPTO_AES_ARM64@@ -67,7 +67,7 @@ config CRYPTO_AES_ARM64_CE_CCM config CRYPTO_AES_ARM64_CE_BLK tristate "AES in ECB/CBC/CTR/XTS modes using ARMv8 Crypto Extensions" - depends on KERNEL_MODE_NEON + depends on KERNEL_MODE_NEON && !PREEMPT_RT_BASE select CRYPTO_BLKCIPHER select CRYPTO_AES_ARM64_CE select CRYPTO_AES_ARM64@@ -75,7 +75,7 @@ config CRYPTO_AES_ARM64_CE_BLK config CRYPTO_AES_ARM64_NEON_BLK tristate "AES in ECB/CBC/CTR/XTS modes using NEON instructions" - depends on KERNEL_MODE_NEON + depends on KERNEL_MODE_NEON && !PREEMPT_RT_BASE select CRYPTO_BLKCIPHER select CRYPTO_AES_ARM64 select CRYPTO_AES@@ -83,13 +83,13 @@ config CRYPTO_AES_ARM64_NEON_BLK config CRYPTO_CHACHA20_NEON tristate "NEON accelerated ChaCha20 symmetric cipher" - depends on KERNEL_MODE_NEON + depends on KERNEL_MODE_NEON && !PREEMPT_RT_BASE select CRYPTO_BLKCIPHER select CRYPTO_CHACHA20 config CRYPTO_AES_ARM64_BS tristate "AES in ECB/CBC/CTR/XTS modes using bit-sliced NEON algorithm" - depends on KERNEL_MODE_NEON + depends on KERNEL_MODE_NEON && !PREEMPT_RT_BASE select CRYPTO_BLKCIPHER select CRYPTO_AES_ARM64_NEON_BLK select CRYPTO_AES_ARM64arch/arm64/crypto/crc32-ce-glue.c +2
-1@@ -206,7 +206,8 @@ static struct shash_alg crc32_pmull_algs[] = { {static int __init crc32_pmull_mod_init (void ) { -if (IS_ENABLED (CONFIG_KERNEL_MODE_NEON ) && (elf_hwcap &HWCAP_PMULL )) { +if (IS_ENABLED (CONFIG_KERNEL_MODE_NEON ) && + !IS_ENABLED (CONFIG_PREEMPT_RT_BASE ) && (elf_hwcap &HWCAP_PMULL )) { crc32_pmull_algs[0 ].update = crc32_pmull_update; crc32_pmull_algs[1 ].update = crc32c_pmull_update;Signature · 1 line
2.15.0_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel at lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Re: [PATCH RT] arm*: disable NEON in kernel mode
Quoted 11 lines · Mark Rutland
[Adding Ard, who wrote the NEON crypto code]
Quoted 7 lines · Sebastian Andrzej Siewior
+arm folks, to let you know
Quoted 4 lines · To linux-rt-users at vger.kernel.org
NEON in kernel mode is used by the crypto algorithms and raid6 code. While the raid6 code looks okay, the crypto algorithms do not: NEON is enabled on first invocation and may allocate/free/map memory before the NEON mode is disabled again.
Quoted 5 lines
Could you elaborate on why this is a problem?
I guess this is because kernel_neon_{begin,end}() disable preemption?
... is this specific to RT?
It is RT specific, yes. One thing are the unbounded latencies since everything in this preempt_disable section can take time depending on the size of the request.
The other thing is code like in
arch/arm64/crypto/aes-ce-ccm-glue.c:ccm_encrypt()
where within this preempt_disable() section skcipher_walk_done() is invoked. That function can allocate/free/map memory which is okay for !RT but is not for RT. I tried to break those loops for x86 [0] and I simply didn't had the time to do the same for ARM. I am aware that store/restore of the NEON registers (as SSE and AVX) is expensive and doing a lot of operations in one go is desired. So for x86 I would want to do some benchmarks and come up with some numbers based on which I can argue with people one way or another depending on how much it hurts and how long preemption can be disabled.
[0] https://www.spinics.net/lists/kernel/msg2663115.html
Quoted 2 lines
Thanks,
Mark.
Sebastian
Re: [PATCH RT] arm*: disable NEON in kernel mode
l
Quoted 26 lines · Sebastian Andrzej Siewior
Quoted 13 lines · Mark Rutland
[Adding Ard, who wrote the NEON crypto code]
Quoted 8 lines · Sebastian Andrzej Siewior
+arm folks, to let you know
Quoted 5 lines · To linux-rt-users at vger.kernel.org
NEON in kernel mode is used by the crypto algorithms and raid6 code. While the raid6 code looks okay, the crypto algorithms do not: NEON is enabled on first invocation and may allocate/free/map memory before the NEON mode is disabled again.
Quoted 5 lines
Could you elaborate on why this is a problem?
I guess this is because kernel_neon_{begin,end}() disable preemption?
... is this specific to RT?
It is RT specific, yes. One thing are the unbounded latencies since everything in this
preempt_disablesection can take time depending on the size of the request.
The other thing is code like in
arch/arm64/crypto/aes-ce-ccm-glue.c:ccm_encrypt()
Quoted 6 lines
where within this
preempt_disable()sectionskcipher_walk_done()is invoked. That function can allocate/free/map memory which is okay for !RT but is not for RT. I tried to break those loops for x86 [0] and I simply didn't had the time to do the same for ARM. I am aware that store/restore of the NEON registers (as SSE and AVX) is expensive and doing a lot of operations in one go is desired.
I wouldn?t mind fixing the code instead. We never disable the neon, but only stack the contents of the registers the first time, and unstack them only before returning to userland (with the exception of nested neon use in softirq context). When this code was introduced, we always stacked/unstacked the whole register file eagerly every time.
Quoted 11 lines
So for x86 I would want
to do some benchmarks and come up with some numbers based on which I can argue with people one way or another depending on how much it hurts and how long preemption can be disabled.[0] https://www.spinics.net/lists/kernel/msg2663115.html
Quoted 2 lines
Thanks,
Mark.Sebastian
Re: [PATCH RT] arm*: disable NEON in kernel mode
Quoted 35 lines · Ard Biesheuvel
l
Quoted 26 lines · Sebastian Andrzej Siewior
Quoted 13 lines · Mark Rutland
[Adding Ard, who wrote the NEON crypto code]
Quoted 8 lines · Sebastian Andrzej Siewior
+arm folks, to let you know
Quoted 5 lines · To linux-rt-users at vger.kernel.org
NEON in kernel mode is used by the crypto algorithms and raid6 code. While the raid6 code looks okay, the crypto algorithms do not: NEON is enabled on first invocation and may allocate/free/map memory before the NEON mode is disabled again.
Quoted 5 lines
Could you elaborate on why this is a problem?
I guess this is because kernel_neon_{begin,end}() disable preemption?
... is this specific to RT?
It is RT specific, yes. One thing are the unbounded latencies since everything in this
preempt_disablesection can take time depending on the size of the request.
The other thing is code like in
arch/arm64/crypto/aes-ce-ccm-glue.c:ccm_encrypt()Quoted 6 lines
where within this
preempt_disable()sectionskcipher_walk_done()is invoked. That function can allocate/free/map memory which is okay for !RT but is not for RT. I tried to break those loops for x86 [0] and I simply didn't had the time to do the same for ARM. I am aware that store/restore of the NEON registers (as SSE and AVX) is expensive and doing a lot of operations in one go is desired.
Quoted 6 lines
I wouldn?t mind fixing the code instead. We never disable the neon, but only stack the contents of the registers the first time, and unstack them only before returning to userland (with the exception of nested neon use in softirq context). When this code was introduced, we always stacked/unstacked the whole register file eagerly every time.
+1, at least for arm64. I don't see a really compelling reason for holding kernel-mode NEON around memory management now that we have a strict save-once-restore-lazily model.
This may not work so well for arm though -- I haven't looked at that code for a while.
If there is memory manamement in any core loop, you already lost the performance battle, and an extra kernel_neon_end()+kernel_neon_begin() may not be that catastrophic.
[...]
Cheers
---Dave
Russell King - ARM Linux
maintainerRe: [PATCH RT] arm*: disable NEON in kernel mode
Quoted 11 lines · Dave Martin
+1, at least for arm64. I don't see a really compelling reason for holding kernel-mode NEON around memory management now that we have a strict save-once-restore-lazily model.
This may not work so well for arm though -- I haven't looked at that code for a while.
If there is memory manamement in any core loop, you already lost the performance battle, and an extra kernel_neon_end()+kernel_neon_begin() may not be that catastrophic.
Remember that we don't permit context switches while kernel neon is in use on ARM - if there's any possibility of scheduling to occur, the get_cpu() in kernel_neon_begin() should trigger a schedule-while- atomic warning.
Signature · 3 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up
Re: [PATCH RT] arm*: disable NEON in kernel mode
Quoted 9 lines · Russell King - ARM Linux
Quoted 8 lines · Dave Martin
+1, at least for arm64. I don't see a really compelling reason for holding kernel-mode NEON around memory management now that we have a strict save-once-restore-lazily model.
This may not work so well for arm though -- I haven't looked at that code for a while.
Quoted 8 lines
Quoted 3 lines
If there is memory manamement in any core loop, you already lost the performance battle, and an extra kernel_neon_end()+kernel_neon_begin() may not be that catastrophic.
Remember that we don't permit context switches while kernel neon is in use on ARM - if there's any possibility of scheduling to occur, the
get_cpu()inkernel_neon_begin()should trigger a schedule-while- atomic warning.
Agreed, and an arm64 the same is true. (Actually softirq is disabled too, for tortuous reasons involving SVE.)
Quoted 7 lines · To linux-rt-users at vger.kernel.org
NEON in kernel mode is used by the crypto algorithms and raid6 code. While the raid6 code looks okay, the crypto algorithms do not: NEON is enabled on first invocation and may allocate/free/map memory before the NEON mode is disabled again.
This needs to be changed until it can be enabled.
On ARM NEON in kernel mode can be simply disabled. on ARM64 it needs to stay on due to possible EFI callbacks so here I disable each algorithm.
[...]
arch/arm64/crypto/Kconfig config CRYPTO_SHA512_ARM64 · line 24
Quoted 12 lines
arch/arm64/crypto/Kconfig +1
-1@@ -19,19 +19,19 @@ config CRYPTO_SHA512_ARM64 config CRYPTO_SHA1_ARM64_CE tristate "SHA-1 digest algorithm (ARMv8 Crypto Extensions)" - depends on KERNEL_MODE_NEON + depends on KERNEL_MODE_NEON && !PREEMPT_RT_BASE select CRYPTO_HASH select CRYPTO_SHA1
Sebastian, can you piont to where sha1 (say) hits this issue? I wonder whether this is really a sign that some refactoring is needed.
Cheers
---Dave
Re: [PATCH RT] arm*: disable NEON in kernel mode
Quoted 39 lines · Dave Martin
Quoted 9 lines · Russell King - ARM Linux
Quoted 8 lines · Dave Martin
+1, at least for arm64. I don't see a really compelling reason for holding kernel-mode NEON around memory management now that we have a strict save-once-restore-lazily model.
This may not work so well for arm though -- I haven't looked at that code for a while.
Quoted 8 lines
Quoted 3 lines
If there is memory manamement in any core loop, you already lost the performance battle, and an extra kernel_neon_end()+kernel_neon_begin() may not be that catastrophic.
Remember that we don't permit context switches while kernel neon is in use on ARM - if there's any possibility of scheduling to occur, the
get_cpu()inkernel_neon_begin()should trigger a schedule-while- atomic warning.Agreed, and an arm64 the same is true. (Actually softirq is disabled too, for tortuous reasons involving SVE.)
Quoted 7 lines · To linux-rt-users at vger.kernel.org
NEON in kernel mode is used by the crypto algorithms and raid6 code. While the raid6 code looks okay, the crypto algorithms do not: NEON is enabled on first invocation and may allocate/free/map memory before the NEON mode is disabled again.
This needs to be changed until it can be enabled.
On ARM NEON in kernel mode can be simply disabled. on ARM64 it needs to stay on due to possible EFI callbacks so here I disable each algorithm.[...]
Quoted 6 lines
arch/arm64/crypto/Kconfig +0
-0@@ -19,19 +19,19 @@ config CRYPTO_SHA512_ARM64
Quoted 10 lines
arch/arm64/crypto/Kconfig config CRYPTO_SHA512_ARM64 · line 24
Quoted 6 lines
arch/arm64/crypto/Kconfig +1
-1config CRYPTO_SHA1_ARM64_CE tristate "SHA-1 digest algorithm (ARMv8 Crypto Extensions)" - depends on KERNEL_MODE_NEON + depends on KERNEL_MODE_NEON && !PREEMPT_RT_BASE select CRYPTO_HASH select CRYPTO_SHA1Sebastian, can you piont to where sha1 (say) hits this issue? I wonder whether this is really a sign that some refactoring is needed.
I don't think the SHA code is affected. It is primarily the block ciphers, where the scatterwalk API offers some guarantees to the API client (the NEON crypto driver) that data is presented in suitable blocks. This may involve copying data from the scatterlist to a temp buffer if any of the chunks are not blocksize aligned.
Re: [PATCH RT] arm*: disable NEON in kernel mode
In reply to Dave MartinQuoted 6 lines · Dave Martin
Quoted 6 lines
arch/arm64/crypto/Kconfig +0
-0@@ -19,19 +19,19 @@ config CRYPTO_SHA512_ARM64
Quoted 9 lines
arch/arm64/crypto/Kconfig config CRYPTO_SHA512_ARM64 · line 24
Quoted 6 lines
arch/arm64/crypto/Kconfig +1
-1config CRYPTO_SHA1_ARM64_CE tristate "SHA-1 digest algorithm (ARMv8 Crypto Extensions)" - depends on KERNEL_MODE_NEON + depends on KERNEL_MODE_NEON && !PREEMPT_RT_BASE select CRYPTO_HASH select CRYPTO_SHA1Sebastian, can you piont to where sha1 (say) hits this issue? I wonder whether this is really a sign that some refactoring is needed.
I disabled all NEON in one go. Looking at this, it shouldn't be a issue with the block-walk. The only thing that might be a problem is that it can do multiple blocks in one go.
Quoted 2 lines
Cheers
---Dave
Sebastian
Re: [PATCH RT] arm*: disable NEON in kernel mode
In reply to Sebastian Andrzej SiewiorQuoted 14 lines · Sebastian Andrzej Siewior
Quoted 13 lines · Mark Rutland
[Adding Ard, who wrote the NEON crypto code]
Quoted 7 lines · Sebastian Andrzej Siewior
+arm folks, to let you know
Quoted 4 lines · To linux-rt-users at vger.kernel.org
NEON in kernel mode is used by the crypto algorithms and raid6 code. While the raid6 code looks okay, the crypto algorithms do not: NEON is enabled on first invocation and may allocate/free/map memory before the NEON mode is disabled again.
Could you elaborate on why this is a problem?
Quoted 7 lines
Quoted 3 lines
I guess this is because kernel_neon_{begin,end}() disable preemption?
... is this specific to RT?
It is RT specific, yes. One thing are the unbounded latencies since everything in this
preempt_disablesection can take time depending on the size of the request.
Well, PREEMPT cares about that too.
Russell King - ARM Linux
maintainerRe: [PATCH RT] arm*: disable NEON in kernel mode
Quoted 17 lines · Peter Zijlstra
Quoted 16 lines · Sebastian Andrzej Siewior
Quoted 15 lines · Mark Rutland
[Adding Ard, who wrote the NEON crypto code]
Quoted 7 lines · Sebastian Andrzej Siewior
+arm folks, to let you know
Quoted 4 lines · To linux-rt-users at vger.kernel.org
NEON in kernel mode is used by the crypto algorithms and raid6 code. While the raid6 code looks okay, the crypto algorithms do not: NEON is enabled on first invocation and may allocate/free/map memory before the NEON mode is disabled again.
Could you elaborate on why this is a problem?
I guess this is because kernel_neon_{begin,end}() disable preemption?
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
... is this specific to RT?
It is RT specific, yes. One thing are the unbounded latencies since everything in this
preempt_disablesection can take time depending on the size of the request.Well, PREEMPT cares about that too.
Preempt may care, but it's the hit you take to use neon in the kernel. The neon register set shares with the FPU, so preempting during that path means that the normal FPU register saving would corrupt the already saved user FPU context - and even worse would result in the kernel's crypto function register contents being leaked to userspace.
If you care about preempt deeply, the only solution is to avoid using kernel mode neon.
Signature · 3 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up
Re: [PATCH RT] arm*: disable NEON in kernel mode
Quoted 8 lines · Russell King - ARM Linux
Quoted 1 line · Peter Zijlstra
Well, PREEMPT cares about that too.
Preempt may care, but it's the hit you take to use neon in the kernel. The neon register set shares with the FPU, so preempting during that path means that the normal FPU register saving would corrupt the already saved user FPU context - and even worse would result in the kernel's crypto function register contents being leaked to userspace.
Same thing on x86.
Quoted 2 lines
If you care about preempt deeply, the only solution is to avoid using kernel mode neon.
Not quite, you can write the code such that it drops out of neon mode regularly to allow preemption.
So setup a crypto block, enter neon, transform the block, drop out of neon, rinse repeat.
Quoted 23 lines · On Thu, 30 Nov 2017 15:22:17 +0100 Sebastian Andrzej Siewior…
arch/x86/crypto/camellia_aesni_avx2_glue.c +15
-0@@ -206,6 +206,32 @@ struct crypt_priv {bool fpu_enabled; }; +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
arch/x86/crypto/camellia_aesni_avx2_glue.c struct crypt_priv { · line 233
Quoted 10 lines
arch/x86/crypto/camellia_aesni_avx2_glue.c +10
-0+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 +}
I think it would be cleaner to do:
#ifdef CONFIG_PREEMPT_RT_FULL
static void camellia_fpu_end_rt(struct crypt_priv *ctx)
{
bool fpu_enabled = ctx->fpu_enabled;
if (!fpu_enabled)
return;
camellia_fpu_end(fpu_enabled);
ctx->fpu_enabled = false;
}static void camellia_fpu_sched_rt(struct crypt_priv *ctx)
{
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();
}#else
static inline void camellia_fpu_end_rt(struct crypt_priv *ctx) { } static void camellia_fpu_sched_rt(struct crypt_priv *ctx) { } #endif
It really shows that these functions are only used by RT. IMO it's bad taste to have functions with the entire body encapsulated in #ifdefs.
And the same goes for the other functions in this patch.
-- Steve
On 2017-11-30 10:19:43 [-0500], Steven Rostedt wrote:
…
Quoted 4 lines
It really shows that these functions are only used by RT. IMO it's bad taste to have functions with the entire body encapsulated in #ifdefs.
And the same goes for the other functions in this patch.
No problem, I will change that.
Quoted 1 line
-- Steve
Sebastian
PATCH v2 1/1 crypto: limit more FPU-enabled sections
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 process as much of those as possible in one 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.
--- arch/x86/crypto/camellia_aesni_avx2_glue.c | 34 +++++++++++++++++++++++++ arch/x86/crypto/camellia_aesni_avx_glue.c | 32 +++++++++++++++++++++++ arch/x86/crypto/cast6_avx_glue.c | 24 ++++++++++++++--- arch/x86/crypto/chacha20_glue.c | 9 +++--- arch/x86/crypto/serpent_avx2_glue.c | 31 +++++++++++++++++++++++ arch/x86/crypto/serpent_avx_glue.c | 23 ++++++++++++++--- arch/x86/crypto/serpent_sse2_glue.c | 23 ++++++++++++++--- arch/x86/crypto/twofish_avx_glue.c | 39 +++++++++++++++++++++++++++-- 8 files changed, 196 insertions(+), 19 deletions(-)
arch/x86/crypto/camellia_aesni_avx2_glue.c +34 -0
@@ -206,6 +206,34 @@ struct crypt_priv {bool fpu_enabled; }; +#ifdef CONFIG_PREEMPT_RT_FULL +static void camellia_fpu_end_rt (struct crypt_priv *ctx) +{ +bool fpu_enabled = ctx->fpu_enabled; + +if (!fpu_enabled) +return ; +camellia_fpu_end (fpu_enabled); + ctx->fpu_enabled = false; +} + +static void camellia_fpu_sched_rt (struct crypt_priv *ctx) +{ +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 (); +} + +#else +static void camellia_fpu_end_rt (struct crypt_priv *ctx) { } +static void camellia_fpu_sched_rt (struct crypt_priv *ctx) { } +#endif +static void encrypt_callback (void *priv,u8 *srcdst,unsigned int nbytes) {const unsigned int bsize =CAMELLIA_BLOCK_SIZE ;
@@ -221,16 +249,19 @@ static void encrypt_callback(void *priv, }if (nbytes >=CAMELLIA_AESNI_PARALLEL_BLOCKS * bsize) { +camellia_fpu_sched_rt (ctx);camellia_ecb_enc_16way (ctx->ctx, srcdst, srcdst); srcdst += bsize *CAMELLIA_AESNI_PARALLEL_BLOCKS ; nbytes -= bsize *CAMELLIA_AESNI_PARALLEL_BLOCKS ; }while (nbytes >=CAMELLIA_PARALLEL_BLOCKS * bsize) { +camellia_fpu_sched_rt (ctx);camellia_enc_blk_2way (ctx->ctx, srcdst, srcdst); srcdst += bsize *CAMELLIA_PARALLEL_BLOCKS ; nbytes -= bsize *CAMELLIA_PARALLEL_BLOCKS ; } +camellia_fpu_end_rt (ctx);for (i =0 ; i < nbytes / bsize; i++, srcdst += bsize)camellia_enc_blk (ctx->ctx, srcdst, srcdst);
@@ -251,16 +282,19 @@ static void decrypt_callback(void *priv, }if (nbytes >=CAMELLIA_AESNI_PARALLEL_BLOCKS * bsize) { +camellia_fpu_sched_rt (ctx);camellia_ecb_dec_16way (ctx->ctx, srcdst, srcdst); srcdst += bsize *CAMELLIA_AESNI_PARALLEL_BLOCKS ; nbytes -= bsize *CAMELLIA_AESNI_PARALLEL_BLOCKS ; }while (nbytes >=CAMELLIA_PARALLEL_BLOCKS * bsize) { +camellia_fpu_sched_rt (ctx);camellia_dec_blk_2way (ctx->ctx, srcdst, srcdst); srcdst += bsize *CAMELLIA_PARALLEL_BLOCKS ; nbytes -= bsize *CAMELLIA_PARALLEL_BLOCKS ; } +camellia_fpu_end_rt (ctx);for (i =0 ; i < nbytes / bsize; i++, srcdst += bsize)camellia_dec_blk (ctx->ctx, srcdst, srcdst);
arch/x86/crypto/camellia_aesni_avx_glue.c +32 -0
@@ -210,6 +210,34 @@ struct crypt_priv {bool fpu_enabled; }; +#ifdef CONFIG_PREEMPT_RT_FULL +static void camellia_fpu_end_rt (struct crypt_priv *ctx) +{ +bool fpu_enabled = ctx->fpu_enabled; + +if (!fpu_enabled) +return ; +camellia_fpu_end (fpu_enabled); + ctx->fpu_enabled = false; +} + +static void camellia_fpu_sched_rt (struct crypt_priv *ctx) +{ +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 (); +} + +#else +static void camellia_fpu_end_rt (struct crypt_priv *ctx) { } +static void camellia_fpu_sched_rt (struct crypt_priv *ctx) { } +#endif +static void encrypt_callback (void *priv,u8 *srcdst,unsigned int nbytes) {const unsigned int bsize =CAMELLIA_BLOCK_SIZE ;
@@ -225,10 +253,12 @@ static void encrypt_callback(void *priv, }while (nbytes >=CAMELLIA_PARALLEL_BLOCKS * bsize) { +camellia_fpu_sched_rt (ctx);camellia_enc_blk_2way (ctx->ctx, srcdst, srcdst); srcdst += bsize *CAMELLIA_PARALLEL_BLOCKS ; nbytes -= bsize *CAMELLIA_PARALLEL_BLOCKS ; } +camellia_fpu_end_rt (ctx);for (i =0 ; i < nbytes / bsize; i++, srcdst += bsize)camellia_enc_blk (ctx->ctx, srcdst, srcdst);
@@ -249,10 +279,12 @@ static void decrypt_callback(void *priv, }while (nbytes >=CAMELLIA_PARALLEL_BLOCKS * bsize) { +camellia_fpu_sched_rt (ctx);camellia_dec_blk_2way (ctx->ctx, srcdst, srcdst); srcdst += bsize *CAMELLIA_PARALLEL_BLOCKS ; nbytes -= bsize *CAMELLIA_PARALLEL_BLOCKS ; } +camellia_fpu_end_rt (ctx);for (i =0 ; i < nbytes / bsize; i++, srcdst += bsize)camellia_dec_blk (ctx->ctx, srcdst, srcdst);
arch/x86/crypto/cast6_avx_glue.c +19 -5
@@ -205,19 +205,33 @@ struct crypt_priv {bool fpu_enabled; }; +#ifdef CONFIG_PREEMPT_RT_FULL +static void cast6_fpu_end_rt (struct crypt_priv *ctx) +{ +bool fpu_enabled = ctx->fpu_enabled; + +if (!fpu_enabled) +return ; +cast6_fpu_end (fpu_enabled); + ctx->fpu_enabled = false; +} + +#else +static void cast6_fpu_end_rt (struct crypt_priv *ctx) { } +#endif +static void encrypt_callback (void *priv,u8 *srcdst,unsigned int nbytes) {const unsigned int bsize =CAST6_BLOCK_SIZE ;struct crypt_priv *ctx = priv;int i; - ctx->fpu_enabled =cast6_fpu_begin (ctx->fpu_enabled, nbytes); -if (nbytes == bsize *CAST6_PARALLEL_BLOCKS ) { + ctx->fpu_enabled =cast6_fpu_begin (ctx->fpu_enabled, nbytes);cast6_ecb_enc_8way (ctx->ctx, srcdst, srcdst); +cast6_fpu_end_rt (ctx);return ; } -for (i =0 ; i < nbytes / bsize; i++, srcdst += bsize)__cast6_encrypt (ctx->ctx, srcdst, srcdst); }
@@ -228,10 +242,10 @@ static void decrypt_callback(void *priv,struct crypt_priv *ctx = priv;int i; - ctx->fpu_enabled =cast6_fpu_begin (ctx->fpu_enabled, nbytes); -if (nbytes == bsize *CAST6_PARALLEL_BLOCKS ) { + ctx->fpu_enabled =cast6_fpu_begin (ctx->fpu_enabled, nbytes);cast6_ecb_dec_8way (ctx->ctx, srcdst, srcdst); +cast6_fpu_end_rt (ctx);return ; }
arch/x86/crypto/chacha20_glue.c +5 -4
@@ -81,23 +81,24 @@ static int chacha20_simd(struct skciphercrypto_chacha20_init (state, ctx, walk.iv); -kernel_fpu_begin (); -while (walk.nbytes >=CHACHA20_BLOCK_SIZE ) { +kernel_fpu_begin (); +chacha20_dosimd (state, walk.dst.virt.addr, walk.src.virt.addr,rounddown (walk.nbytes,CHACHA20_BLOCK_SIZE )); +kernel_fpu_end (); err =skcipher_walk_done (&walk, walk.nbytes %CHACHA20_BLOCK_SIZE ); }if (walk.nbytes) { +kernel_fpu_begin ();chacha20_dosimd (state, walk.dst.virt.addr, walk.src.virt.addr, walk.nbytes); +kernel_fpu_end (); err =skcipher_walk_done (&walk,0 ); } -kernel_fpu_end (); -return err; }
arch/x86/crypto/serpent_avx2_glue.c +31 -0
@@ -184,6 +184,33 @@ struct crypt_priv {bool fpu_enabled; }; +#ifdef CONFIG_PREEMPT_RT_FULL +static void serpent_fpu_end_rt (struct crypt_priv *ctx) +{ +bool fpu_enabled = ctx->fpu_enabled; + +if (!fpu_enabled) +return ; +serpent_fpu_end (fpu_enabled); + ctx->fpu_enabled = false; +} + +static void serpent_fpu_sched_rt (struct crypt_priv *ctx) +{ +bool fpu_enabled = ctx->fpu_enabled; + +if (!fpu_enabled || !tif_need_resched_now ()) +return ; +kernel_fpu_end (); +/* schedule due to preemptible */ +kernel_fpu_begin (); +} + +#else +static void serpent_fpu_end_rt (struct crypt_priv *ctx) { } +static void serpent_fpu_sched_rt (struct crypt_priv *ctx) { } +#endif +static void encrypt_callback (void *priv,u8 *srcdst,unsigned int nbytes) {const unsigned int bsize =SERPENT_BLOCK_SIZE ;
@@ -199,10 +226,12 @@ static void encrypt_callback(void *priv, }while (nbytes >=SERPENT_PARALLEL_BLOCKS * bsize) { +serpent_fpu_sched_rt (ctx);serpent_ecb_enc_8way_avx (ctx->ctx, srcdst, srcdst); srcdst += bsize *SERPENT_PARALLEL_BLOCKS ; nbytes -= bsize *SERPENT_PARALLEL_BLOCKS ; } +serpent_fpu_end_rt (ctx);for (i =0 ; i < nbytes / bsize; i++, srcdst += bsize)__serpent_encrypt (ctx->ctx, srcdst, srcdst);
@@ -223,10 +252,12 @@ static void decrypt_callback(void *priv, }while (nbytes >=SERPENT_PARALLEL_BLOCKS * bsize) { +serpent_fpu_sched_rt (ctx);serpent_ecb_dec_8way_avx (ctx->ctx, srcdst, srcdst); srcdst += bsize *SERPENT_PARALLEL_BLOCKS ; nbytes -= bsize *SERPENT_PARALLEL_BLOCKS ; } +serpent_fpu_end_rt (ctx);for (i =0 ; i < nbytes / bsize; i++, srcdst += bsize)__serpent_decrypt (ctx->ctx, srcdst, srcdst);
arch/x86/crypto/serpent_avx_glue.c +19 -4
@@ -218,16 +218,31 @@ struct crypt_priv {bool fpu_enabled; }; +#ifdef CONFIG_PREEMPT_RT_FULL +static void serpent_fpu_end_rt (struct crypt_priv *ctx) +{ +bool fpu_enabled = ctx->fpu_enabled; + +if (!fpu_enabled) +return ; +serpent_fpu_end (fpu_enabled); + ctx->fpu_enabled = false; +} + +#else +static void serpent_fpu_end_rt (struct crypt_priv *ctx) { } +#endif +static void encrypt_callback (void *priv,u8 *srcdst,unsigned int nbytes) {const unsigned int bsize =SERPENT_BLOCK_SIZE ;struct crypt_priv *ctx = priv;int i; - ctx->fpu_enabled =serpent_fpu_begin (ctx->fpu_enabled, nbytes); -if (nbytes == bsize *SERPENT_PARALLEL_BLOCKS ) { + ctx->fpu_enabled =serpent_fpu_begin (ctx->fpu_enabled, nbytes);serpent_ecb_enc_8way_avx (ctx->ctx, srcdst, srcdst); +serpent_fpu_end_rt (ctx);return ; }
@@ -241,10 +256,10 @@ static void decrypt_callback(void *priv,struct crypt_priv *ctx = priv;int i; - ctx->fpu_enabled =serpent_fpu_begin (ctx->fpu_enabled, nbytes); -if (nbytes == bsize *SERPENT_PARALLEL_BLOCKS ) { + ctx->fpu_enabled =serpent_fpu_begin (ctx->fpu_enabled, nbytes);serpent_ecb_dec_8way_avx (ctx->ctx, srcdst, srcdst); +serpent_fpu_end_rt (ctx);return ; }
arch/x86/crypto/serpent_sse2_glue.c +19 -4
@@ -187,16 +187,31 @@ struct crypt_priv {bool fpu_enabled; }; +#ifdef CONFIG_PREEMPT_RT_FULL +static void serpent_fpu_end_rt (struct crypt_priv *ctx) +{ +bool fpu_enabled = ctx->fpu_enabled; + +if (!fpu_enabled) +return ; +serpent_fpu_end (fpu_enabled); + ctx->fpu_enabled = false; +} + +#else +static void serpent_fpu_end_rt (struct crypt_priv *ctx) { } +#endif +static void encrypt_callback (void *priv,u8 *srcdst,unsigned int nbytes) {const unsigned int bsize =SERPENT_BLOCK_SIZE ;struct crypt_priv *ctx = priv;int i; - ctx->fpu_enabled =serpent_fpu_begin (ctx->fpu_enabled, nbytes); -if (nbytes == bsize *SERPENT_PARALLEL_BLOCKS ) { + ctx->fpu_enabled =serpent_fpu_begin (ctx->fpu_enabled, nbytes);serpent_enc_blk_xway (ctx->ctx, srcdst, srcdst); +serpent_fpu_end_rt (ctx);return ; }
@@ -210,10 +225,10 @@ static void decrypt_callback(void *priv,struct crypt_priv *ctx = priv;int i; - ctx->fpu_enabled =serpent_fpu_begin (ctx->fpu_enabled, nbytes); -if (nbytes == bsize *SERPENT_PARALLEL_BLOCKS ) { + ctx->fpu_enabled =serpent_fpu_begin (ctx->fpu_enabled, nbytes);serpent_dec_blk_xway (ctx->ctx, srcdst, srcdst); +serpent_fpu_end_rt (ctx);return ; }
arch/x86/crypto/twofish_avx_glue.c +37 -2
@@ -218,6 +218,33 @@ struct crypt_priv {bool fpu_enabled; }; +#ifdef CONFIG_PREEMPT_RT_FULL +static void twofish_fpu_end_rt (struct crypt_priv *ctx) +{ +bool fpu_enabled = ctx->fpu_enabled; + +if (!fpu_enabled) +return ; +twofish_fpu_end (fpu_enabled); + ctx->fpu_enabled = false; +} + +static void twofish_fpu_sched_rt (struct crypt_priv *ctx) +{ +bool fpu_enabled = ctx->fpu_enabled; + +if (!fpu_enabled || !tif_need_resched_now ()) +return ; +kernel_fpu_end (); +/* schedule due to preemptible */ +kernel_fpu_begin (); +} + +#else +static void twofish_fpu_end_rt (struct crypt_priv *ctx) { } +static void twofish_fpu_sched_rt (struct crypt_priv *ctx) { } +#endif +static void encrypt_callback (void *priv,u8 *srcdst,unsigned int nbytes) {const unsigned int bsize =TF_BLOCK_SIZE ;
@@ -228,12 +255,16 @@ static void encrypt_callback(void *priv,if (nbytes == bsize *TWOFISH_PARALLEL_BLOCKS ) {twofish_ecb_enc_8way (ctx->ctx, srcdst, srcdst); +twofish_fpu_end_rt (ctx);return ; } -for (i =0 ; i < nbytes / (bsize *3 ); i++, srcdst += bsize *3 ) +for (i =0 ; i < nbytes / (bsize *3 ); i++, srcdst += bsize *3 ) { +twofish_fpu_sched_rt (ctx);twofish_enc_blk_3way (ctx->ctx, srcdst, srcdst); + } +twofish_fpu_end_rt (ctx); nbytes %= bsize *3 ;for (i =0 ; i < nbytes / bsize; i++, srcdst += bsize)
@@ -250,11 +281,15 @@ static void decrypt_callback(void *priv,if (nbytes == bsize *TWOFISH_PARALLEL_BLOCKS ) {twofish_ecb_dec_8way (ctx->ctx, srcdst, srcdst); +twofish_fpu_end_rt (ctx);return ; } -for (i =0 ; i < nbytes / (bsize *3 ); i++, srcdst += bsize *3 ) +for (i =0 ; i < nbytes / (bsize *3 ); i++, srcdst += bsize *3 ) { +twofish_fpu_sched_rt (ctx);twofish_dec_blk_3way (ctx->ctx, srcdst, srcdst); + } +twofish_fpu_end_rt (ctx); nbytes %= bsize *3 ;
Quoted 18 lines · Sebastian Andrzej Siewior
arch/x86/crypto/camellia_aesni_avx2_glue.c +12
-0@@ -206,6 +206,34 @@ struct crypt_priv {bool fpu_enabled; }; +#ifdef CONFIG_PREEMPT_RT_FULL +static void camellia_fpu_end_rt (struct crypt_priv *ctx) +{ +bool fpu_enabled = ctx->fpu_enabled; + +if (!fpu_enabled) +return ; +camellia_fpu_end (fpu_enabled); + ctx->fpu_enabled = false; +} + +static void camellia_fpu_sched_rt (struct crypt_priv *ctx)
arch/x86/crypto/camellia_aesni_avx2_glue.c struct crypt_priv { · line 230
Quoted 10 lines
arch/x86/crypto/camellia_aesni_avx2_glue.c +10
-0+{ +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 (); +}
There's a ton of duplication in there; you're not nearly lazy enough.
Why can't we do something simple like kernel_fpu_resched() ?
arch/x86/kernel/fpu/core.c +12 -0
@@ -130,6 +130,18 @@ void kernel_fpu_begin(void) }EXPORT_SYMBOL_GPL (kernel_fpu_begin); +void kernel_fpu_resched (void ) +{ +WARN_ON_FPU (!this_cpu_read (in_kernel_fpu)); + +if (should_resched (PREEMPT_OFFSET )) { +kernel_fpu_end (); +cond_resched (); +kernel_fpu_begin (); + } +} +EXPORT_SYMBOL_GPL (kernel_fpu_resched); +void kernel_fpu_end (void ) {__kernel_fpu_end ();
Quoted 1 line · Peter Zijlstra
On Fri, Dec 01, 2017 at 11:44:22AM +0100, Sebastian Andrzej Siewior wrote:
…
Quoted 22 lines
Review
Quoted 11 lines
(quoted diff) +11
-0+static void camellia_fpu_sched_rt(struct crypt_priv *ctx) +{ + 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(); +}There's a ton of duplication in there; you're not nearly lazy enough.
Why can't we do something simple like
kernel_fpu_resched()?arch/x86/kernel/fpu/core.c +0
-0@@ -130,6 +130,18 @@ void kernel_fpu_begin(void) }
arch/x86/kernel/fpu/core.c void kernel_fpu_begin(void) · line 140
Quoted 10 lines
arch/x86/kernel/fpu/core.c +8
-0EXPORT_SYMBOL_GPL (kernel_fpu_begin); +void kernel_fpu_resched (void ) +{ +WARN_ON_FPU (!this_cpu_read (in_kernel_fpu)); + +if (should_resched (PREEMPT_OFFSET )) { +kernel_fpu_end (); +cond_resched (); +kernel_fpu_begin ();
I can do that but I would still keep it RT only to avoid the kernel_fpu_begin/end to be invoked more often on !RT.
But why that cond_resched()? kernel_fpu_end() ends with preempt_enable() and this one should do the trick.
arch/x86/kernel/fpu/core.c void kernel_fpu_begin(void) · line 147
Quoted 7 lines
arch/x86/kernel/fpu/core.c +4
-0+ } +} +EXPORT_SYMBOL_GPL (kernel_fpu_resched); +void kernel_fpu_end (void ) {__kernel_fpu_end ();
Sebastian
Quoted 8 lines · Sebastian Andrzej Siewior
Quoted 8 lines
arch/x86/kernel/fpu/core.c +0
-0@@ -130,6 +130,18 @@ void kernel_fpu_begin(void) }EXPORT_SYMBOL_GPL (kernel_fpu_begin);
Quoted 13 lines
arch/x86/kernel/fpu/core.c void kernel_fpu_begin(void) · line 140
Quoted 8 lines
arch/x86/kernel/fpu/core.c +8
-0+void kernel_fpu_resched (void ) +{ +WARN_ON_FPU (!this_cpu_read (in_kernel_fpu)); + +if (should_resched (PREEMPT_OFFSET )) { +kernel_fpu_end (); +cond_resched (); +kernel_fpu_begin ();I can do that but I would still keep it RT only to avoid the kernel_fpu_begin/end to be invoked more often on !RT.
But why thatcond_resched()?kernel_fpu_end()ends withpreempt_enable()and this one should do the trick.
!PREEMPT kernels. The above should work for everyone and would allow using 'long' kernel_fpu loops:
kernel_fpu_begin();
while (work) {
do_work();
kernel_fpu_resched();
}
kernel_fpu_end();regardless of preempt setting.
Quoted 11 lines · Peter Zijlstra
!PREEMPT kernels. The above should work for everyone and would allow using 'long'
kernel_fpuloops:kernel_fpu_begin(); while (work) { do_work(); kernel_fpu_resched(); } kernel_fpu_end();regardless of preempt setting.
Ach, wasn't aware that you wanted that explicit for !PREEMPT kernels, too. Okay, so let me pick this hunk up as-is for RT and before I throw it mainline I will add some numbers.
Sebastian
PATCH v3 1/1 crypto: limit more FPU-enabled sections
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 process as much of those as possible in one go. The new *_fpu_sched_rt() schedules only if a RT task is pending.
Probably we should measure the performance those ciphers in pure SW mode and with this optimisations to see if it makes sense to keep them for RT.
This kernel_fpu_resched() makes the code more preemptible which might hurt performance.
--- arch/x86/crypto/camellia_aesni_avx2_glue.c | 20 ++++++++++++++++++++ arch/x86/crypto/camellia_aesni_avx_glue.c | 19 +++++++++++++++++++ arch/x86/crypto/cast6_avx_glue.c | 24 +++++++++++++++++++----- arch/x86/crypto/chacha20_glue.c | 9 +++++---- arch/x86/crypto/serpent_avx2_glue.c | 19 +++++++++++++++++++ arch/x86/crypto/serpent_avx_glue.c | 23 +++++++++++++++++++---- arch/x86/crypto/serpent_sse2_glue.c | 23 +++++++++++++++++++---- arch/x86/crypto/twofish_avx_glue.c | 27 +++++++++++++++++++++++++-- arch/x86/include/asm/fpu/api.h | 1 + arch/x86/kernel/fpu/core.c | 12 ++++++++++++ 10 files changed, 158 insertions(+), 19 deletions(-)
arch/x86/crypto/camellia_aesni_avx2_glue.c +20 -0
@@ -206,6 +206,20 @@ struct crypt_priv {bool fpu_enabled; }; +#ifdef CONFIG_PREEMPT_RT_FULL +static void camellia_fpu_end_rt (struct crypt_priv *ctx) +{ +bool fpu_enabled = ctx->fpu_enabled; + +if (!fpu_enabled) +return ; +camellia_fpu_end (fpu_enabled); + ctx->fpu_enabled = false; +} +#else +static void camellia_fpu_end_rt (struct crypt_priv *ctx) { } +#endif +static void encrypt_callback (void *priv,u8 *srcdst,unsigned int nbytes) {const unsigned int bsize =CAMELLIA_BLOCK_SIZE ;
@@ -221,16 +235,19 @@ static void encrypt_callback(void *priv, }if (nbytes >=CAMELLIA_AESNI_PARALLEL_BLOCKS * bsize) { +kernel_fpu_resched ();camellia_ecb_enc_16way (ctx->ctx, srcdst, srcdst); srcdst += bsize *CAMELLIA_AESNI_PARALLEL_BLOCKS ; nbytes -= bsize *CAMELLIA_AESNI_PARALLEL_BLOCKS ; }while (nbytes >=CAMELLIA_PARALLEL_BLOCKS * bsize) { +kernel_fpu_resched ();camellia_enc_blk_2way (ctx->ctx, srcdst, srcdst); srcdst += bsize *CAMELLIA_PARALLEL_BLOCKS ; nbytes -= bsize *CAMELLIA_PARALLEL_BLOCKS ; } +camellia_fpu_end_rt (ctx);for (i =0 ; i < nbytes / bsize; i++, srcdst += bsize)camellia_enc_blk (ctx->ctx, srcdst, srcdst);
@@ -251,16 +268,19 @@ static void decrypt_callback(void *priv, }if (nbytes >=CAMELLIA_AESNI_PARALLEL_BLOCKS * bsize) { +kernel_fpu_resched ();camellia_ecb_dec_16way (ctx->ctx, srcdst, srcdst); srcdst += bsize *CAMELLIA_AESNI_PARALLEL_BLOCKS ; nbytes -= bsize *CAMELLIA_AESNI_PARALLEL_BLOCKS ; }while (nbytes >=CAMELLIA_PARALLEL_BLOCKS * bsize) { +kernel_fpu_resched ();camellia_dec_blk_2way (ctx->ctx, srcdst, srcdst); srcdst += bsize *CAMELLIA_PARALLEL_BLOCKS ; nbytes -= bsize *CAMELLIA_PARALLEL_BLOCKS ; } +camellia_fpu_end_rt (ctx);for (i =0 ; i < nbytes / bsize; i++, srcdst += bsize)camellia_dec_blk (ctx->ctx, srcdst, srcdst);
arch/x86/crypto/camellia_aesni_avx_glue.c +19 -0
@@ -210,6 +210,21 @@ struct crypt_priv {bool fpu_enabled; }; +#ifdef CONFIG_PREEMPT_RT_FULL +static void camellia_fpu_end_rt (struct crypt_priv *ctx) +{ +bool fpu_enabled = ctx->fpu_enabled; + +if (!fpu_enabled) +return ; +camellia_fpu_end (fpu_enabled); + ctx->fpu_enabled = false; +} + +#else +static void camellia_fpu_end_rt (struct crypt_priv *ctx) { } +#endif +static void encrypt_callback (void *priv,u8 *srcdst,unsigned int nbytes) {const unsigned int bsize =CAMELLIA_BLOCK_SIZE ;
@@ -225,10 +240,12 @@ static void encrypt_callback(void *priv, }while (nbytes >=CAMELLIA_PARALLEL_BLOCKS * bsize) { +kernel_fpu_resched ();camellia_enc_blk_2way (ctx->ctx, srcdst, srcdst); srcdst += bsize *CAMELLIA_PARALLEL_BLOCKS ; nbytes -= bsize *CAMELLIA_PARALLEL_BLOCKS ; } +camellia_fpu_end_rt (ctx);for (i =0 ; i < nbytes / bsize; i++, srcdst += bsize)camellia_enc_blk (ctx->ctx, srcdst, srcdst);
@@ -249,10 +266,12 @@ static void decrypt_callback(void *priv, }while (nbytes >=CAMELLIA_PARALLEL_BLOCKS * bsize) { +kernel_fpu_resched ();camellia_dec_blk_2way (ctx->ctx, srcdst, srcdst); srcdst += bsize *CAMELLIA_PARALLEL_BLOCKS ; nbytes -= bsize *CAMELLIA_PARALLEL_BLOCKS ; } +camellia_fpu_end_rt (ctx);for (i =0 ; i < nbytes / bsize; i++, srcdst += bsize)camellia_dec_blk (ctx->ctx, srcdst, srcdst);
arch/x86/crypto/cast6_avx_glue.c +19 -5
@@ -205,19 +205,33 @@ struct crypt_priv {bool fpu_enabled; }; +#ifdef CONFIG_PREEMPT_RT_FULL +static void cast6_fpu_end_rt (struct crypt_priv *ctx) +{ +bool fpu_enabled = ctx->fpu_enabled; + +if (!fpu_enabled) +return ; +cast6_fpu_end (fpu_enabled); + ctx->fpu_enabled = false; +} + +#else +static void cast6_fpu_end_rt (struct crypt_priv *ctx) { } +#endif +static void encrypt_callback (void *priv,u8 *srcdst,unsigned int nbytes) {const unsigned int bsize =CAST6_BLOCK_SIZE ;struct crypt_priv *ctx = priv;int i; - ctx->fpu_enabled =cast6_fpu_begin (ctx->fpu_enabled, nbytes); -if (nbytes == bsize *CAST6_PARALLEL_BLOCKS ) { + ctx->fpu_enabled =cast6_fpu_begin (ctx->fpu_enabled, nbytes);cast6_ecb_enc_8way (ctx->ctx, srcdst, srcdst); +cast6_fpu_end_rt (ctx);return ; } -for (i =0 ; i < nbytes / bsize; i++, srcdst += bsize)__cast6_encrypt (ctx->ctx, srcdst, srcdst); }
@@ -228,10 +242,10 @@ static void decrypt_callback(void *priv,struct crypt_priv *ctx = priv;int i; - ctx->fpu_enabled =cast6_fpu_begin (ctx->fpu_enabled, nbytes); -if (nbytes == bsize *CAST6_PARALLEL_BLOCKS ) { + ctx->fpu_enabled =cast6_fpu_begin (ctx->fpu_enabled, nbytes);cast6_ecb_dec_8way (ctx->ctx, srcdst, srcdst); +cast6_fpu_end_rt (ctx);return ; }
arch/x86/crypto/chacha20_glue.c +5 -4
@@ -81,23 +81,24 @@ static int chacha20_simd(struct skciphercrypto_chacha20_init (state, ctx, walk.iv); -kernel_fpu_begin (); -while (walk.nbytes >=CHACHA20_BLOCK_SIZE ) { +kernel_fpu_begin (); +chacha20_dosimd (state, walk.dst.virt.addr, walk.src.virt.addr,rounddown (walk.nbytes,CHACHA20_BLOCK_SIZE )); +kernel_fpu_end (); err =skcipher_walk_done (&walk, walk.nbytes %CHACHA20_BLOCK_SIZE ); }if (walk.nbytes) { +kernel_fpu_begin ();chacha20_dosimd (state, walk.dst.virt.addr, walk.src.virt.addr, walk.nbytes); +kernel_fpu_end (); err =skcipher_walk_done (&walk,0 ); } -kernel_fpu_end (); -return err; }
arch/x86/crypto/serpent_avx2_glue.c +19 -0
@@ -184,6 +184,21 @@ struct crypt_priv {bool fpu_enabled; }; +#ifdef CONFIG_PREEMPT_RT_FULL +static void serpent_fpu_end_rt (struct crypt_priv *ctx) +{ +bool fpu_enabled = ctx->fpu_enabled; + +if (!fpu_enabled) +return ; +serpent_fpu_end (fpu_enabled); + ctx->fpu_enabled = false; +} + +#else +static void serpent_fpu_end_rt (struct crypt_priv *ctx) { } +#endif +static void encrypt_callback (void *priv,u8 *srcdst,unsigned int nbytes) {const unsigned int bsize =SERPENT_BLOCK_SIZE ;
@@ -199,10 +214,12 @@ static void encrypt_callback(void *priv, }while (nbytes >=SERPENT_PARALLEL_BLOCKS * bsize) { +kernel_fpu_resched ();serpent_ecb_enc_8way_avx (ctx->ctx, srcdst, srcdst); srcdst += bsize *SERPENT_PARALLEL_BLOCKS ; nbytes -= bsize *SERPENT_PARALLEL_BLOCKS ; } +serpent_fpu_end_rt (ctx);for (i =0 ; i < nbytes / bsize; i++, srcdst += bsize)__serpent_encrypt (ctx->ctx, srcdst, srcdst);
@@ -223,10 +240,12 @@ static void decrypt_callback(void *priv, }while (nbytes >=SERPENT_PARALLEL_BLOCKS * bsize) { +kernel_fpu_resched ();serpent_ecb_dec_8way_avx (ctx->ctx, srcdst, srcdst); srcdst += bsize *SERPENT_PARALLEL_BLOCKS ; nbytes -= bsize *SERPENT_PARALLEL_BLOCKS ; } +serpent_fpu_end_rt (ctx);for (i =0 ; i < nbytes / bsize; i++, srcdst += bsize)__serpent_decrypt (ctx->ctx, srcdst, srcdst);
arch/x86/crypto/serpent_avx_glue.c +19 -4
@@ -218,16 +218,31 @@ struct crypt_priv {bool fpu_enabled; }; +#ifdef CONFIG_PREEMPT_RT_FULL +static void serpent_fpu_end_rt (struct crypt_priv *ctx) +{ +bool fpu_enabled = ctx->fpu_enabled; + +if (!fpu_enabled) +return ; +serpent_fpu_end (fpu_enabled); + ctx->fpu_enabled = false; +} + +#else +static void serpent_fpu_end_rt (struct crypt_priv *ctx) { } +#endif +static void encrypt_callback (void *priv,u8 *srcdst,unsigned int nbytes) {const unsigned int bsize =SERPENT_BLOCK_SIZE ;struct crypt_priv *ctx = priv;int i; - ctx->fpu_enabled =serpent_fpu_begin (ctx->fpu_enabled, nbytes); -if (nbytes == bsize *SERPENT_PARALLEL_BLOCKS ) { + ctx->fpu_enabled =serpent_fpu_begin (ctx->fpu_enabled, nbytes);serpent_ecb_enc_8way_avx (ctx->ctx, srcdst, srcdst); +serpent_fpu_end_rt (ctx);return ; }
@@ -241,10 +256,10 @@ static void decrypt_callback(void *priv,struct crypt_priv *ctx = priv;int i; - ctx->fpu_enabled =serpent_fpu_begin (ctx->fpu_enabled, nbytes); -if (nbytes == bsize *SERPENT_PARALLEL_BLOCKS ) { + ctx->fpu_enabled =serpent_fpu_begin (ctx->fpu_enabled, nbytes);serpent_ecb_dec_8way_avx (ctx->ctx, srcdst, srcdst); +serpent_fpu_end_rt (ctx);return ; }
arch/x86/crypto/serpent_sse2_glue.c +19 -4
@@ -187,16 +187,31 @@ struct crypt_priv {bool fpu_enabled; }; +#ifdef CONFIG_PREEMPT_RT_FULL +static void serpent_fpu_end_rt (struct crypt_priv *ctx) +{ +bool fpu_enabled = ctx->fpu_enabled; + +if (!fpu_enabled) +return ; +serpent_fpu_end (fpu_enabled); + ctx->fpu_enabled = false; +} + +#else +static void serpent_fpu_end_rt (struct crypt_priv *ctx) { } +#endif +static void encrypt_callback (void *priv,u8 *srcdst,unsigned int nbytes) {const unsigned int bsize =SERPENT_BLOCK_SIZE ;struct crypt_priv *ctx = priv;int i; - ctx->fpu_enabled =serpent_fpu_begin (ctx->fpu_enabled, nbytes); -if (nbytes == bsize *SERPENT_PARALLEL_BLOCKS ) { + ctx->fpu_enabled =serpent_fpu_begin (ctx->fpu_enabled, nbytes);serpent_enc_blk_xway (ctx->ctx, srcdst, srcdst); +serpent_fpu_end_rt (ctx);return ; }
@@ -210,10 +225,10 @@ static void decrypt_callback(void *priv,struct crypt_priv *ctx = priv;int i; - ctx->fpu_enabled =serpent_fpu_begin (ctx->fpu_enabled, nbytes); -if (nbytes == bsize *SERPENT_PARALLEL_BLOCKS ) { + ctx->fpu_enabled =serpent_fpu_begin (ctx->fpu_enabled, nbytes);serpent_dec_blk_xway (ctx->ctx, srcdst, srcdst); +serpent_fpu_end_rt (ctx);return ; }
arch/x86/crypto/twofish_avx_glue.c +25 -2
@@ -218,6 +218,21 @@ struct crypt_priv {bool fpu_enabled; }; +#ifdef CONFIG_PREEMPT_RT_FULL +static void twofish_fpu_end_rt (struct crypt_priv *ctx) +{ +bool fpu_enabled = ctx->fpu_enabled; + +if (!fpu_enabled) +return ; +twofish_fpu_end (fpu_enabled); + ctx->fpu_enabled = false; +} + +#else +static void twofish_fpu_end_rt (struct crypt_priv *ctx) { } +#endif +static void encrypt_callback (void *priv,u8 *srcdst,unsigned int nbytes) {const unsigned int bsize =TF_BLOCK_SIZE ;
@@ -228,12 +243,16 @@ static void encrypt_callback(void *priv,if (nbytes == bsize *TWOFISH_PARALLEL_BLOCKS ) {twofish_ecb_enc_8way (ctx->ctx, srcdst, srcdst); +twofish_fpu_end_rt (ctx);return ; } -for (i =0 ; i < nbytes / (bsize *3 ); i++, srcdst += bsize *3 ) +for (i =0 ; i < nbytes / (bsize *3 ); i++, srcdst += bsize *3 ) { +kernel_fpu_resched ();twofish_enc_blk_3way (ctx->ctx, srcdst, srcdst); + } +twofish_fpu_end_rt (ctx); nbytes %= bsize *3 ;for (i =0 ; i < nbytes / bsize; i++, srcdst += bsize)
@@ -250,11 +269,15 @@ static void decrypt_callback(void *priv,if (nbytes == bsize *TWOFISH_PARALLEL_BLOCKS ) {twofish_ecb_dec_8way (ctx->ctx, srcdst, srcdst); +twofish_fpu_end_rt (ctx);return ; } -for (i =0 ; i < nbytes / (bsize *3 ); i++, srcdst += bsize *3 ) +for (i =0 ; i < nbytes / (bsize *3 ); i++, srcdst += bsize *3 ) { +kernel_fpu_resched ();twofish_dec_blk_3way (ctx->ctx, srcdst, srcdst); + } +twofish_fpu_end_rt (ctx); nbytes %= bsize *3 ;
arch/x86/include/asm/fpu/api.h +1 -0
@@ -25,6 +25,7 @@ extern void __kernel_fpu_begin(void);extern void __kernel_fpu_end (void );extern void kernel_fpu_begin (void );extern void kernel_fpu_end (void ); +extern void kernel_fpu_resched (void );extern bool irq_fpu_usable (void );/*
arch/x86/kernel/fpu/core.c +12 -0
@@ -137,6 +137,18 @@ void kernel_fpu_end(void) }EXPORT_SYMBOL_GPL (kernel_fpu_end); +void kernel_fpu_resched (void ) +{ +WARN_ON_FPU (!this_cpu_read (in_kernel_fpu)); + +if (should_resched (PREEMPT_OFFSET )) { +kernel_fpu_end (); +cond_resched (); +kernel_fpu_begin (); + } +} +EXPORT_SYMBOL_GPL (kernel_fpu_resched); +/* * Save the FPU state (mark it for reload if necessary): *
Quoted 11 lines · On Thu, 30 Nov 2017 15:22:17 +0100 Sebastian Andrzej Siewior…
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 usekmap_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/
Quoted 48 lines
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 <[email protected]>arch/x86/crypto/camellia_aesni_avx2_glue.c +36
-0+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
arch/x86/crypto/camellia_aesni_avx2_glue.c struct crypt_priv {
Quoted 10 lines
arch/x86/crypto/camellia_aesni_avx2_glue.c +10
-0+} + +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?
arch/x86/crypto/camellia_aesni_avx2_glue.c struct crypt_priv { · line 234
Quoted 6 lines
arch/x86/crypto/camellia_aesni_avx2_glue.c +6
-0+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
Quoted 105 lines
arch/x86/crypto/serpent_avx2_glue.c +48
-0+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 +} +arch/x86/crypto/serpent_sse2_glue.c +37
-0@@ -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 +} +
Quoted 15 lines · Steven Rostedt
Quoted 15 lines
arch/x86/crypto/camellia_aesni_avx2_glue.c +15
-0+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
Quoted 8 lines
arch/x86/crypto/camellia_aesni_avx2_glue.c struct crypt_priv { · line 228
Quoted 5 lines
arch/x86/crypto/camellia_aesni_avx2_glue.c +5
-0+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?
There is lrw_encrypt() which as "end" function after the end of the full operation. So encrypt_callback() might be invoked multiple times but only the first invocation does the fpu-enable part.
#1 We need to disable-FTP after one invocation of encrypt_callback()
because blkcipher_walk_done() might alloc/free/map memory.
#2 That camellia_fpu_sched_rt() is only RT specific in order to enable
preempt if need-resched is set for a RT task. For !RT we continue.
Quoted 10 lines
arch/x86/crypto/camellia_aesni_avx2_glue.c struct crypt_priv { · line 234
Quoted 6 lines
arch/x86/crypto/camellia_aesni_avx2_glue.c +6
-0+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:
That whole thing is kind of ugly, yes. That *_fpu_end_rt() use a struct crypt_priv but this is defined in each cipher file.
I would keep it for now until we decide if we keep fixing those things are disable for RT because it is not worth it. As I said, this is only x86 specific and we have something similar on ARM with NEON. But I guess the decision here will be postponed until someone posts numbers with SW and with this workaround and cyclictest with a cycle of 1 - 10 ms.
Sebastian
Quoted 6 lines · On Thu, 30 Nov 2017 16:41:08 +0100 Sebastian Andrzej Siewior…
I would keep it for now until we decide if we keep fixing those things are disable for RT because it is not worth it. As I said, this is only x86 specific and we have something similar on ARM with NEON. But I guess the decision here will be postponed until someone posts numbers with SW and with this workaround and cyclictest with a cycle of 1 - 10 ms.
Fair enough.
-- Steve