Re: [PATCH 0/2] crypto: zstd - avoid initializing the workspace twice

[Usama Arif <usama.arif@linux.dev>]

On 25/08/2026 23:27, Yosry Ahmed wrote:
> On Tue, Aug 25, 2026 at 3:06 PM Usama Arif <usama.arif@linux.dev> wrote:
>>
>> Both zstd_compress() and zstd_decompress() set up the shared per-CPU
>> workspace as a C/DStream before walking the request, and then, when the
>> first source and destination fragments each span the whole request, hand
>> off to zstd_compress_one()/zstd_decompress_one(), which immediately
>> overwrite that same ctx->wksp with a CCtx/DCtx. The stream setup is
>> discarded without a byte having been processed.
>>
>> That one-shot path is not a corner case: zswap always takes it when
>> storing, and takes it for a load whenever the stored object lies within a
>> single zsmalloc page.
>>
>> These two patches defer the stream initialization to the first walk
>> iteration that actually streams, guarded by a flag because that iteration
>> can be reached more than once.
>>
>> A 4 KiB crypto_acomp benchmark [1], twelve runs of nine 30,000-operation
>> rounds. Bare metal is an Intel Xeon Platinum 8321HC, turbo off,
>> performance governor, pinned to one core; the VM is a one-vCPU KVM guest
>> on a faster host.
>>
>>                   baseline    patched     delta
>>   bare metal
>>     compress      52,283 ns   51,038 ns   1,245 ns   2.4%
>>     decompress     2,317 ns    1,998 ns     319 ns  13.8%
>>   one-vCPU KVM
>>     compress      16,675 ns   15,050 ns   1,625 ns   9.8%
>>     decompress     3,516 ns    2,265 ns   1,251 ns  35.6%
> 
> Nice!
> 
>>
>> The guest numbers are larger because the two CPUID instructions in
>> ZSTD_cpuid() become unconditional VM exits there.
> 
> I wonder how much benefit we get from just doing CPUID once in
> ZSTD_cpuid() and cache the results. There might still be value in
> avoiding the overall initialization, but I think ZSTD_cpuid() should
> only be executing CPUID once anyway (e.g. in case it's called in other
> paths)?
> 


So I tried something like below and the numbers look really good on a VM:
         compression      decompression
Before   14523ns          2250ns
After    13554ns (-6.7%)  973ns (-56.8%)

commit c169757935b9c4f4d0765227cdf989515f1987a2 (HEAD -> zstd_cpuid_cache)
Author: Usama Arif <usama.arif@linux.dev>
Date:   Wed Aug 26 04:03:35 2026 -0700

    zstd: probe the CPU for BMI2 support only once

    ZSTD_cpuSupportsBmi2() issues CPUID on every context setup for an answer
    that cannot change while the kernel is running.  On x86 that is two
    serializing CPUID instructions, and the callers are not rare:
    squashfs, erofs, btrfs, f2fs and crypto/zstd all initialise a context
    per operation, so a busy squashfs or zswap workload pays for it per
    block or per page.  Under KVM it is worse, because CPUID is an
    unconditional VM exit.

    Cache the result.  Keeping the cache as a single int with a negative
    sentinel, rather than a copy of ZSTD_cpuid_t, keeps it to one word: a
    racing pair of probes computes the same value from the same CPUID leaf,
    so the unsynchronized access is benign, and READ_ONCE()/WRITE_ONCE()
    keep the compiler and KCSAN in agreement about that.

    ZSTD_cpuSupportsBmi2() is MEM_STATIC, so each translation unit that
    inlines it gets its own cache - three in a modular build, plus one in
    each preboot decompressor.  That is a handful of ints in bss and one
    extra probe apiece, not worth avoiding.

    The cached answer is the one the probing CPU reported.  zstd could
    already be migrated between the probe and the use of the flag, so this
    does not introduce a heterogeneity question that was not there before.

    Suggested-by: Yosry Ahmed <yosry@kernel.org>
    Signed-off-by: Usama Arif <usama.arif@linux.dev>

diff --git a/lib/zstd/common/zstd_internal.h b/lib/zstd/common/zstd_internal.h
index 41f190b533209..b179f44753598 100644
--- a/lib/zstd/common/zstd_internal.h
+++ b/lib/zstd/common/zstd_internal.h
@@ -312,8 +312,21 @@ 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);
+    /*
+     * The answer cannot change over the life of the kernel, so probe
+     * once.  Racing probes compute the same value, so the unsynchronized
+     * access is benign; the annotations are there to keep it that way.
+     */
+    static int supported = -1;
+    int s = READ_ONCE(supported);
+
+    if (s < 0) {
+        ZSTD_cpuid_t const cpuid = ZSTD_cpuid();
+
+        s = ZSTD_cpuid_bmi1(cpuid) && ZSTD_cpuid_bmi2(cpuid);
+        WRITE_ONCE(supported, s);
+    }
+    return s;
 #else
     /* Nothing looks at the flag in this configuration. */
     return 0;

>>
>> [1] https://gist.github.com/uarif1/5cf02f0e22c23f0d1b3d84348f12914c
>>
>> Usama Arif (2):
>>   crypto: zstd - Avoid redundant cstream initialization
>>   crypto: zstd - Avoid redundant dstream initialization
>>
>>  crypto/zstd.c | 40 ++++++++++++++++++++++++++++------------
>>  1 file changed, 28 insertions(+), 12 deletions(-)
>>
>>
>> base-commit: 4b18edbd8e70f7e6860d56370f13244896d0f95c
>> --
>> 2.53.0-Meta
>>