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 [off-list ref] wrote:

quoted

On Mon, Jun 09, 2025 at 06:15:24PM +1000, Julian Calaby wrote:

quoted

Hi Eric,

On Sun, Jun 8, 2025 at 6:07 AM Eric Biggers [off-list ref] wrote:

quoted

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

quoted

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