| Message ID | 20250607200454.73587-1-ebiggers@kernel.org |
|---|---|
| Headers | show |
| Series | lib/crc: improve how arch-optimized code is integrated | expand |
On Sat, Jun 07, 2025 at 05:47:02PM -0600, Jason A. Donenfeld wrote: > On Sat, Jun 07, 2025 at 01:04:42PM -0700, Eric Biggers wrote: > > Having arch-specific code outside arch/ was somewhat controversial when > > Zinc proposed it back in 2018. But I don't think the concerns are > > warranted. It's better from a technical perspective, as it enables the > > improvements mentioned above. This model is already successfully used > > in other places in the kernel such as lib/raid6/. The community of each > > architecture still remains free to work on the code, even if it's not in > > arch/. At the time there was also a desire to put the library code in > > the same files as the old-school crypto API, but that was a mistake; now > > that the library is separate, that's no longer a constraint either. > > I can't express how happy I am to see this revived. It's clearly the > right way forward and makes it a lot simpler for us to dispatch to > various arch implementations and also is organizationally simpler. > > Jason Thanks! Can I turn that into an Acked-by? - Eric
* Eric Biggers <ebiggers@kernel.org> wrote: > This series is also available at: > > git fetch https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git lib-crc-arch-v2 > > This series improves how lib/crc supports arch-optimized code. First, > instead of the arch-optimized CRC code being in arch/$(SRCARCH)/lib/, it > will now be in lib/crc/$(SRCARCH)/. Second, the API functions (e.g. > crc32c()), arch-optimized functions (e.g. crc32c_arch()), and generic > functions (e.g. crc32c_base()) will now be part of a single module for > each CRC type, allowing better inlining and dead code elimination. The > second change is made possible by the first. > > As an example, consider CONFIG_CRC32=m on x86. We'll now have just > crc32.ko instead of both crc32-x86.ko and crc32.ko. The two modules > were already coupled together and always both got loaded together via > direct symbol dependency, so the separation provided no benefit. > > Note: later I'd like to apply the same design to lib/crypto/ too, where > often the API functions are out-of-line so this will work even better. > In those cases, for each algorithm we currently have 3 modules all > coupled together, e.g. libsha256.ko, libsha256-generic.ko, and > sha256-x86.ko. We should have just one, inline things properly, and > rely on the compiler's dead code elimination to decide the inclusion of > the generic code instead of manually setting it via kconfig. > > Having arch-specific code outside arch/ was somewhat controversial when > Zinc proposed it back in 2018. But I don't think the concerns are > warranted. It's better from a technical perspective, as it enables the > improvements mentioned above. This model is already successfully used > in other places in the kernel such as lib/raid6/. The community of each > architecture still remains free to work on the code, even if it's not in > arch/. At the time there was also a desire to put the library code in > the same files as the old-school crypto API, but that was a mistake; now > that the library is separate, that's no longer a constraint either. > > Changed in v2: > - Fixed build warning on architectures without any optimized CRC code > - Fixed build warning in sparc/crc32.h by removing pr_fmt > - Moved fallback definitions of crc32*_arch back into arch files > - Remove ARCH_HAS_CRC* symbols at end of series instead of beginning, > so that they're not removed until they're no longer being selected > - Slightly improved some commit messages > - Rebased onto other pending lib/crc changes > > Eric Biggers (12): > lib/crc: move files into lib/crc/ > lib/crc: prepare for arch-optimized code in subdirs of lib/crc/ > lib/crc/arm: migrate arm-optimized CRC code into lib/crc/ > lib/crc/arm64: migrate arm64-optimized CRC code into lib/crc/ > lib/crc/loongarch: migrate loongarch-optimized CRC code into lib/crc/ > lib/crc/mips: migrate mips-optimized CRC code into lib/crc/ > lib/crc/powerpc: migrate powerpc-optimized CRC code into lib/crc/ > lib/crc/riscv: migrate riscv-optimized CRC code into lib/crc/ > lib/crc/s390: migrate s390-optimized CRC code into lib/crc/ > lib/crc/sparc: migrate sparc-optimized CRC code into lib/crc/ > lib/crc/x86: migrate x86-optimized CRC code into lib/crc/ > lib/crc: remove ARCH_HAS_* kconfig symbols For the movement of the x86 bits: Acked-by: Ingo Molnar <mingo@kernel.org> > rename {arch/s390/lib => lib/crc/s390}/crc32be-vx.c (100%) > rename {arch/s390/lib => lib/crc/s390}/crc32le-vx.c (100%) > rename arch/sparc/lib/crc32.c => lib/crc/sparc/crc32.h (60%) > rename {arch/sparc/lib => lib/crc/sparc}/crc32c_asm.S (100%) > create mode 100644 lib/crc/tests/Makefile > rename lib/{ => crc}/tests/crc_kunit.c (100%) > rename {arch/x86/lib => lib/crc/x86}/crc-pclmul-consts.h (100%) > rename {arch/x86/lib => lib/crc/x86}/crc-pclmul-template.S (100%) > rename {arch/x86/lib => lib/crc/x86}/crc-pclmul-template.h (100%) > rename arch/x86/lib/crc-t10dif.c => lib/crc/x86/crc-t10dif.h (56%) > rename {arch/x86/lib => lib/crc/x86}/crc16-msb-pclmul.S (100%) > rename {arch/x86/lib => lib/crc/x86}/crc32-pclmul.S (100%) One small namespace suggestion: wouldn't it be better to move the arch support code to lib/crc/arch/, instead of lib/crc/? That way any generic code will stand out better and architecture directories don't crowd out what is supposed to be generic code. Thanks, Ingo
Hi Eric, On Sun, Jun 8, 2025 at 6:07 AM Eric Biggers <ebiggers@kernel.org> wrote: > > This series is also available at: > > git fetch https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git lib-crc-arch-v2 > > This series improves how lib/crc supports arch-optimized code. First, > instead of the arch-optimized CRC code being in arch/$(SRCARCH)/lib/, it > will now be in lib/crc/$(SRCARCH)/. Second, the API functions (e.g. > crc32c()), arch-optimized functions (e.g. crc32c_arch()), and generic > functions (e.g. crc32c_base()) will now be part of a single module for > each CRC type, allowing better inlining and dead code elimination. The > second change is made possible by the first. > > As an example, consider CONFIG_CRC32=m on x86. We'll now have just > crc32.ko instead of both crc32-x86.ko and crc32.ko. The two modules > were already coupled together and always both got loaded together via > direct symbol dependency, so the separation provided no benefit. > > Note: later I'd like to apply the same design to lib/crypto/ too, where > often the API functions are out-of-line so this will work even better. > In those cases, for each algorithm we currently have 3 modules all > coupled together, e.g. libsha256.ko, libsha256-generic.ko, and > sha256-x86.ko. We should have just one, inline things properly, and > rely on the compiler's dead code elimination to decide the inclusion of > the generic code instead of manually setting it via kconfig. > > Having arch-specific code outside arch/ was somewhat controversial when > Zinc proposed it back in 2018. But I don't think the concerns are > warranted. It's better from a technical perspective, as it enables the > improvements mentioned above. This model is already successfully used > in other places in the kernel such as lib/raid6/. The community of each > architecture still remains free to work on the code, even if it's not in > arch/. At the time there was also a desire to put the library code in > the same files as the old-school crypto API, but that was a mistake; now > that the library is separate, that's no longer a constraint either. Quick question, and apologies if this has been covered elsewhere. Why not just use choice blocks in Kconfig to choose the compiled-in crc32 variant instead of this somewhat indirect scheme? This would keep the dependencies grouped by arch and provide a single place to choose whether the generic or arch-specific method is used. It would also allow for alternatives if that ever becomes a thing and compile testing of the arch-specific variants if that even offers any actual value. Thanks,
On Mon, Jun 09, 2025 at 09:40:40AM +0200, Ingo Molnar wrote: > > * Eric Biggers <ebiggers@kernel.org> wrote: > > > This series is also available at: > > > > git fetch https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git lib-crc-arch-v2 > > > > This series improves how lib/crc supports arch-optimized code. First, > > instead of the arch-optimized CRC code being in arch/$(SRCARCH)/lib/, it > > will now be in lib/crc/$(SRCARCH)/. Second, the API functions (e.g. > > crc32c()), arch-optimized functions (e.g. crc32c_arch()), and generic > > functions (e.g. crc32c_base()) will now be part of a single module for > > each CRC type, allowing better inlining and dead code elimination. The > > second change is made possible by the first. > > > > As an example, consider CONFIG_CRC32=m on x86. We'll now have just > > crc32.ko instead of both crc32-x86.ko and crc32.ko. The two modules > > were already coupled together and always both got loaded together via > > direct symbol dependency, so the separation provided no benefit. > > > > Note: later I'd like to apply the same design to lib/crypto/ too, where > > often the API functions are out-of-line so this will work even better. > > In those cases, for each algorithm we currently have 3 modules all > > coupled together, e.g. libsha256.ko, libsha256-generic.ko, and > > sha256-x86.ko. We should have just one, inline things properly, and > > rely on the compiler's dead code elimination to decide the inclusion of > > the generic code instead of manually setting it via kconfig. > > > > Having arch-specific code outside arch/ was somewhat controversial when > > Zinc proposed it back in 2018. But I don't think the concerns are > > warranted. It's better from a technical perspective, as it enables the > > improvements mentioned above. This model is already successfully used > > in other places in the kernel such as lib/raid6/. The community of each > > architecture still remains free to work on the code, even if it's not in > > arch/. At the time there was also a desire to put the library code in > > the same files as the old-school crypto API, but that was a mistake; now > > that the library is separate, that's no longer a constraint either. > > > > Changed in v2: > > - Fixed build warning on architectures without any optimized CRC code > > - Fixed build warning in sparc/crc32.h by removing pr_fmt > > - Moved fallback definitions of crc32*_arch back into arch files > > - Remove ARCH_HAS_CRC* symbols at end of series instead of beginning, > > so that they're not removed until they're no longer being selected > > - Slightly improved some commit messages > > - Rebased onto other pending lib/crc changes > > > > Eric Biggers (12): > > lib/crc: move files into lib/crc/ > > lib/crc: prepare for arch-optimized code in subdirs of lib/crc/ > > lib/crc/arm: migrate arm-optimized CRC code into lib/crc/ > > lib/crc/arm64: migrate arm64-optimized CRC code into lib/crc/ > > lib/crc/loongarch: migrate loongarch-optimized CRC code into lib/crc/ > > lib/crc/mips: migrate mips-optimized CRC code into lib/crc/ > > lib/crc/powerpc: migrate powerpc-optimized CRC code into lib/crc/ > > lib/crc/riscv: migrate riscv-optimized CRC code into lib/crc/ > > lib/crc/s390: migrate s390-optimized CRC code into lib/crc/ > > lib/crc/sparc: migrate sparc-optimized CRC code into lib/crc/ > > lib/crc/x86: migrate x86-optimized CRC code into lib/crc/ > > lib/crc: remove ARCH_HAS_* kconfig symbols > > For the movement of the x86 bits: > > Acked-by: Ingo Molnar <mingo@kernel.org> > > > rename {arch/s390/lib => lib/crc/s390}/crc32be-vx.c (100%) > > rename {arch/s390/lib => lib/crc/s390}/crc32le-vx.c (100%) > > rename arch/sparc/lib/crc32.c => lib/crc/sparc/crc32.h (60%) > > rename {arch/sparc/lib => lib/crc/sparc}/crc32c_asm.S (100%) > > create mode 100644 lib/crc/tests/Makefile > > rename lib/{ => crc}/tests/crc_kunit.c (100%) > > rename {arch/x86/lib => lib/crc/x86}/crc-pclmul-consts.h (100%) > > rename {arch/x86/lib => lib/crc/x86}/crc-pclmul-template.S (100%) > > rename {arch/x86/lib => lib/crc/x86}/crc-pclmul-template.h (100%) > > rename arch/x86/lib/crc-t10dif.c => lib/crc/x86/crc-t10dif.h (56%) > > rename {arch/x86/lib => lib/crc/x86}/crc16-msb-pclmul.S (100%) > > rename {arch/x86/lib => lib/crc/x86}/crc32-pclmul.S (100%) > > One small namespace suggestion: wouldn't it be better to move the arch > support code to lib/crc/arch/, instead of lib/crc/? That way any > generic code will stand out better and architecture directories don't > crowd out what is supposed to be generic code. I don't think that yet another level of directories would provide much value here. The only non-arch subdirectory of lib/crc/ is "tests", so it's not like there are a lot of subdirectories that could be confused with arch names. - Eric
On Mon, Jun 09, 2025 at 06:15:24PM +1000, Julian Calaby wrote: > Hi Eric, > > On Sun, Jun 8, 2025 at 6:07 AM Eric Biggers <ebiggers@kernel.org> wrote: > > > > This series is also available at: > > > > git fetch https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git lib-crc-arch-v2 > > > > This series improves how lib/crc supports arch-optimized code. First, > > instead of the arch-optimized CRC code being in arch/$(SRCARCH)/lib/, it > > will now be in lib/crc/$(SRCARCH)/. Second, the API functions (e.g. > > crc32c()), arch-optimized functions (e.g. crc32c_arch()), and generic > > functions (e.g. crc32c_base()) will now be part of a single module for > > each CRC type, allowing better inlining and dead code elimination. The > > second change is made possible by the first. > > > > As an example, consider CONFIG_CRC32=m on x86. We'll now have just > > crc32.ko instead of both crc32-x86.ko and crc32.ko. The two modules > > were already coupled together and always both got loaded together via > > direct symbol dependency, so the separation provided no benefit. > > > > Note: later I'd like to apply the same design to lib/crypto/ too, where > > often the API functions are out-of-line so this will work even better. > > In those cases, for each algorithm we currently have 3 modules all > > coupled together, e.g. libsha256.ko, libsha256-generic.ko, and > > sha256-x86.ko. We should have just one, inline things properly, and > > rely on the compiler's dead code elimination to decide the inclusion of > > the generic code instead of manually setting it via kconfig. > > > > Having arch-specific code outside arch/ was somewhat controversial when > > Zinc proposed it back in 2018. But I don't think the concerns are > > warranted. It's better from a technical perspective, as it enables the > > improvements mentioned above. This model is already successfully used > > in other places in the kernel such as lib/raid6/. The community of each > > architecture still remains free to work on the code, even if it's not in > > arch/. At the time there was also a desire to put the library code in > > the same files as the old-school crypto API, but that was a mistake; now > > that the library is separate, that's no longer a constraint either. > > Quick question, and apologies if this has been covered elsewhere. > > Why not just use choice blocks in Kconfig to choose the compiled-in > crc32 variant instead of this somewhat indirect scheme? > > This would keep the dependencies grouped by arch and provide a single place to > choose whether the generic or arch-specific method is used. It's not clear exactly what you're suggesting, but it sounds like you're complaining about this: config CRC32_ARCH bool depends on CRC32 && CRC_OPTIMIZATIONS default y if ARM && KERNEL_MODE_NEON default y if ARM64 default y if LOONGARCH default y if MIPS && CPU_MIPSR6 default y if PPC64 && ALTIVEC default y if RISCV && RISCV_ISA_ZBC default y if S390 default y if SPARC64 default y if X86 We could instead make each arch be responsible for selecting this from lib/crc/$(SRCARCH)/Kconfig, which lib/crc/Kconfig would then have to include. But I don't think the small bit of additional per-arch separation would be worth the extra complexity here. Something similar applies to lib/crc/Makefile too. This patchset strikes a balance where the vast majority of the arch-specific CRC code is isolated in lib/crc/$(SRCARCH), and the exceptions are just lib/crc/Makefile and lib/crc/Kconfig. I think these exceptions make sense, given that we're building a single module per CRC variant. We'd have to go through some hoops to isolate the arch-specific Kconfig and Makefile snippets into per-arch files, which don't seem worth it here IMO. > It would also allow for alternatives if that ever becomes a thing and If you mean one arch with multiple alternative implementations of a particular CRC variant, that already exists for many of the architectures. They just build in as many as can be, and the best one is chosen at boot or module load time. But that's existing behavior, unchanged by this patchset. > compile testing of the arch-specific variants if that even offers any > actual value. They all use instructions specific to the corresponding arch, so I don't think any of them would be compatible with COMPILE_TEST. - Eric
Hi Eric, On Tue, Jun 10, 2025 at 5:49 AM Eric Biggers <ebiggers@kernel.org> wrote: > > On Mon, Jun 09, 2025 at 06:15:24PM +1000, Julian Calaby wrote: > > Hi Eric, > > > > On Sun, Jun 8, 2025 at 6:07 AM Eric Biggers <ebiggers@kernel.org> wrote: > > > > > > This series is also available at: > > > > > > git fetch https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git lib-crc-arch-v2 > > > > > > This series improves how lib/crc supports arch-optimized code. First, > > > instead of the arch-optimized CRC code being in arch/$(SRCARCH)/lib/, it > > > will now be in lib/crc/$(SRCARCH)/. Second, the API functions (e.g. > > > crc32c()), arch-optimized functions (e.g. crc32c_arch()), and generic > > > functions (e.g. crc32c_base()) will now be part of a single module for > > > each CRC type, allowing better inlining and dead code elimination. The > > > second change is made possible by the first. > > > > > > As an example, consider CONFIG_CRC32=m on x86. We'll now have just > > > crc32.ko instead of both crc32-x86.ko and crc32.ko. The two modules > > > were already coupled together and always both got loaded together via > > > direct symbol dependency, so the separation provided no benefit. > > > > > > Note: later I'd like to apply the same design to lib/crypto/ too, where > > > often the API functions are out-of-line so this will work even better. > > > In those cases, for each algorithm we currently have 3 modules all > > > coupled together, e.g. libsha256.ko, libsha256-generic.ko, and > > > sha256-x86.ko. We should have just one, inline things properly, and > > > rely on the compiler's dead code elimination to decide the inclusion of > > > the generic code instead of manually setting it via kconfig. > > > > > > Having arch-specific code outside arch/ was somewhat controversial when > > > Zinc proposed it back in 2018. But I don't think the concerns are > > > warranted. It's better from a technical perspective, as it enables the > > > improvements mentioned above. This model is already successfully used > > > in other places in the kernel such as lib/raid6/. The community of each > > > architecture still remains free to work on the code, even if it's not in > > > arch/. At the time there was also a desire to put the library code in > > > the same files as the old-school crypto API, but that was a mistake; now > > > that the library is separate, that's no longer a constraint either. > > > > Quick question, and apologies if this has been covered elsewhere. > > > > Why not just use choice blocks in Kconfig to choose the compiled-in > > crc32 variant instead of this somewhat indirect scheme? > > > > This would keep the dependencies grouped by arch and provide a single place to > > choose whether the generic or arch-specific method is used. > > It's not clear exactly what you're suggesting, but it sounds like you're > complaining about this: > > config CRC32_ARCH > bool > depends on CRC32 && CRC_OPTIMIZATIONS > default y if ARM && KERNEL_MODE_NEON > default y if ARM64 > default y if LOONGARCH > default y if MIPS && CPU_MIPSR6 > default y if PPC64 && ALTIVEC > default y if RISCV && RISCV_ISA_ZBC > default y if S390 > default y if SPARC64 > default y if X86 I was suggesting something roughly like: choice prompt "CRC32 Variant" depends on CRC32 && CRC_OPTIMIZATIONS config CRC32_ARCH_ARM_NEON bool "ARM NEON" default y depends ARM && KERNEL_MODE_NEON ... config CRC32_GENERIC bool "Generic" endchoice > This patchset strikes a balance where the vast majority of the arch-specific CRC > code is isolated in lib/crc/$(SRCARCH), and the exceptions are just > lib/crc/Makefile and lib/crc/Kconfig. I think these exceptions make sense, > given that we're building a single module per CRC variant. We'd have to go > through some hoops to isolate the arch-specific Kconfig and Makefile snippets > into per-arch files, which don't seem worth it here IMO. I was only really concerned with the Kconfig structure, I was expecting Kbuild to look roughly like this: (filenames are wrong) crc32-y += crc32-base.o crc32-$(CRC32_ARCH_ARM_NEON) += arch/arm/crc32-neon.o ... crc32-$(CRC32_GENERIC) += crc32-generic.o but yeah, your proposal here has grown on me now that I think about it and the only real "benefit" mine has is that architectures can display choices for variants that have Kconfig-visible requirements, which probably isn't that many so it wouldn't be useful in practice. Thanks for answering my question,
On Tue, Jun 10, 2025 at 08:36:39AM +1000, Julian Calaby wrote: > Hi Eric, > > On Tue, Jun 10, 2025 at 5:49 AM Eric Biggers <ebiggers@kernel.org> wrote: > > > > On Mon, Jun 09, 2025 at 06:15:24PM +1000, Julian Calaby wrote: > > > Hi Eric, > > > > > > On Sun, Jun 8, 2025 at 6:07 AM Eric Biggers <ebiggers@kernel.org> wrote: > > > > > > > > This series is also available at: > > > > > > > > git fetch https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git lib-crc-arch-v2 > > > > > > > > This series improves how lib/crc supports arch-optimized code. First, > > > > instead of the arch-optimized CRC code being in arch/$(SRCARCH)/lib/, it > > > > will now be in lib/crc/$(SRCARCH)/. Second, the API functions (e.g. > > > > crc32c()), arch-optimized functions (e.g. crc32c_arch()), and generic > > > > functions (e.g. crc32c_base()) will now be part of a single module for > > > > each CRC type, allowing better inlining and dead code elimination. The > > > > second change is made possible by the first. > > > > > > > > As an example, consider CONFIG_CRC32=m on x86. We'll now have just > > > > crc32.ko instead of both crc32-x86.ko and crc32.ko. The two modules > > > > were already coupled together and always both got loaded together via > > > > direct symbol dependency, so the separation provided no benefit. > > > > > > > > Note: later I'd like to apply the same design to lib/crypto/ too, where > > > > often the API functions are out-of-line so this will work even better. > > > > In those cases, for each algorithm we currently have 3 modules all > > > > coupled together, e.g. libsha256.ko, libsha256-generic.ko, and > > > > sha256-x86.ko. We should have just one, inline things properly, and > > > > rely on the compiler's dead code elimination to decide the inclusion of > > > > the generic code instead of manually setting it via kconfig. > > > > > > > > Having arch-specific code outside arch/ was somewhat controversial when > > > > Zinc proposed it back in 2018. But I don't think the concerns are > > > > warranted. It's better from a technical perspective, as it enables the > > > > improvements mentioned above. This model is already successfully used > > > > in other places in the kernel such as lib/raid6/. The community of each > > > > architecture still remains free to work on the code, even if it's not in > > > > arch/. At the time there was also a desire to put the library code in > > > > the same files as the old-school crypto API, but that was a mistake; now > > > > that the library is separate, that's no longer a constraint either. > > > > > > Quick question, and apologies if this has been covered elsewhere. > > > > > > Why not just use choice blocks in Kconfig to choose the compiled-in > > > crc32 variant instead of this somewhat indirect scheme? > > > > > > This would keep the dependencies grouped by arch and provide a single place to > > > choose whether the generic or arch-specific method is used. > > > > It's not clear exactly what you're suggesting, but it sounds like you're > > complaining about this: > > > > config CRC32_ARCH > > bool > > depends on CRC32 && CRC_OPTIMIZATIONS > > default y if ARM && KERNEL_MODE_NEON > > default y if ARM64 > > default y if LOONGARCH > > default y if MIPS && CPU_MIPSR6 > > default y if PPC64 && ALTIVEC > > default y if RISCV && RISCV_ISA_ZBC > > default y if S390 > > default y if SPARC64 > > default y if X86 > > I was suggesting something roughly like: > > choice > prompt "CRC32 Variant" > depends on CRC32 && CRC_OPTIMIZATIONS > > config CRC32_ARCH_ARM_NEON > bool "ARM NEON" > default y > depends ARM && KERNEL_MODE_NEON > > ... > > config CRC32_GENERIC > bool "Generic" > > endchoice > > > This patchset strikes a balance where the vast majority of the arch-specific CRC > > code is isolated in lib/crc/$(SRCARCH), and the exceptions are just > > lib/crc/Makefile and lib/crc/Kconfig. I think these exceptions make sense, > > given that we're building a single module per CRC variant. We'd have to go > > through some hoops to isolate the arch-specific Kconfig and Makefile snippets > > into per-arch files, which don't seem worth it here IMO. > > I was only really concerned with the Kconfig structure, I was > expecting Kbuild to look roughly like this: (filenames are wrong) > > crc32-y += crc32-base.o > crc32-$(CRC32_ARCH_ARM_NEON) += arch/arm/crc32-neon.o > ... > crc32-$(CRC32_GENERIC) += crc32-generic.o > > but yeah, your proposal here has grown on me now that I think about it > and the only real "benefit" mine has is that architectures can display > choices for variants that have Kconfig-visible requirements, which > probably isn't that many so it wouldn't be useful in practice. > > Thanks for answering my question, The CRC32 implementation did used to be user-selectable, but that was already removed in v6.14 (except for the coarse-grained knob CONFIG_CRC_OPTIMIZATIONS that remains and can be disabled only when CONFIG_EXPERT=y) since the vast majority of users simply want the optimized CRC32 code enabled. The fact that it wasn't just enabled by default was a longstanding bug. - Eric
On Tue, Jun 10, 2025 at 11:39:22AM -0600, Jason A. Donenfeld wrote: > On Sun, Jun 08, 2025 at 04:48:17PM -0700, Eric Biggers wrote: > > On Sat, Jun 07, 2025 at 05:47:02PM -0600, Jason A. Donenfeld wrote: > > > On Sat, Jun 07, 2025 at 01:04:42PM -0700, Eric Biggers wrote: > > > > Having arch-specific code outside arch/ was somewhat controversial when > > > > Zinc proposed it back in 2018. But I don't think the concerns are > > > > warranted. It's better from a technical perspective, as it enables the > > > > improvements mentioned above. This model is already successfully used > > > > in other places in the kernel such as lib/raid6/. The community of each > > > > architecture still remains free to work on the code, even if it's not in > > > > arch/. At the time there was also a desire to put the library code in > > > > the same files as the old-school crypto API, but that was a mistake; now > > > > that the library is separate, that's no longer a constraint either. > > > > > > I can't express how happy I am to see this revived. It's clearly the > > > right way forward and makes it a lot simpler for us to dispatch to > > > various arch implementations and also is organizationally simpler. > > > > > > Jason > > > > Thanks! Can I turn that into an Acked-by? > > Took me a little while longer to fully review it. Sure, > > Acked-by: Jason A. Donenfeld <Jason@zx2c4.com> > > Side note: I wonder about eventually turning some of the static branches > into static calls. Yes, Linus was wondering the same thing earlier. It does run into a couple issues. First, only x86 and powerpc implement static_call properly; everywhere else it's just an indirect call. Second, there's often some code shared above the level at which we'd like to do the dispatch. For example, consider crc32_le on x86. If we expand the CRC_PCLMUL macro and inline crc32_le_arch and crc32_le_base as the compiler does, crc32_le ends up as: u32 crc32_le(u32 crc, const u8 *p, size_t len) { if (len >= 16 && static_branch_likely(&have_pclmulqdq) && crypto_simd_usable()) { const void *consts_ptr; consts_ptr = crc32_lsb_0xedb88320_consts.fold_across_128_bits_consts; kernel_fpu_begin(); crc = static_call(crc32_lsb_pclmul)(crc, p, len, consts_ptr); kernel_fpu_end(); return crc; } while (len--) crc = (crc >> 8) ^ crc32table_le[(crc & 255) ^ *p++]; return crc; } The existing static_call selects between 3 different assembly functions, all of which require a kernel-mode FPU section and only support len >= 16. We could instead unconditionally do a static_call upon entry to the function, with 4 possible targets. But then we'd have to duplicate the kernel FPU begin/end sequence in 3 different functions. Also, it would add an extra function call for the case where 'len < 16', which is a common case and is exactly the case where per-call overhead matters the most. However, if we could actually inline the static call into the *callers* of crc32_le(), that would make it more worthwhile. I'm not sure that's possible, though, especially considering that this code is tristate. Anyway, this is tangential to this patchset. Though the new way the code is organized does make it more feasible to have e.g. a centralized static_call in the future if we choose to go in that direction. - Eric
On Sat, Jun 07, 2025 at 01:04:51PM -0700, Eric Biggers wrote: > From: Eric Biggers <ebiggers@google.com> > > Move the s390-optimized CRC code from arch/s390/lib/crc* into its new > location in lib/crc/s390/, and wire it up in the new way. This new way > of organizing the CRC code eliminates the need to artificially split the > code for each CRC variant into separate arch and generic modules, > enabling better inlining and dead code elimination. For more details, > see "lib/crc: prepare for arch-optimized code in subdirs of lib/crc/". > > Signed-off-by: Eric Biggers <ebiggers@google.com> ... Hi Eric, With this series I am getting on s390: alg: hash: skipping comparison tests for crc32c-s390 because crc32c-generic is unavailable Thanks!
On Fri, Jun 13, 2025 at 06:01:41PM +0200, Alexander Gordeev wrote: > On Sat, Jun 07, 2025 at 01:04:51PM -0700, Eric Biggers wrote: > > From: Eric Biggers <ebiggers@google.com> > > > > Move the s390-optimized CRC code from arch/s390/lib/crc* into its new > > location in lib/crc/s390/, and wire it up in the new way. This new way > > of organizing the CRC code eliminates the need to artificially split the > > code for each CRC variant into separate arch and generic modules, > > enabling better inlining and dead code elimination. For more details, > > see "lib/crc: prepare for arch-optimized code in subdirs of lib/crc/". > > > > Signed-off-by: Eric Biggers <ebiggers@google.com> > ... > > Hi Eric, > > With this series I am getting on s390: > > alg: hash: skipping comparison tests for crc32c-s390 because crc32c-generic is unavailable > > Thanks! I think that's actually from "crypto/crc32c: register only one shash_alg" (https://lore.kernel.org/linux-crypto/20250601224441.778374-3-ebiggers@kernel.org/), not the patch you replied to. Those self-test warnings are expected. But I guess they are going to confuse people, so we should do something to make them go away. I think we should do what I've proposed for SHA-512: stop worrying about setting the cra_driver_name to something meaningful (which has never really worked anyway), instead just use *-lib, and update crypto/testmgr.c accordingly. I'll send out patches that do that. - Eric
This series is also available at: git fetch https://git.kernel.org/pub/scm/linux/kernel/git/ebiggers/linux.git lib-crc-arch-v2 This series improves how lib/crc supports arch-optimized code. First, instead of the arch-optimized CRC code being in arch/$(SRCARCH)/lib/, it will now be in lib/crc/$(SRCARCH)/. Second, the API functions (e.g. crc32c()), arch-optimized functions (e.g. crc32c_arch()), and generic functions (e.g. crc32c_base()) will now be part of a single module for each CRC type, allowing better inlining and dead code elimination. The second change is made possible by the first. As an example, consider CONFIG_CRC32=m on x86. We'll now have just crc32.ko instead of both crc32-x86.ko and crc32.ko. The two modules were already coupled together and always both got loaded together via direct symbol dependency, so the separation provided no benefit. Note: later I'd like to apply the same design to lib/crypto/ too, where often the API functions are out-of-line so this will work even better. In those cases, for each algorithm we currently have 3 modules all coupled together, e.g. libsha256.ko, libsha256-generic.ko, and sha256-x86.ko. We should have just one, inline things properly, and rely on the compiler's dead code elimination to decide the inclusion of the generic code instead of manually setting it via kconfig. Having arch-specific code outside arch/ was somewhat controversial when Zinc proposed it back in 2018. But I don't think the concerns are warranted. It's better from a technical perspective, as it enables the improvements mentioned above. This model is already successfully used in other places in the kernel such as lib/raid6/. The community of each architecture still remains free to work on the code, even if it's not in arch/. At the time there was also a desire to put the library code in the same files as the old-school crypto API, but that was a mistake; now that the library is separate, that's no longer a constraint either. Changed in v2: - Fixed build warning on architectures without any optimized CRC code - Fixed build warning in sparc/crc32.h by removing pr_fmt - Moved fallback definitions of crc32*_arch back into arch files - Remove ARCH_HAS_CRC* symbols at end of series instead of beginning, so that they're not removed until they're no longer being selected - Slightly improved some commit messages - Rebased onto other pending lib/crc changes Eric Biggers (12): lib/crc: move files into lib/crc/ lib/crc: prepare for arch-optimized code in subdirs of lib/crc/ lib/crc/arm: migrate arm-optimized CRC code into lib/crc/ lib/crc/arm64: migrate arm64-optimized CRC code into lib/crc/ lib/crc/loongarch: migrate loongarch-optimized CRC code into lib/crc/ lib/crc/mips: migrate mips-optimized CRC code into lib/crc/ lib/crc/powerpc: migrate powerpc-optimized CRC code into lib/crc/ lib/crc/riscv: migrate riscv-optimized CRC code into lib/crc/ lib/crc/s390: migrate s390-optimized CRC code into lib/crc/ lib/crc/sparc: migrate sparc-optimized CRC code into lib/crc/ lib/crc/x86: migrate x86-optimized CRC code into lib/crc/ lib/crc: remove ARCH_HAS_* kconfig symbols Documentation/core-api/kernel-api.rst | 14 +-- MAINTAINERS | 4 +- arch/arm/Kconfig | 2 - arch/arm/lib/Makefile | 6 - arch/arm64/Kconfig | 2 - arch/arm64/lib/Makefile | 6 - arch/loongarch/Kconfig | 1 - arch/loongarch/lib/Makefile | 2 - arch/mips/Kconfig | 1 - arch/mips/lib/Makefile | 2 - arch/powerpc/Kconfig | 2 - arch/powerpc/lib/Makefile | 6 - arch/riscv/Kconfig | 3 - arch/riscv/lib/Makefile | 6 - arch/s390/Kconfig | 1 - arch/s390/lib/Makefile | 3 - arch/sparc/Kconfig | 1 - arch/sparc/lib/Makefile | 2 - arch/x86/Kconfig | 3 - arch/x86/lib/Makefile | 10 -- include/linux/crc-t10dif.h | 10 +- include/linux/crc32.h | 30 +---- include/linux/crc64.h | 22 +--- lib/Kconfig | 87 +------------ lib/Kconfig.debug | 21 ---- lib/Makefile | 32 +---- lib/crc/.gitignore | 5 + lib/crc/Kconfig | 118 ++++++++++++++++++ lib/crc/Makefile | 63 ++++++++++ .../arm/lib => lib/crc/arm}/crc-t10dif-core.S | 0 .../crc-t10dif.c => lib/crc/arm/crc-t10dif.h | 23 +--- {arch/arm/lib => lib/crc/arm}/crc32-core.S | 0 arch/arm/lib/crc32.c => lib/crc/arm/crc32.h | 38 ++---- .../lib => lib/crc/arm64}/crc-t10dif-core.S | 0 .../crc/arm64/crc-t10dif.h | 22 +--- .../arm64/lib => lib/crc/arm64}/crc32-core.S | 0 .../lib/crc32.c => lib/crc/arm64/crc32.h | 19 +-- lib/{ => crc}/crc-ccitt.c | 3 - lib/{ => crc}/crc-itu-t.c | 0 lib/{crc-t10dif.c => crc/crc-t10dif-main.c} | 37 ++++-- lib/{ => crc}/crc16.c | 0 lib/{crc32.c => crc/crc32-main.c} | 69 ++++++++-- lib/{ => crc}/crc4.c | 0 lib/{crc64.c => crc/crc64-main.c} | 47 +++++-- lib/{ => crc}/crc7.c | 0 lib/{ => crc}/crc8.c | 0 lib/{ => crc}/gen_crc32table.c | 4 +- lib/{ => crc}/gen_crc64table.c | 11 +- .../crc/loongarch/crc32.h | 32 +---- .../lib/crc32-mips.c => lib/crc/mips/crc32.h | 33 +---- .../crc/powerpc/crc-t10dif.h | 20 +-- .../crc/powerpc}/crc-vpmsum-template.S | 0 .../lib/crc32.c => lib/crc/powerpc/crc32.h | 38 ++---- .../crc/powerpc}/crc32c-vpmsum_asm.S | 0 .../crc/powerpc}/crct10dif-vpmsum_asm.S | 0 .../lib => lib/crc/riscv}/crc-clmul-consts.h | 0 .../crc/riscv}/crc-clmul-template.h | 0 {arch/riscv/lib => lib/crc/riscv}/crc-clmul.h | 0 .../crc/riscv/crc-t10dif.h | 8 +- {arch/riscv/lib => lib/crc/riscv}/crc16_msb.c | 0 .../lib/crc32.c => lib/crc/riscv/crc32.h | 17 +-- {arch/riscv/lib => lib/crc/riscv}/crc32_lsb.c | 0 {arch/riscv/lib => lib/crc/riscv}/crc32_msb.c | 0 .../lib/crc64.c => lib/crc/riscv/crc64.h | 11 +- {arch/riscv/lib => lib/crc/riscv}/crc64_lsb.c | 0 {arch/riscv/lib => lib/crc/riscv}/crc64_msb.c | 0 {arch/s390/lib => lib/crc/s390}/crc32-vx.h | 0 arch/s390/lib/crc32.c => lib/crc/s390/crc32.h | 16 +-- {arch/s390/lib => lib/crc/s390}/crc32be-vx.c | 0 {arch/s390/lib => lib/crc/s390}/crc32le-vx.c | 0 .../lib/crc32.c => lib/crc/sparc/crc32.h | 42 ++----- .../sparc/lib => lib/crc/sparc}/crc32c_asm.S | 0 lib/crc/tests/Makefile | 2 + lib/{ => crc}/tests/crc_kunit.c | 0 .../lib => lib/crc/x86}/crc-pclmul-consts.h | 0 .../lib => lib/crc/x86}/crc-pclmul-template.S | 0 .../lib => lib/crc/x86}/crc-pclmul-template.h | 0 .../crc-t10dif.c => lib/crc/x86/crc-t10dif.h | 18 +-- .../lib => lib/crc/x86}/crc16-msb-pclmul.S | 0 {arch/x86/lib => lib/crc/x86}/crc32-pclmul.S | 0 arch/x86/lib/crc32.c => lib/crc/x86/crc32.h | 30 +---- {arch/x86/lib => lib/crc/x86}/crc32c-3way.S | 0 {arch/x86/lib => lib/crc/x86}/crc64-pclmul.S | 0 arch/x86/lib/crc64.c => lib/crc/x86/crc64.h | 21 +--- lib/tests/Makefile | 1 - 85 files changed, 406 insertions(+), 621 deletions(-) create mode 100644 lib/crc/.gitignore create mode 100644 lib/crc/Kconfig create mode 100644 lib/crc/Makefile rename {arch/arm/lib => lib/crc/arm}/crc-t10dif-core.S (100%) rename arch/arm/lib/crc-t10dif.c => lib/crc/arm/crc-t10dif.h (70%) rename {arch/arm/lib => lib/crc/arm}/crc32-core.S (100%) rename arch/arm/lib/crc32.c => lib/crc/arm/crc32.h (69%) rename {arch/arm64/lib => lib/crc/arm64}/crc-t10dif-core.S (100%) rename arch/arm64/lib/crc-t10dif.c => lib/crc/arm64/crc-t10dif.h (70%) rename {arch/arm64/lib => lib/crc/arm64}/crc32-core.S (100%) rename arch/arm64/lib/crc32.c => lib/crc/arm64/crc32.h (81%) rename lib/{ => crc}/crc-ccitt.c (98%) rename lib/{ => crc}/crc-itu-t.c (100%) rename lib/{crc-t10dif.c => crc/crc-t10dif-main.c} (78%) rename lib/{ => crc}/crc16.c (100%) rename lib/{crc32.c => crc/crc32-main.c} (58%) rename lib/{ => crc}/crc4.c (100%) rename lib/{crc64.c => crc/crc64-main.c} (66%) rename lib/{ => crc}/crc7.c (100%) rename lib/{ => crc}/crc8.c (100%) rename lib/{ => crc}/gen_crc32table.c (95%) rename lib/{ => crc}/gen_crc64table.c (81%) rename arch/loongarch/lib/crc32-loongarch.c => lib/crc/loongarch/crc32.h (71%) rename arch/mips/lib/crc32-mips.c => lib/crc/mips/crc32.h (82%) rename arch/powerpc/lib/crc-t10dif.c => lib/crc/powerpc/crc-t10dif.h (75%) rename {arch/powerpc/lib => lib/crc/powerpc}/crc-vpmsum-template.S (100%) rename arch/powerpc/lib/crc32.c => lib/crc/powerpc/crc32.h (64%) rename {arch/powerpc/lib => lib/crc/powerpc}/crc32c-vpmsum_asm.S (100%) rename {arch/powerpc/lib => lib/crc/powerpc}/crct10dif-vpmsum_asm.S (100%) rename {arch/riscv/lib => lib/crc/riscv}/crc-clmul-consts.h (100%) rename {arch/riscv/lib => lib/crc/riscv}/crc-clmul-template.h (100%) rename {arch/riscv/lib => lib/crc/riscv}/crc-clmul.h (100%) rename arch/riscv/lib/crc-t10dif.c => lib/crc/riscv/crc-t10dif.h (62%) rename {arch/riscv/lib => lib/crc/riscv}/crc16_msb.c (100%) rename arch/riscv/lib/crc32.c => lib/crc/riscv/crc32.h (66%) rename {arch/riscv/lib => lib/crc/riscv}/crc32_lsb.c (100%) rename {arch/riscv/lib => lib/crc/riscv}/crc32_msb.c (100%) rename arch/riscv/lib/crc64.c => lib/crc/riscv/crc64.h (65%) rename {arch/riscv/lib => lib/crc/riscv}/crc64_lsb.c (100%) rename {arch/riscv/lib => lib/crc/riscv}/crc64_msb.c (100%) rename {arch/s390/lib => lib/crc/s390}/crc32-vx.h (100%) rename arch/s390/lib/crc32.c => lib/crc/s390/crc32.h (81%) rename {arch/s390/lib => lib/crc/s390}/crc32be-vx.c (100%) rename {arch/s390/lib => lib/crc/s390}/crc32le-vx.c (100%) rename arch/sparc/lib/crc32.c => lib/crc/sparc/crc32.h (60%) rename {arch/sparc/lib => lib/crc/sparc}/crc32c_asm.S (100%) create mode 100644 lib/crc/tests/Makefile rename lib/{ => crc}/tests/crc_kunit.c (100%) rename {arch/x86/lib => lib/crc/x86}/crc-pclmul-consts.h (100%) rename {arch/x86/lib => lib/crc/x86}/crc-pclmul-template.S (100%) rename {arch/x86/lib => lib/crc/x86}/crc-pclmul-template.h (100%) rename arch/x86/lib/crc-t10dif.c => lib/crc/x86/crc-t10dif.h (56%) rename {arch/x86/lib => lib/crc/x86}/crc16-msb-pclmul.S (100%) rename {arch/x86/lib => lib/crc/x86}/crc32-pclmul.S (100%) rename arch/x86/lib/crc32.c => lib/crc/x86/crc32.h (76%) rename {arch/x86/lib => lib/crc/x86}/crc32c-3way.S (100%) rename {arch/x86/lib => lib/crc/x86}/crc64-pclmul.S (100%) rename arch/x86/lib/crc64.c => lib/crc/x86/crc64.h (61%) base-commit: 464e77d8417003dfa3b0f556eba0297169483249