| Message ID | 20190130031444.28935-1-ebiggers@kernel.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | crypto: crct10dif assembly cleanup and optimizations | expand |
On Wed, 30 Jan 2019 at 04:15, Eric Biggers <ebiggers@kernel.org> wrote: > > The x86, arm, and arm64 asm implementations of crct10dif are very > difficult to understand partly because many of the comments, labels, and > macros are named incorrectly: the lengths mentioned are usually off by a > factor of two from the actual code. Many other things are unnecessarily > convoluted as well, e.g. there are many more fold constants than > actually needed and some aren't fully reduced. > > This series therefore cleans up all these implementations to be much > more maintainable. I also made some small optimizations where I saw > opportunities, resulting in slightly better performance. > > This is based on top of the pending patches from Ard Biesheuvel. > > These all pass the new extra self-tests. > Hi Eric, As a FYI, the issue that broke ARM and arm64 with your updated selftests was the 1 byte length special case that you also have special handling for in the x86 version (but while fixing that, I noticed my version was reading beyond the end of the input). I think it hardly matters, though, given the way T10-DIF appears to be used in practice (disk blocks), although it is hard to be sure from reading the code, and the algo should be correct in any case. So what remains is the way these implementations are encapsulated by the crct10dif() library function, which is raster nasty, making CRC-T10DIF an excellent use case to discuss whether we can make any improvements to address some of the concerns that were also raised in the zinc discussion. I threw some code together a while ago [0] (and posted it as well, IIRC). In the mean time, a 'static call' infrastructure is being proposed that could be used in a similar way to avoid function pointers. I'm also interested in hearing opinions on whether the indirect call overhead is actually significant in use cases such as this one.
On Wed, Jan 30, 2019 at 09:33:57AM +0100, Ard Biesheuvel wrote: > > So what remains is the way these implementations are encapsulated by > the crct10dif() library function, which is raster nasty, making > CRC-T10DIF an excellent use case to discuss whether we can make any > improvements to address some of the concerns that were also raised in > the zinc discussion. I threw some code together a while ago [0] (and > posted it as well, IIRC). In the mean time, a 'static call' > infrastructure is being proposed that could be used in a similar way > to avoid function pointers. I'm also interested in hearing opinions on > whether the indirect call overhead is actually significant in use > cases such as this one. I think even if the overhead wasn't significant it would still make sense to make the move just for the sake of simplicity. Thanks,
On Wed, 30 Jan 2019 at 10:13, Herbert Xu <herbert@gondor.apana.org.au> wrote: > > On Wed, Jan 30, 2019 at 09:33:57AM +0100, Ard Biesheuvel wrote: > > > > So what remains is the way these implementations are encapsulated by > > the crct10dif() library function, which is raster nasty, making > > CRC-T10DIF an excellent use case to discuss whether we can make any > > improvements to address some of the concerns that were also raised in > > the zinc discussion. I threw some code together a while ago [0] (and > > posted it as well, IIRC). In the mean time, a 'static call' > > infrastructure is being proposed that could be used in a similar way > > to avoid function pointers. I'm also interested in hearing opinions on > > whether the indirect call overhead is actually significant in use > > cases such as this one. > > I think even if the overhead wasn't significant it would still make > sense to make the move just for the sake of simplicity. > Agreed, we should simplify this if we can. However, my question is whether in this particular case, a simple indirect call via a function pointer is /so/ much worse than a direct call that relies on code patching techniques that are different on every arch (and may rely on objtool or GCC plugins) that the extra complexity is justified.
On Wed, Jan 30, 2019 at 09:33:57AM +0100, Ard Biesheuvel wrote: > On Wed, 30 Jan 2019 at 04:15, Eric Biggers <ebiggers@kernel.org> wrote: > > > > The x86, arm, and arm64 asm implementations of crct10dif are very > > difficult to understand partly because many of the comments, labels, and > > macros are named incorrectly: the lengths mentioned are usually off by a > > factor of two from the actual code. Many other things are unnecessarily > > convoluted as well, e.g. there are many more fold constants than > > actually needed and some aren't fully reduced. > > > > This series therefore cleans up all these implementations to be much > > more maintainable. I also made some small optimizations where I saw > > opportunities, resulting in slightly better performance. > > > > This is based on top of the pending patches from Ard Biesheuvel. > > > > These all pass the new extra self-tests. > > > > Hi Eric, > > As a FYI, the issue that broke ARM and arm64 with your updated > selftests was the 1 byte length special case that you also have > special handling for in the x86 version (but while fixing that, I > noticed my version was reading beyond the end of the input). I think > it hardly matters, though, given the way T10-DIF appears to be used in > practice (disk blocks), although it is hard to be sure from reading > the code, and the algo should be correct in any case. Yes, on second thought I'm thinking the len < 16 support in the x86 assembly isn't worthwhile. Actually it's much slower than the generic table-based code on those lengths due to the overhead of kernel_fpu_begin(). And even if kernel_fpu_begin() were free, the generic code is faster until about len=11. There's a theoretical niceness to using pclmul for all lengths so that no table is needed. But we still need the table for the !irq_fpu_usable() case anyway. So I'll drop the len < 16 case. > > So what remains is the way these implementations are encapsulated by > the crct10dif() library function, which is raster nasty, making > CRC-T10DIF an excellent use case to discuss whether we can make any > improvements to address some of the concerns that were also raised in > the zinc discussion. I threw some code together a while ago [0] (and > posted it as well, IIRC). In the mean time, a 'static call' > infrastructure is being proposed that could be used in a similar way > to avoid function pointers. I'm also interested in hearing opinions on > whether the indirect call overhead is actually significant in use > cases such as this one. > I agree that lib/crc-t10dif.c is very ugly, and we need to find a better way to provide simple crypto library functions. But I'm not sure how to make everyone happy. I actually think the Zinc approach of centrally dispatching to all the software implementations of each algorithm (with one module per algorithm rather than one per implementation) is fine for the vast majority of users. So maybe we should just go with that along with per-implementation knobs so that users can still disable unwanted implementations at build or boot time if they want. E.g., CONFIG_ZINC_CHACHA would be a module that has all the software ChaCha implementations for the architecture. But people building the kernel who do not want or need, say, the NEON implementation could unset the bool CONFIG_ZINC_CHACHA_NEON to exclude it from the zinc_chacha module at build time. Alternatively, users with a precompiled kernel who don't want to use the NEON implementation despite their CPU supporting it could set zinc_chacha.neon=0 on the kernel command line (when CONFIG_ZINC_CHACHA=y) or when loading the zinc_chacha module (when CONFIG_ZINC_CHACHA=m). - Eric