Re: [PATCH v3 2/2] zstd: use cpu_feature_enabled() for in-kernel BMI2 dispatch

[Usama Arif]

On 02/09/2026 18:40, Dhruva G wrote:
> Hi Usama,
> 
> On 01-09-2026 16:37, Usama Arif wrote:
>> Zstd's dynamic BMI2 implementation probes CPUID when a compression or
>> decompression context is initialized, stores the result in the context,
>> and tests that value at every dispatch site. In normal kernel builds this
>> bypasses the x86 feature policy and uses ordinary runtime branches instead
>> of allowing x86 alternatives to resolve the feature check at boot.
>>
>> Add ZSTD_USE_BMI2() and use it at every runtime BMI2/default selector. For
>> normal x86 kernel objects, the predicate expands directly to
>> cpu_feature_enabled(X86_FEATURE_BMI2). When dynamic BMI2 dispatch is not
>> available it is false; other builds retain the caller-provided flag. Keep
>> the existing HUF conditional layout because DYNAMIC_BMI2 also controls
>> whether target-attributed variants are emitted.
>>
>> Add the matching ZSTD_SET_BMI2() abstraction for context initialization.
>> Normal x86 kernel objects and builds without dynamic BMI2 do not cache CPU
>> state. Other builds with dynamic dispatch, including preboot, retain the
>> existing behavior. Keep the BMI2 members in the context structures so
>> their layouts do not change, and make the accessors return zero when
>> cached state is unused.
>>
>> Select the normal-kernel policy in zstd_deps.h. Builds which define
>> __DISABLE_EXPORTS, including the x86 preboot decompressor, retain the existing
>> CPUID-backed dispatch because the normal alternatives infrastructure is not
>> available there.
>>
>> A 4 KiB zstd-generic crypto_acomp benchmark in a one-vCPU KVM guest gave
>> these median results:
>>
>>                         Before      After     Change
>>   Compression      16,634 ns/op  13,394 ns/op  -19.5%
>>   Decompression     3,480 ns/op     963 ns/op  -72.3%
>>
>> Signed-off-by: Usama Arif <usama.arif@xxxxxxxxx>
>> ---
> 
> These comments still say BMI2 support is determined once per context:
> - lib/zstd/compress/zstd_compress_internal.h:473
> - lib/zstd/decompress/zstd_decompress_internal.h:159

Good catch! Thanks. I will change to:

Cached for per-context dispatch: 1 if the CPU supports BMI2, 0 otherwise.

> 
>>  lib/zstd/common/compiler.h                    | 12 ++++++++++++
>>  lib/zstd/common/entropy_common.c              | 12 ++++++------
>>  lib/zstd/common/fse_decompress.c              |  7 ++++---
>>  lib/zstd/common/zstd_deps.h                   |  5 +++++
>>  lib/zstd/compress/huf_compress.c              |  6 +++++-
>>  lib/zstd/compress/zstd_compress.c             | 12 ++++++------
>>  lib/zstd/compress/zstd_compress_internal.h    |  9 +++++++++
>>  lib/zstd/compress/zstd_compress_sequences.c   |  8 +++++---
>>  lib/zstd/compress/zstd_compress_superblock.c  |  2 +-
>>  lib/zstd/decompress/huf_decompress.c          | 18 +++++++++---------
>>  lib/zstd/decompress/zstd_decompress.c         |  4 +---
>>  lib/zstd/decompress/zstd_decompress_block.c   | 19 +++++++------------
>>  .../decompress/zstd_decompress_internal.h     |  2 +-
>>  13 files changed, 71 insertions(+), 45 deletions(-)
>>
>> diff --git a/lib/zstd/common/compiler.h b/lib/zstd/common/compiler.h
>> index dc9bd15e174e9..47f6c0372c58c 100644
>> --- a/lib/zstd/common/compiler.h
>> +++ b/lib/zstd/common/compiler.h
>> @@ -14,6 +14,7 @@
>>  
>>  #include <linux/types.h>
>>  
>> +#include "zstd_deps.h"
>>  #include "portability_macros.h"
>>  
>>  /*-*******************************************************
>> @@ -96,6 +97,17 @@
>>   */
>>  #define BMI2_TARGET_ATTRIBUTE TARGET_ATTRIBUTE("lzcnt,bmi,bmi2")
>>  
>> +#if !DYNAMIC_BMI2
>> +#  define ZSTD_USE_BMI2(bmi2) 0
>> +#  define ZSTD_SET_BMI2(state, value) do { } while (0)
>> +#elif defined(ZSTD_USE_KERNEL_CPU_FEATURES)
>> +#  define ZSTD_USE_BMI2(bmi2) cpu_feature_enabled(X86_FEATURE_BMI2)
> 
> Here, we do not include <asm/cpufeature.h>. Instead, every current .c user includes that header separately.
> This works today, but it maybe fragile: the next user of ZSTD_USE_BMI2() can fail to compile unless they know
> about this hidden requirement. 
> Do you think perhaps we should provide that here in this header itself?

I tried that, but compiler.h is included by unrelated zstd translation units.
On x86, <asm/cpufeature.h> eventually includes <asm/current.h>, which defines
current as get_current(). This breaks the existing local variable named current 
in zstd_double_fast.c.

I think the current apporach is ok?

> 
> With that, feel free to add
> 
> Reviewed-by: Dhruva Gole <goledhruva@xxxxxxxxx>

Thanks for the review!