| 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
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