mirror of https://lore.kernel.org/lkml/
 help / color / mirror / Atom feed
* [PATCH 0/3] zstd: probe the CPU for BMI2 support once, not per context
@ 2026-08-26 12:25 Usama Arif
  2026-08-26 12:25 ` [PATCH 1/3] zstd: use ZSTD_cpuSupportsBmi2() in ZSTD_initStaticCCtx() Usama Arif
                   ` (4 more replies)
  0 siblings, 5 replies; 8+ messages in thread
From: Usama Arif @ 2026-08-26 12:25 UTC (permalink / raw)
  To: dsterba, linux-kernel, terrelln, linux-crypto, yosry
  Cc: hannes, nphamcs, chengming.zhou, shakeel.butt, kernel-team, Usama Arif

zstd picks between BMI2 and generic code paths at runtime, and issues
CPUID to decide every time a compression or decompression context is set
up. The answer cannot change while the kernel is running.

It is not a cold path: squashfs calls zstd_init_dstream() for every block
it decompresses, and erofs, btrfs, f2fs and crypto/zstd all initialise a
context per operation. Each probe is two serializing CPUID instructions
on x86.

Patch 1 routes ZSTD_initStaticCCtx() through ZSTD_cpuSupportsBmi2()
instead of open-coding the probe, which also fixes it testing for BMI2
without BMI1 - the bodies it selects are tagged
TARGET_ATTRIBUTE("lzcnt,bmi,bmi2"), so both are needed. No CPU in the
field implements BMI2 without BMI1, so this is latent. Patch 2 skips the
probe when DYNAMIC_BMI2 is 0, where every consumer ignores the flag
anyway. Patch 3 caches the result.

A 4 KiB crypto_acomp benchmark [1] in a one-vCPU KVM guest, twelve boots
of nine 30,000-operation rounds, median per-round mean over 108 rounds:

                    compress     decompress
  unpatched         16,756 ns     3,455 ns
  patched           13,646 ns     1,002 ns
                    -3,110 ns    -2,452 ns
                     (18.6%)      (71.0%)

The main reason is because CPUID is an unconditional VM exit.

[1] https://gist.github.com/uarif1/5cf02f0e22c23f0d1b3d84348f12914c
 
Usama Arif (3):
  zstd: use ZSTD_cpuSupportsBmi2() in ZSTD_initStaticCCtx()
  zstd: skip the BMI2 probe when dynamic BMI2 dispatch is disabled
  zstd: probe the CPU for BMI2 support only once

 lib/zstd/common/zstd_internal.h   | 22 ++++++++++++++++++++--
 lib/zstd/compress/zstd_compress.c |  2 +-
 2 files changed, 21 insertions(+), 3 deletions(-)


base-commit: 4b18edbd8e70f7e6860d56370f13244896d0f95c
-- 
2.53.0-Meta


^ permalink raw reply	[flat|nested] 8+ messages in thread
* Re: [PATCH 0/3] zstd: probe the CPU for BMI2 support once, not per context
@ 2026-08-28 20:19 Linus Torvalds
  0 siblings, 0 replies; 8+ messages in thread
From: Linus Torvalds @ 2026-08-28 20:19 UTC (permalink / raw)
  To: Usama Arif
  Cc: Eric Biggers, David Sterba, Linux Kernel Mailing List,
	Nick Terrell, Linux Crypto Mailing List, yosry, Johannes Weiner,
	Nhat Pham, Chengming Zhou, Shakeel Butt, Kernel Team

[ Sorry for breaking threading - I have turned off IMAP access to my
mailbox, and so I have issues replying to lore messages sanely ]

On Thu, 27 Aug 2026, Usama Arif wrote:
>
> On 27/08/2026 03:39, Eric Biggers wrote:
> >
> > Why not just use cpu_feature_enabled(X86_FEATURE_BMI2), which compiles
> > down to a static branch?  All these issues are caused by lib/zstd/ using
> > its own custom CPU feature detection code, instead of the normal CPU
> > feature detection code that the rest of the kernel uses.

Yes, please. The zlib code is just broken in how it makes up its own
random inine asm that is actively worse than what the kernel already
exposes.

> The only issue I saw with that was that zstd is a standalone library that
> is imported

Let's ignore that part, and just make it work right. The Zstd people
should think about this problem on their side.

> Keep the existing raw CPUID fallback for preboot and other builds which
> cannot use the normal x86 feature infrastructure. Also retain the early
> return when dynamic dispatch is disabled.

No, this is not great, that whole

        cctx->bmi2 = ZSTD_cpuSupportsBmi2();
        ...
        if (bmi2) ...

model in zstd needs to just die.

For the kernel, the whole dynamic "test a variable" model is simply wrong.
It should expand to

        if (cpu_feature_enabled(X86_FEATURE_BMI2))

because for the kernel, that becomes a simple static branch.

When zstd goes through that variable, it loses that entirely and
instead turns it in a static assignment and then a dynamic test (well,
not "entirely" - with inlining it could still recover the right code).

So zstd really should be fixed to get rid of that bad

        #if DYNAMIC_BMI2
            if (bmi2) {
         ....

pattern entirely, and be taught to have a *helper* macro that just turns
into 0 for when DYNAMIC_BMI2 is not set, and turns into using that stupid
flag in user mode, and for the kernel it should just turn into that
"cpu_feature_enabled(X86_FEATURE_BMI2)"

Why does it check for both BMI1 and BMI2 anyway? And Arif added an
extra check for ABM. That all looks bogus. You can't have BMI2 without
having BMI1, so all this code looks completely bogus to begin with.

Nick? David?

                 Linus

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2026-08-28 20:20 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-08-26 12:25 [PATCH 0/3] zstd: probe the CPU for BMI2 support once, not per context Usama Arif
2026-08-26 12:25 ` [PATCH 1/3] zstd: use ZSTD_cpuSupportsBmi2() in ZSTD_initStaticCCtx() Usama Arif
2026-08-26 12:25 ` [PATCH 2/3] zstd: skip the BMI2 probe when dynamic BMI2 dispatch is disabled Usama Arif
2026-08-26 12:25 ` [PATCH 3/3] zstd: probe the CPU for BMI2 support only once Usama Arif
2026-08-26 17:10 ` [PATCH 0/3] zstd: probe the CPU for BMI2 support once, not per context Nhat Pham
2026-08-27  2:39 ` Eric Biggers
2026-08-27 14:21   ` Usama Arif
2026-08-28 20:19 Linus Torvalds

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox

all inboxes | Powered by JetHome®