Re: [PATCH 0/3] zstd: probe the CPU for BMI2 support once, not per context

Re: [PATCH 0/3] zstd: probe the CPU for BMI2 support once, not per context

[Linus Torvalds]

[ 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

Re: [PATCH 0/3] zstd: probe the CPU for BMI2 support once, not per context

[Usama Arif]

On 28/08/2026 21:19, Linus Torvalds wrote:
> [ 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.
> 

The three checks came from interpreting BMI2_TARGET_ATTRIBUTE("lzcnt,bmi,bmi2")
as requiring all three feature bits. Agreed that this is unnecessary here.
The kernel path should check only X86_FEATURE_BMI2.

I was also trying to preserve zstd’s standalone-library behavior.
I will instead make normal x86 kernel builds use a helper that expands directly to
cpu_feature_enabled(X86_FEATURE_BMI2) at each dispatch site. The stored BMI2
context flag and private CPUID probe will be removed from that path.
Preboot and standalone builds will retain their existing caller-provided 
lag and CPUID-based detection.

I will send a revised patch in a little bit.

Thanks!
Usama

[PATCH 0/3] zstd: probe the CPU for BMI2 support once, not per context

[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

Re: [PATCH 0/3] zstd: probe the CPU for BMI2 support once, not per context

[Nhat Pham]

On Wed, Aug 26, 2026 at 5:26 AM Usama Arif <usama.arif@linux.dev> wrote:
>
> 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

Awesome! I'm no zstd expert, but this looks very nice. Thanks for
fixing it. I'll defer mostly to Nick (is the fb.com email still
working?) and Herbert for correctness checking.

BTW, I think this has been reported in the past:

https://lore.kernel.org/all/CAJxJ_jhvyMukPLThpgcdCMHwbp3b3bFvc4Va1cK79_3z6ubhwQ@mail.gmail.com/

Re: [PATCH 0/3] zstd: probe the CPU for BMI2 support once, not per context

[Eric Biggers]

On Wed, Aug 26, 2026 at 05:25:35AM -0700, Usama Arif wrote:
> 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(-)

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.

- Eric

Re: [PATCH 0/3] zstd: probe the CPU for BMI2 support once, not per context

[Usama Arif]

On 27/08/2026 03:39, Eric Biggers wrote:
> On Wed, Aug 26, 2026 at 05:25:35AM -0700, Usama Arif wrote:
>> 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(-)
> 
> 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.
> 
> - Eric


The only issue I saw with that was that zstd is a standalone library that
is imported, so I am not sure how using cpu_feature_enabled() would work
for the maintainers. If the maintainers are happy with it, I think the
below patch is much better.


From 0b21d945f579cbb5e41b92b4c9306e9d5bac84f1 Mon Sep 17 00:00:00 2001
From: Usama Arif <usama.arif@linux.dev>
Date: Thu, 27 Aug 2026 05:34:41 -0700
Subject: [PATCH] zstd: use kernel CPU feature detection on x86

Zstd currently probes CPUID while initializing each compression or
decompression context. This bypasses the x86 feature policy used by the
rest of the kernel and repeats a serializing instruction sequence for
every context.

Use cpu_feature_enabled() in the normal x86 compressor and decompressor
objects. Check ABM, BMI1, and BMI2 because the dispatched functions are
compiled with lzcnt, bmi, and bmi2. The checks are alternatives-patched
at boot.

The effect is especially visible under virtualization, where CPUID
normally causes a VM exit. A 4 KiB crypto_acomp benchmark was run in
one-vCPU KVM guests. Comparing the unpatched baseline with this
three-patch series, the median reported ns/op values were:

                         before    after
  compression            16,777   13,635 ns/op  (-18.7%)
  decompression           3,454    1,006 ns/op  (-70.9%)

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.

Signed-off-by: Usama Arif <usama.arif@linux.dev>
---
 lib/zstd/Makefile               |  5 +++++
 lib/zstd/common/zstd_internal.h | 18 ++++++++++++++----
 2 files changed, 19 insertions(+), 4 deletions(-)

diff --git a/lib/zstd/Makefile b/lib/zstd/Makefile
index be218b5e0ed59..db1d1439f5447 100644
--- a/lib/zstd/Makefile
+++ b/lib/zstd/Makefile
@@ -42,3 +42,8 @@ zstd_common-y := \
 		common/error_private.o \
 		common/fse_decompress.o \
 		common/zstd_common.o \
+
+ifeq ($(CONFIG_X86),y)
+CFLAGS_compress/zstd_compress.o += -DZSTD_USE_KERNEL_CPU_FEATURES
+CFLAGS_decompress/zstd_decompress.o += -DZSTD_USE_KERNEL_CPU_FEATURES
+endif
diff --git a/lib/zstd/common/zstd_internal.h b/lib/zstd/common/zstd_internal.h
index 41f190b533209..f7d3bca650747 100644
--- a/lib/zstd/common/zstd_internal.h
+++ b/lib/zstd/common/zstd_internal.h
@@ -31,6 +31,11 @@
 #include "fse.h"
 #include "huf.h"
 #include <linux/xxhash.h>                /* XXH_reset, update, digest */
+
+/* Use the kernel's CPU feature policy in normal x86 kernel builds. */
+#if defined(ZSTD_USE_KERNEL_CPU_FEATURES)
+#include <asm/cpufeature.h>
+#endif
 #define ZSTD_TRACE 0
 
 /* ---- static assert (debug) --- */
@@ -311,12 +316,17 @@ size_t ZSTD_decodeSeqHeaders(ZSTD_DCtx* dctx, int* nbSeqPtr,
  */
 MEM_STATIC int ZSTD_cpuSupportsBmi2(void)
 {
-#if DYNAMIC_BMI2
-    ZSTD_cpuid_t cpuid = ZSTD_cpuid();
-    return ZSTD_cpuid_bmi1(cpuid) && ZSTD_cpuid_bmi2(cpuid);
-#else
+#if !DYNAMIC_BMI2
     /* Nothing looks at the flag in this configuration. */
     return 0;
+#elif defined(ZSTD_USE_KERNEL_CPU_FEATURES)
+    return cpu_feature_enabled(X86_FEATURE_ABM) &&
+           cpu_feature_enabled(X86_FEATURE_BMI1) &&
+           cpu_feature_enabled(X86_FEATURE_BMI2);
+#else
+    ZSTD_cpuid_t cpuid = ZSTD_cpuid();
+    return ZSTD_cpuid_bmi1(cpuid) &&
+           ZSTD_cpuid_bmi2(cpuid);
 #endif
 }
 
-- 
2.53.0-Meta