Repository navigation
zstd: use x86 feature infrastructure for BMI2 dispatch #4767
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| /* | ||
| * Copyright (c) Meta Platforms, Inc. and affiliates. | ||
| * All rights reserved. | ||
| * | ||
| * This source code is licensed under both the BSD-style license (found in the | ||
| * LICENSE file in the root directory of this source tree) and the GPLv2 (found | ||
| * in the COPYING file in the root directory of this source tree). | ||
| * You may select, at your option, one of the above-listed licenses. | ||
| */ | ||
| #ifndef ASM_CPUFEATURE_H | ||
| #define ASM_CPUFEATURE_H | ||
|
|
||
| #define X86_FEATURE_BMI2 0 | ||
| #define cpu_feature_enabled(feature) ((void)(feature), 0) | ||
|
|
||
| #endif |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,40 @@ | ||
| /* | ||
| * Copyright (c) Meta Platforms, Inc. and affiliates. | ||
| * All rights reserved. | ||
| * | ||
| * This source code is licensed under both the BSD-style license (found in the | ||
| * LICENSE file in the root directory of this source tree) and the GPLv2 (found | ||
| * in the COPYING file in the root directory of this source tree). | ||
| * You may select, at your option, one of the above-listed licenses. | ||
| */ | ||
|
|
||
| #ifndef ZSTD_BMI2_H | ||
| #define ZSTD_BMI2_H | ||
|
|
||
| #include "portability_macros.h" | ||
|
|
||
| #if defined(ZSTD_USE_KERNEL_CPU_FEATURES) | ||
| # include <asm/cpufeature.h> | ||
| #endif | ||
|
|
||
| /* | ||
| * Select between the BMI2 and default implementations at the final dispatch | ||
| * site. Integrations may ignore the caller-provided hint and consult their own | ||
| * CPU-feature policy instead. | ||
| * | ||
| * ZSTD_USE_BMI2() may not evaluate its argument. ZSTD_SET_BMI2() may not | ||
| * evaluate its value argument, which avoids executing the userspace CPUID | ||
| * probe when the integration supplies the dispatch decision. | ||
| */ | ||
| #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) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This guard selects code compiled with Unless the kernel guarantees that
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So this came about because Linus Torvalds raised it on the mailing list in https://lore.kernel.org/all/CAHk-=whZf9=X+qKkO9_pQzbLdOnbMw5yO8gu0TkahegVW3cC-g@mail.gmail.com/ Mainly there is no CPU that has BMI2 but does not have BMI1. If there is a preference to add it, I can do it, but I believe just checking for BMI2 is sufficient. |
||
| # define ZSTD_SET_BMI2(state, value) do { (state) = 0; } while (0) | ||
| #else | ||
| # define ZSTD_USE_BMI2(bmi2) (bmi2) | ||
| # define ZSTD_SET_BMI2(state, value) do { (state) = (value); } while (0) | ||
| #endif | ||
|
|
||
| #endif /* ZSTD_BMI2_H */ | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -102,6 +102,18 @@ | |
| # endif | ||
| #endif | ||
|
|
||
| /* | ||
| * The Linux kernel importer consumes the outer guard while preserving the | ||
| * distinction below. Normal x86 kernel objects can use the kernel CPU-feature | ||
| * policy, while preboot objects cannot use that infrastructure. | ||
| */ | ||
| #if defined(ZSTD_LINUX_KERNEL) | ||
| # if defined(__KERNEL__) && defined(CONFIG_X86) && \ | ||
| !defined(__DISABLE_EXPORTS) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This means that we will continue to do BMI2 detection via cpuid in the pre-boot environment. Is this desired, or should we instead just disable the BMI2 detection entirely in preboot? This might be a better question for on-list discussion. |
||
| # define ZSTD_USE_KERNEL_CPU_FEATURES | ||
| # endif | ||
| #endif | ||
|
|
||
| /** | ||
| * Only enable assembly for GNU C compatible compilers, | ||
| * because other platforms may not support GAS assembly syntax. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this kernel-only path compiled by any CI job?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can check, where are the jobs defined? I think if there is VM infrastructure, it would probably be quite complicated to add a test?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@uarif1 We can test it, at least that all the code compiles as expected
contrib/linux-kernel/test/include/asm/cpufeature.hthat defines the required symbols & macros so thatcpu_feature_enabled(X86_FEATURE_BMI2)has a mock implementation (e.g. just always return0).testincontrib/linux-kernel/test/Makefilethat definesCONFIG_X86so thatZSTD_USE_KERNEL_CPU_FEATURESis enabled.This will then run in our CI.