On Wed, Feb 26, 2025, Maxim Levitsky wrote:
> Add an option to skip sanity check of number of still idle pages,
> and force it on, in case hypervisor or NUMA balancing
> is detected.
>
> Signed-off-by: Maxim Levitsky <mlevitsk@xxxxxxxxxx>
> ---
> .../selftests/kvm/access_tracking_perf_test.c | 23 +++++++++++++++++--
> .../testing/selftests/kvm/include/test_util.h | 1 +
> tools/testing/selftests/kvm/lib/test_util.c | 22 ++++++++++++++++++
> 3 files changed, 44 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/kvm/access_tracking_perf_test.c b/tools/testing/selftests/kvm/access_tracking_perf_test.c
> index 3c7defd34f56..eafaecf086c4 100644
> --- a/tools/testing/selftests/kvm/access_tracking_perf_test.c
> +++ b/tools/testing/selftests/kvm/access_tracking_perf_test.c
> @@ -65,6 +65,8 @@ static int vcpu_last_completed_iteration[KVM_MAX_VCPUS];
> /* Whether to overlap the regions of memory vCPUs access. */
> static bool overlap_memory_access;
>
> +static bool skip_sanity_check;
I think it makes sense to keep the sanity check, but to warn instead of asserting,
which is already the proposed behavior. I.e. the sanity check is still there,
it's only the assert that's being "skipped". So maybe something like this?
static bool warn_on_too_many_idle_pages;
> +
> struct test_params {
> /* The backing source for the region of memory. */
> enum vm_mem_backing_src_type backing_src;
> @@ -185,7 +187,7 @@ static void mark_vcpu_memory_idle(struct kvm_vm *vm,
> */
> if (still_idle >= pages / 10) {
> #ifdef __x86_64__
Gah, the assert needs to be applied to all architectures. Commit 6336a810db5c
("KVM: selftests: replace assertion with warning in access_tracking_perf_test")
dropped it entirely. It got restored by 8fcee0421386 ("KVM: selftests: Restore
assert for non-nested VMs in access tracking test"), but only for x86.
Maybe do that in a follow-up patch? Or alternatively, introduce the boolean in a
prep patch, then enable the assert for !x86, and then finally add the command
line option to make it controllable.
I'd probably vote for the second option?
> - TEST_ASSERT(this_cpu_has(X86_FEATURE_HYPERVISOR),
> + TEST_ASSERT(skip_sanity_check,
> "vCPU%d: Too many pages still idle (%lu out of %lu)",
> vcpu_idx, still_idle, pages);
> #endif
> @@ -342,6 +344,8 @@ static void help(char *name)
> printf(" -v: specify the number of vCPUs to run.\n");
> printf(" -o: Overlap guest memory accesses instead of partitioning\n"
> " them into a separate region of memory for each vCPU.\n");
> + printf(" -u: Skip check that after dirtying the guest memory, most (90%%) of\n"
Maybe '-w' if we go with something along the lines of "warn_on..."
> + "it is reported as dirty again");
> backing_src_help("-s");
> puts("");
> exit(0);
> @@ -359,7 +363,7 @@ int main(int argc, char *argv[])
>
> guest_modes_append_default();
>
> - while ((opt = getopt(argc, argv, "hm:b:v:os:")) != -1) {
> + while ((opt = getopt(argc, argv, "hm:b:v:os:u")) != -1) {
> switch (opt) {
> case 'm':
> guest_modes_cmdline(optarg);
> @@ -376,6 +380,9 @@ int main(int argc, char *argv[])
> case 's':
> params.backing_src = parse_backing_src_type(optarg);
> break;
> + case 'u':
> + skip_sanity_check = true;
Do we want to allow the user to force the assert? E.g. move the logic that sets
the default value to before the parsing of the command line, and then make this
something like:
warn_on_too_many_idle_pages = atoi_positive(optarg);
break;
> + break;
> case 'h':
> default:
> help(argv[0]);
> @@ -386,6 +393,18 @@ int main(int argc, char *argv[])
> page_idle_fd = open("/sys/kernel/mm/page_idle/bitmap", O_RDWR);
> __TEST_REQUIRE(page_idle_fd >= 0,
> "CONFIG_IDLE_PAGE_TRACKING is not enabled");
> +
Extra newline.
> +
> + if (skip_sanity_check == false) {
if (!skip_sanity_check) {
> + if (this_cpu_has(X86_FEATURE_HYPERVISOR)) {
This needs to be guarded with __x86_64__.
> + printf("Skipping idle page count sanity check, because the test is run nested\n");
> + skip_sanity_check = true;
> + } else if (is_numa_balancing_enabled()) {
> + printf("Skipping idle page count sanity check, because NUMA balance is enabled\n");
> + skip_sanity_check = true;
> + }
> + }
> +
> close(page_idle_fd);
>
> for_each_guest_mode(run_test, ¶ms);
> diff --git a/tools/testing/selftests/kvm/include/test_util.h b/tools/testing/selftests/kvm/include/test_util.h
> index 3e473058849f..1bc9b0a92427 100644
> --- a/tools/testing/selftests/kvm/include/test_util.h
> +++ b/tools/testing/selftests/kvm/include/test_util.h
> @@ -153,6 +153,7 @@ bool is_backing_src_hugetlb(uint32_t i);
> void backing_src_help(const char *flag);
> enum vm_mem_backing_src_type parse_backing_src_type(const char *type_name);
> long get_run_delay(void);
> +bool is_numa_balancing_enabled(void);
>
> /*
> * Whether or not the given source type is shared memory (as opposed to
> diff --git a/tools/testing/selftests/kvm/lib/test_util.c b/tools/testing/selftests/kvm/lib/test_util.c
> index 8ed0b74ae837..1271863613fa 100644
> --- a/tools/testing/selftests/kvm/lib/test_util.c
> +++ b/tools/testing/selftests/kvm/lib/test_util.c
> @@ -163,6 +163,28 @@ size_t get_trans_hugepagesz(void)
> return size;
> }
>
> +
Extra newline.
> +bool is_numa_balancing_enabled(void)
> +{
> + int ret;
> + int val;
> + struct stat statbuf;
> + FILE *f;
> +
> + ret = stat("/proc/sys/kernel/numa_balancing", &statbuf);
> + TEST_ASSERT(ret == 0 || (ret == -1 && errno == ENOENT),
> + "Error in stating /proc/sys/kernel/numa_balancing");
Align indentation.
> + if (ret != 0)
if (ret)
> + return false;
> +
> + f = fopen("/proc/sys/kernel/numa_balancing", "r");
Pretty sure this needs to assert on f being valid.
> + ret = fscanf(f, "%d", &val);
file needs to be closed.
Actually, rather than fix these things, extract the mechanical aspects of the THP
code to helpers (patch at the bottom), and then this can be:
bool is_numa_balancing_enabled(void)
{
const char *numa_balancing = "/proc/sys/kernel/numa_balancing";
return test_sysfs_path(numa_balancing) && get_sysfs_val(numa_balancing);
}
> + TEST_ASSERT(val == 0 || val == 1, "Unexpected value in /proc/sys/kernel/numa_balancing");
I vote to omit this sanity check. The odds of numa_balancing having a different
value *and* the bad value going unnoticed are quite low, so I'd prefer to make
the helper short and simple.
---
From: Sean Christopherson <seanjc@xxxxxxxxxx>
Date: Thu, 27 Feb 2025 07:31:09 -0800
Subject: [PATCH] KVM: selftests: Extract guts of THP accessor to standalone
sysfs helpers
Extract the guts of thp_configured() and get_trans_hugepagesz() to
standalone helpers so that the core logic can be reused for other sysfs
files, e.g. to query numa_balancing.
Opportunistically assert that the initial fscanf() read at least one byte,
and add a comment explaining the second call to fscanf().
Signed-off-by: Sean Christopherson <seanjc@xxxxxxxxxx>
---
tools/testing/selftests/kvm/lib/test_util.c | 35 ++++++++++++++-------
1 file changed, 24 insertions(+), 11 deletions(-)
diff --git a/tools/testing/selftests/kvm/lib/test_util.c b/tools/testing/selftests/kvm/lib/test_util.c
index 8ed0b74ae837..3dc8538f5d69 100644
--- a/tools/testing/selftests/kvm/lib/test_util.c
+++ b/tools/testing/selftests/kvm/lib/test_util.c
@@ -132,37 +132,50 @@ void print_skip(const char *fmt, ...)
puts(", skipping test");
}
-bool thp_configured(void)
+static bool test_sysfs_path(const char *path)
{
- int ret;
struct stat statbuf;
+ int ret;
- ret = stat("/sys/kernel/mm/transparent_hugepage", &statbuf);
+ ret = stat(path, &statbuf);
TEST_ASSERT(ret == 0 || (ret == -1 && errno == ENOENT),
- "Error in stating /sys/kernel/mm/transparent_hugepage");
+ "Error in stat()ing '%s'", path);
return ret == 0;
}
-size_t get_trans_hugepagesz(void)
+bool thp_configured(void)
+{
+ return test_sysfs_path("/sys/kernel/mm/transparent_hugepage");
+}
+
+static size_t get_sysfs_val(const char *path)
{
size_t size;
FILE *f;
int ret;
- TEST_ASSERT(thp_configured(), "THP is not configured in host kernel");
-
- f = fopen("/sys/kernel/mm/transparent_hugepage/hpage_pmd_size", "r");
- TEST_ASSERT(f != NULL, "Error in opening transparent_hugepage/hpage_pmd_size");
+ f = fopen(path, "r");
+ TEST_ASSERT(f, "Error opening '%s'", path);
ret = fscanf(f, "%ld", &size);
+ TEST_ASSERT(ret > 0, "Error reading '%s'", path);
+
+ /* Re-scan the input stream to verify the entire file was read. */
ret = fscanf(f, "%ld", &size);
- TEST_ASSERT(ret < 1, "Error reading transparent_hugepage/hpage_pmd_size");
+ TEST_ASSERT(ret < 1, "Error reading '%s'", path);
+
fclose(f);
-
return size;
}
+size_t get_trans_hugepagesz(void)
+{
+ TEST_ASSERT(thp_configured(), "THP is not configured in host kernel");
+
+ return get_sysfs_val("/sys/kernel/mm/transparent_hugepage/hpage_pmd_size");
+}
+
size_t get_def_hugetlb_pagesz(void)
{
char buf[64];
base-commit: c2b800261f01eda7e811b588eaa6324cfcfdecc6
--
