Messages in this thread | | | Date | Mon, 8 May 2023 17:55:29 -0700 | Subject | Re: [PATCH v6 10/12] crypto: x86/aes - Prepare for a new AES implementation | From | "Chang S. Bae" <> |
| |
On 5/5/2023 4:27 PM, Eric Biggers wrote: > On Mon, Apr 10, 2023 at 03:59:34PM -0700, Chang S. Bae wrote: >> Refactor the common C code to avoid code duplication. The AES-NI code uses >> it with a function pointer argument to call back the AES-NI assembly code. >> So will the AES-KL code. > > Actually, the AES-NI XTS glue code currently makes direct calls to the assembly > code. This patch changes it to make indirect calls. Indirect calls are very > expensive these days, partly due to all the speculative execution mitigations. > So this patch likely causes a performance regression. How about making > xts_crypt_common() and xts_setkey_common() be inline functions?
I guess this series is relevant: https://lore.kernel.org/lkml/20201222160629.22268-1-ardb@kernel.org/#t
Yeah, inlining those looks to be just a cut-and-paste work. Then I was curious about the performance impact.
So I picked one of my old machines. Then, I was able to quickly run through with these cases:
$ cryptsetup benchmark -c aes-xts -s 256
# Tests are approximate using memory only (no storage IO). # Algorithm | Key | Encryption | Decryption
Upstream (6.4-rc1) aes-xts 256b 3949.3 MiB/s 4014.2 MiB/s aes-xts 256b 4016.1 MiB/s 4011.6 MiB/s aes-xts 256b 4026.2 MiB/s 4018.4 MiB/s aes-xts 256b 4009.2 MiB/s 4006.9 MiB/s aes-xts 256b 4025.0 MiB/s 4016.4 MiB/s
Upstream + V6 aes-xts 256b 3876.1 MiB/s 3963.6 MiB/s aes-xts 256b 3926.3 MiB/s 3984.2 MiB/s aes-xts 256b 3940.8 MiB/s 3961.2 MiB/s aes-xts 256b 3929.7 MiB/s 3984.7 MiB/s aes-xts 256b 3892.5 MiB/s 3942.5 MiB/s
Upstream + V6 + {inlined helpers} aes-xts 256b 3996.9 MiB/s 4085.4 MiB/s aes-xts 256b 4087.6 MiB/s 4104.9 MiB/s aes-xts 256b 4117.9 MiB/s 4130.2 MiB/s aes-xts 256b 4098.4 MiB/s 4120.6 MiB/s aes-xts 256b 4095.5 MiB/s 4111.5 MiB/s
Okay, I'm a bit more convinced with this inlining. Will try to comment this along with the change.
> Another issue with having the above be exported symbols is that their names are > too generic, so they could easily collide with another symbols in the kernel. > To be exported symbols, they would need something x86-specific in their names.
I think that's another good point though, they don't need to be exported once moved into the header so that inlined.
>> arch/x86/crypto/Makefile | 2 +- >> arch/x86/crypto/aes-intel_asm.S | 26 ++++ >> arch/x86/crypto/aes-intel_glue.c | 127 ++++++++++++++++ >> arch/x86/crypto/aes-intel_glue.h | 44 ++++++ >> arch/x86/crypto/aesni-intel_asm.S | 58 +++---- >> arch/x86/crypto/aesni-intel_glue.c | 235 +++++++++-------------------- >> arch/x86/crypto/aesni-intel_glue.h | 17 +++ > > It's confusing having aes-intel, aesni-intel, *and* aeskl-intel. Maybe call the > first one "aes-helpers" or "aes-common" instead?
Yeah, I can see a few files named with helper. So, maybe s/aes-intel/aes-helpers/.
>> +struct aes_xts_ctx { >> + u8 raw_tweak_ctx[sizeof(struct crypto_aes_ctx)] AES_ALIGN_ATTR; >> + u8 raw_crypt_ctx[sizeof(struct crypto_aes_ctx)] AES_ALIGN_ATTR; >> +}; > > This struct does not make sense. It should look like: > > struct aes_xts_ctx { > struct crypto_aes_ctx tweak_ctx AES_ALIGN_ATTR; > struct crypto_aes_ctx crypt_ctx AES_ALIGN_ATTR; > }; > > The runtime alignment to a 16-byte boundary should happen when translating the > raw crypto_skcipher_ctx() into the pointer to the aes_xts_ctx. It should not > happen when accessing each individual field in the aes_xts_ctx.
Oh, ugly. This came from mindless copy & paste here. I guess the fix could be a standalone patch. Or, it can be fixed along with this mess.
>> /* >> - * int aesni_set_key(struct crypto_aes_ctx *ctx, const u8 *in_key, >> - * unsigned int key_len) >> + * int _aesni_set_key(struct crypto_aes_ctx *ctx, const u8 *in_key, >> + * unsigned int key_len) >> */ > > It's conventional to use two leading underscores, not one.
Yes, will fix.
Thanks, Chang
| |