Re: [PATCH v3 2/2] add ksm test for smart-scan feature

[Petr Vorel <pvorel@suse.cz>]

Hi Stefan,

> This adds a new ksm (kernel samepage merging) test to evaluate the new
> smart scan feature. It allocates a page and fills it with 'a'
> characters. It captures the pages_skipped counter, waits for a few
> iterations and captures the pages_skipped counter again. The expectation
> is that over 50% of the page scans are skipped (There is only one page
> that has KSM enabled and it gets scanned during each iteration and it
> cannot be de-duplicated).

> Signed-off-by: Stefan Roesch <shr@devkernel.io>
> ---
>  runtest/mm                       |   1 +
>  testcases/kernel/mem/.gitignore  |   1 +
>  testcases/kernel/mem/ksm/ksm07.c | 131 +++++++++++++++++++++++++++++++
>  3 files changed, 133 insertions(+)
>  create mode 100644 testcases/kernel/mem/ksm/ksm07.c

> diff --git a/runtest/mm b/runtest/mm
> index f288fed36..d859b331c 100644
> --- a/runtest/mm
> +++ b/runtest/mm
> @@ -70,6 +70,7 @@ ksm05 ksm05 -I 10
>  ksm06 ksm06
>  ksm06_1 ksm06 -n 10
>  ksm06_2 ksm06 -n 8000
> +ksm07 ksm07

>  cpuset01 cpuset01

> diff --git a/testcases/kernel/mem/.gitignore b/testcases/kernel/mem/.gitignore
> index 7258489ed..c96fe8bfc 100644
> --- a/testcases/kernel/mem/.gitignore
> +++ b/testcases/kernel/mem/.gitignore
> @@ -53,6 +53,7 @@
>  /ksm/ksm04
>  /ksm/ksm05
>  /ksm/ksm06
> +/ksm/ksm07
>  /mem/mem02
>  /mmapstress/mmap-corruption01
>  /mmapstress/mmapstress01
> diff --git a/testcases/kernel/mem/ksm/ksm07.c b/testcases/kernel/mem/ksm/ksm07.c
> new file mode 100644
> index 000000000..16153fdb2
> --- /dev/null
> +++ b/testcases/kernel/mem/ksm/ksm07.c
> @@ -0,0 +1,131 @@
> +// SPDX-License-Identifier: GPL-2.0-or-later
> +/*
> + * Copyright (C) 2010-2017  Red Hat, Inc.
> + */
> +/*\
> + * [Description]
> + *
> + * Kernel Samepage Merging (KSM)
> + *
> + * This adds a new ksm (kernel samepage merging) test to evaluate the new
> + * smart scan feature. It allocates a page and fills it with 'a'
> + * characters. It captures the pages_skipped counter, waits for a few
> + * iterations and captures the pages_skipped counter again. The expectation
> + * is that over 50% of the page scans are skipped (There is only one page
> + * that has KSM enabled and it gets scanned during each iteration and it
> + * cannot be de-duplicated).
> + *
> + * Prerequisites:
> + *
> + * 1) ksm and ksmtuned daemons need to be disabled. Otherwise, it could
> + *    distrub the testing as they also change some ksm tunables depends
> + *    on current workloads.
> + */
> +#include <sys/types.h>
> +#include <sys/mman.h>
> +#include <sys/stat.h>
> +#include <sys/wait.h>
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <signal.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <unistd.h>
> +#include "../include/mem.h"
> +#include "ksm_common.h"
> +
> +/* This test allocates one page, fills the page with a's, captures the
> + * full_scan and pages_skipped counters. Then it makes sure at least 3
> + * full scans have been performed and measures the above counters again.
> + * The expectation is that at least 50% of the pages are skipped.
> + *
> + * To wait for at least 3 scans it uses the wait_ksmd_full_scan() function. In
> + * reality, it will be a lot more scans as the wait_ksmd_full_scan() function
> + * sleeps for one second.
> + */
> +static void create_memory(void)
> +{
> +	int status;
> +	int full_scans_begin;
> +	int full_scans_end;
> +	int pages_skipped_begin;
> +	int pages_skipped_end;
> +	int diff_pages;
> +	int diff_scans;
> +	unsigned long page_size;
> +	char *memory;
> +
> +	/* Apply for the space for memory. */
> +	page_size = sysconf(_SC_PAGE_SIZE);
> +	memory = SAFE_MALLOC(page_size);
> +
> +	for (int i = 0; i < 1; i++) {
> +		memory = SAFE_MMAP(NULL, page_size, PROT_READ|PROT_WRITE,
> +			MAP_ANONYMOUS|MAP_PRIVATE, -1, 0);
> +#ifdef HAVE_DECL_MADV_MERGEABLE
> +		if (madvise(memory, page_size, MADV_MERGEABLE) == -1)
> +			tst_brk(TBROK|TERRNO, "madvise");
> +#endif
> +	}
> +	memset(memory, 'a', page_size);
> +
> +	tst_res(TINFO, "KSM merging...");
> +	if (access(PATH_KSM "max_page_sharing", F_OK) == 0) {
> +		SAFE_FILE_PRINTF(PATH_KSM "run", "2");
> +	}
> +
> +	/* Set defalut ksm scan values. */
> +	SAFE_FILE_PRINTF(PATH_KSM "run", "1");
> +	SAFE_FILE_PRINTF(PATH_KSM "pages_to_scan", "%ld", 100l);
> +	SAFE_FILE_PRINTF(PATH_KSM "sleep_millisecs", "0");
> +
> +	/* Measure pages skipped aka "smart scan". */
> +	SAFE_FILE_SCANF(PATH_KSM "full_scans", "%d", &full_scans_begin);
> +	SAFE_FILE_SCANF(PATH_KSM "pages_skipped", "%d", &pages_skipped_begin);
Unfortunately SAFE_FILE_SCANF quits on missing file and
/sys/kernel/mm/ksm/pages_skipped is not on kernel < 6.7.

safe_file_vprintf() which SAFE_FILE_SCANF() internally uses does not support any
flag to quit with TCONF instead TBROK when /sys/kernel/mm/ksm/pages_skipped does
not exists.

We could use access() in setup function, but another line in .save_restore will
help:

{PATH_KSM "pages_skipped", NULL, TST_SR_TCONF}

> +	wait_ksmd_full_scan();
> +
> +	tst_res(TINFO, "stop KSM.");
> +	SAFE_FILE_PRINTF(PATH_KSM "run", "0");
> +
> +	SAFE_FILE_SCANF(PATH_KSM "full_scans", "%d", &full_scans_end);
> +	SAFE_FILE_SCANF(PATH_KSM "pages_skipped", "%d", &pages_skipped_end);
> +	diff_pages = pages_skipped_end - pages_skipped_begin;
> +	diff_scans = full_scans_end - full_scans_begin;

I was going to merge this (with minor cleanup), but the only remaining issue is
that we allow to test run repeatedly via -iN:

# ./ksm07 -i2
tst_test.c:1574: TINFO: Timeout per run is 0h 00m 30s
ksm07.c:73: TINFO: KSM merging...
ksm_helper.c:36: TINFO: ksm daemon takes 1s to run two full scans
ksm07.c:88: TINFO: stop KSM.
ksm07.c:99: TPASS: smart_scan skipped more than 50% of the pages.
ksm07.c:73: TINFO: KSM merging...
ksm_helper.c:36: TINFO: ksm daemon takes 1s to run two full scans
ksm07.c:88: TINFO: stop KSM.
ksm07.c:97: TFAIL: not enough pages have been skipped by smart_scan.

I'm confused, how to reset KSM to be able to run the test more times?

> +
> +	if (diff_pages < diff_scans * 50 / 100) {
> +		tst_res(TFAIL, "not enough pages have been skipped by smart_scan.");
> +	} else {
> +		tst_res(TPASS, "smart_scan skipped more than 50%% of the pages.");
> +	}
> +
> +	while (waitpid(-1, &status, 0) > 0)
> +		if (WEXITSTATUS(status) != 0)
> +			tst_res(TFAIL, "child exit status is %d",
> +					WEXITSTATUS(status));
> +}
This is not needed, done by check_child_status() in
lib/tst_test.c.

> +
> +static void verify_ksm(void)
> +{
> +	create_memory();
> +}
Why this wrapper? "verify_" as a pattern name is not a must.
> +
> +static struct tst_test test = {
> +	.needs_root = 1,
> +	.forks_child = 1,
> +	.options = (struct tst_option[]) {
> +		{}
> +	},
> +	.save_restore = (const struct tst_path_val[]) {
> +		{"/sys/kernel/mm/ksm/run", NULL, TST_SR_TCONF},
> +		{"/sys/kernel/mm/ksm/sleep_millisecs", NULL, TST_SR_TCONF},
> +		{"/sys/kernel/mm/ksm/smart_scan", "1",
We have PATH_KSM definition, lets use it.

> +			TST_SR_SKIP_MISSING | TST_SR_TCONF},
> +		{}
> +	},
> +	.needs_kconfigs = (const char *const[]){
> +		"CONFIG_KSM=y",
> +		NULL
> +	},
> +	.test_all = verify_ksm,
> +};

I was going to merge your patch with following changes.
We just need solve -i2 issue.

Kind regards,
Petr

diff --git testcases/kernel/mem/ksm/ksm07.c testcases/kernel/mem/ksm/ksm07.c
index 16153fdb2..e5c31775b 100644
--- testcases/kernel/mem/ksm/ksm07.c
+++ testcases/kernel/mem/ksm/ksm07.c
@@ -5,35 +5,25 @@
 /*\
  * [Description]
  *
- * Kernel Samepage Merging (KSM)
+ * Kernel Samepage Merging (KSM) for smart scan feature
  *
- * This adds a new ksm (kernel samepage merging) test to evaluate the new
- * smart scan feature. It allocates a page and fills it with 'a'
- * characters. It captures the pages_skipped counter, waits for a few
- * iterations and captures the pages_skipped counter again. The expectation
- * is that over 50% of the page scans are skipped (There is only one page
- * that has KSM enabled and it gets scanned during each iteration and it
- * cannot be de-duplicated).
+ * Test allocates a page and fills it with 'a' characters. It captures the
+ * pages_skipped counter, waits for a few  iterations and captures the
+ * pages_skipped counter again. The expectation  is that over 50% of the page
+ * scans are skipped. (There is only one page that has KSM enabled and it gets
+ * scanned during each iteration and it cannot be de-duplicated.)
  *
- * Prerequisites:
+ * Smart scan feature was added in kernel v6.7.
  *
- * 1) ksm and ksmtuned daemons need to be disabled. Otherwise, it could
- *    distrub the testing as they also change some ksm tunables depends
- *    on current workloads.
+ * [Prerequisites]
+ *
+ * ksm and ksmtuned daemons need to be disabled. Otherwise, it could
+ * distrub the testing as they also change some ksm tunables depends
+ * on current workloads.
  */
-#include <sys/types.h>
-#include <sys/mman.h>
-#include <sys/stat.h>
+
 #include <sys/wait.h>
-#include <errno.h>
-#include <fcntl.h>
-#include <signal.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
-#include <unistd.h>
-#include "../include/mem.h"
-#include "ksm_common.h"
+#include "mem.h"
 
 /* This test allocates one page, fills the page with a's, captures the
  * full_scan and pages_skipped counters. Then it makes sure at least 3
@@ -46,7 +36,6 @@
  */
 static void create_memory(void)
 {
-	int status;
 	int full_scans_begin;
 	int full_scans_end;
 	int pages_skipped_begin;
@@ -70,10 +59,10 @@ static void create_memory(void)
 	}
 	memset(memory, 'a', page_size);
 
-	tst_res(TINFO, "KSM merging...");
-	if (access(PATH_KSM "max_page_sharing", F_OK) == 0) {
+	tst_res(TINFO, "KSM merging");
+
+	if (access(PATH_KSM "max_page_sharing", F_OK) == 0)
 		SAFE_FILE_PRINTF(PATH_KSM "run", "2");
-	}
 
 	/* Set defalut ksm scan values. */
 	SAFE_FILE_PRINTF(PATH_KSM "run", "1");
@@ -85,7 +74,7 @@ static void create_memory(void)
 	SAFE_FILE_SCANF(PATH_KSM "pages_skipped", "%d", &pages_skipped_begin);
 	wait_ksmd_full_scan();
 
-	tst_res(TINFO, "stop KSM.");
+	tst_res(TINFO, "stop KSM");
 	SAFE_FILE_PRINTF(PATH_KSM "run", "0");
 
 	SAFE_FILE_SCANF(PATH_KSM "full_scans", "%d", &full_scans_end);
@@ -93,21 +82,10 @@ static void create_memory(void)
 	diff_pages = pages_skipped_end - pages_skipped_begin;
 	diff_scans = full_scans_end - full_scans_begin;
 
-	if (diff_pages < diff_scans * 50 / 100) {
-		tst_res(TFAIL, "not enough pages have been skipped by smart_scan.");
-	} else {
-		tst_res(TPASS, "smart_scan skipped more than 50%% of the pages.");
-	}
-
-	while (waitpid(-1, &status, 0) > 0)
-		if (WEXITSTATUS(status) != 0)
-			tst_res(TFAIL, "child exit status is %d",
-					WEXITSTATUS(status));
-}
-
-static void verify_ksm(void)
-{
-	create_memory();
+	if (diff_pages < diff_scans * 50 / 100)
+		tst_res(TFAIL, "not enough pages have been skipped by smart_scan");
+	else
+		tst_res(TPASS, "smart_scan skipped more than 50%% of the pages");
 }
 
 static struct tst_test test = {
@@ -117,9 +95,10 @@ static struct tst_test test = {
 		{}
 	},
 	.save_restore = (const struct tst_path_val[]) {
-		{"/sys/kernel/mm/ksm/run", NULL, TST_SR_TCONF},
-		{"/sys/kernel/mm/ksm/sleep_millisecs", NULL, TST_SR_TCONF},
-		{"/sys/kernel/mm/ksm/smart_scan", "1",
+		{PATH_KSM "pages_skipped", NULL, TST_SR_TCONF},
+		{PATH_KSM "run", NULL, TST_SR_TCONF},
+		{PATH_KSM "sleep_millisecs", NULL, TST_SR_TCONF},
+		{PATH_KSM "smart_scan", "1",
 			TST_SR_SKIP_MISSING | TST_SR_TCONF},
 		{}
 	},
@@ -127,5 +106,5 @@ static struct tst_test test = {
 		"CONFIG_KSM=y",
 		NULL
 	},
-	.test_all = verify_ksm,
+	.test_all = create_memory,
 };