Re: [PATCH V3] selftests: vm: Add test for Soft-Dirty PTE bit

On 2/24/22 2:23 PM, Muhammad Usama Anjum wrote:

This introduces three tests:

1. Sanity check soft dirty basic semantics: allocate area, clean, dirty,

check if the SD bit flipped. 2) Check VMA reuse: validate the VM_SOFTDIRTY usage 3) Check soft-dirty on huge pages

This was motivated by Will Deacon's fix commit 912efa17e512 ("mm: proc: Invalidate TLB after clearing soft-dirty page state"). I was tracking the same issue that he fixed, and this test would have caught it.

Cc: Will Deacon will@kernel.org Signed-off-by: Gabriel Krisman Bertazi krisman@collabora.com Signed-off-by: Muhammad Usama Anjum usama.anjum@collabora.com

---

Changes in V3: Move test to selftests/vm Use kselftest macros Minor updates to make code more maintainable Add configurations in config file

Tests of soft dirty bit in this patch and in selftest/vm/madv_populate.c are non-overlapping.

V2 of this patch: https://lore.kernel.org/lkml/20210603151518.2437813-1-krisman@collabora.com/

---

tools/testing/selftests/vm/.gitignore | 1 + tools/testing/selftests/vm/Makefile | 1 + tools/testing/selftests/vm/config | 2 + tools/testing/selftests/vm/soft-dirty.c | 238 ++++++++++++++++++++++++ 4 files changed, 242 insertions(+) create mode 100644 tools/testing/selftests/vm/soft-dirty.c

diff --git a/tools/testing/selftests/vm/.gitignore b/tools/testing/selftests/vm/.gitignore index d7507f3c7c76..3cb4fa771ec2 100644 --- a/tools/testing/selftests/vm/.gitignore +++ b/tools/testing/selftests/vm/.gitignore @@ -29,5 +29,6 @@ write_to_hugetlbfs hmm-tests memfd_secret local_config.* +soft-dirty split_huge_page_test ksm_tests diff --git a/tools/testing/selftests/vm/Makefile b/tools/testing/selftests/vm/Makefile index 5e43f072f5b7..de9b13d018c3 100644 --- a/tools/testing/selftests/vm/Makefile +++ b/tools/testing/selftests/vm/Makefile @@ -47,6 +47,7 @@ TEST_GEN_FILES += on-fault-limit TEST_GEN_FILES += thuge-gen TEST_GEN_FILES += transhuge-stress TEST_GEN_FILES += userfaultfd +TEST_GEN_FILES += soft-dirty TEST_GEN_FILES += split_huge_page_test TEST_GEN_FILES += ksm_tests diff --git a/tools/testing/selftests/vm/config b/tools/testing/selftests/vm/config index 60e82da0de85..be087c4bc396 100644 --- a/tools/testing/selftests/vm/config +++ b/tools/testing/selftests/vm/config @@ -4,3 +4,5 @@ CONFIG_TEST_VMALLOC=m CONFIG_DEVICE_PRIVATE=y CONFIG_TEST_HMM=m CONFIG_GUP_TEST=y +CONFIG_TRANSPARENT_HUGEPAGE=y +CONFIG_MEM_SOFT_DIRTY=y diff --git a/tools/testing/selftests/vm/soft-dirty.c b/tools/testing/selftests/vm/soft-dirty.c new file mode 100644 index 000000000000..f56c215e0ece --- /dev/null +++ b/tools/testing/selftests/vm/soft-dirty.c @@ -0,0 +1,238 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <stdio.h> +#include <string.h> +#include <stdbool.h> +#include <fcntl.h> +#include <stdint.h> +#include <malloc.h> +#include <sys/mman.h> +#include "../kselftest.h"

+#define PAGEMAP_PATH "/proc/self/pagemap"

Why is this names PATH - it is the file name right?

+#define CLEAR_REFS_PATH "/proc/self/clear_refs"

Same here - why is this named _PATH?

Let's line these defines up - makes it easier to read

+#define SMAP_PATH "/proc/self/smaps"

Same here - why is this named _PATH?

+#define PMD_SIZE_PATH "/sys/kernel/mm/transparent_hugepage/hpage_pmd_size"

Same here - why is this named _PATH?

+#define MAX_LINE_LENGTH 512 +#define TEST_ITERATIONS 10000 +#define PAGE_NUM_TO_TEST 2

+int clear_refs; +int pagemap;

Get rid of these globals and pass these in - please find name that clearly indicates them as fds

+int pagesize; +int mmap_size; /* Size of test region */

Get rid of these globals and pass these in - please find name that clearly indicates

+static void clear_all_refs(void) +{

• const char *ctrl = "4";
• if (write(clear_refs, ctrl, strlen(ctrl)) != strlen(ctrl))

• ksft_exit_fail_msg("%s: failed to clear references\n", __func__);
  

+}

+static void touch_page(char *map, int n) +{

• map[(pagesize * n) + 1]++;

+}

+static int check_page(char *start, int page_num, int clear) +{

• unsigned long pfn = (unsigned long)start / pagesize;
• uint64_t entry;
• int ret;
• ret = pread(pagemap, &entry, sizeof(entry), (pfn + page_num) * sizeof(entry));
• if (ret != sizeof(entry))

• ksft_exit_fail_msg("reading pagemap failed\n");
  

• if (clear)

• clear_all_refs();
  

• return ((entry >> 55) & 1);

Add a define for 55 insead of hardcoding with a meaningful name that describes what this value is.

+}

+static void test_simple(void) +{

• int i;
• char *map;
• map = aligned_alloc(pagesize, mmap_size);
• if (!map)

• ksft_exit_fail_msg("mmap failed\n");
  

• clear_all_refs();

If clear_all_refs() fails and exits, when does map get freed?

• for (i = 0 ; i < TEST_ITERATIONS; i++) {

• if (check_page(map, PAGE_NUM_TO_TEST, 1) == 1) {
  

• 	ksft_print_msg("dirty bit was 1, but should be 0 (i=%d)\n", i);
  

• 	break;
  

• }
  

• touch_page(map, 2);
  

• if (check_page(map, PAGE_NUM_TO_TEST, 1) == 0) {
  

• 	ksft_print_msg("dirty bit was 0, but should be 1 (i=%d)\n", i);
  

• 	break;
  

• }
  

• }
• free(map);
• ksft_test_result(i == TEST_ITERATIONS, "Test %s\n", __func__);

+}

+static void test_vma_reuse(void) +{

• char *map, *map2;
• map = (char *) 0x900000000000;
• map = mmap(map, mmap_size, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANON, -1, 0);
• if (map == MAP_FAILED)

• ksft_exit_fail_msg("mmap failed");
  

• clear_all_refs();
• touch_page(map, PAGE_NUM_TO_TEST);
• munmap(map, mmap_size);
• map2 = mmap(map, mmap_size, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANON, -1, 0);
• if (map2 == MAP_FAILED)

• ksft_exit_fail_msg("mmap failed");
  

• ksft_test_result(map == map2, "Test %s reused memory location\n", __func__);
• ksft_test_result(check_page(map, PAGE_NUM_TO_TEST, 1) != 0,

• 	 "Test %s dirty bit of previous page\n", __func__);
  

• munmap(map2, mmap_size);

+}

+/*

• • read_pmd_pagesize(), check_for_pattern() and check_huge() adapted
• • from 'tools/testing/selftest/vm/split_huge_page_test.c'

Don't use the full path here - just use the file name

• */

+static uint64_t read_pmd_pagesize(void) +{

• int fd;
• char buf[20];
• ssize_t num_read;
• fd = open(PMD_SIZE_PATH, O_RDONLY);
• if (fd == -1)

• ksft_exit_fail_msg("Open hpage_pmd_size failed\n");
  

• num_read = read(fd, buf, 19);
• if (num_read < 1) {

• close(fd);
  

• ksft_exit_fail_msg("Read hpage_pmd_size failed\n");
  

• }
• buf[num_read] = '\0';
• close(fd);
• return strtoul(buf, NULL, 10);

+}

+static bool check_for_pattern(FILE *fp, const char *pattern, char *buf) +{

• while (fgets(buf, MAX_LINE_LENGTH, fp) != NULL) {

• if (!strncmp(buf, pattern, strlen(pattern)))
  

• 	return true;
  

• }
• return false;

+}

+static uint64_t check_huge(void *addr) +{

• uint64_t thp = 0;
• int ret;
• FILE *fp;
• char buffer[MAX_LINE_LENGTH];
• char addr_pattern[MAX_LINE_LENGTH];
• ret = snprintf(addr_pattern, MAX_LINE_LENGTH, "%08lx-",

•        (unsigned long) addr);
  

• if (ret >= MAX_LINE_LENGTH)

• ksft_print_msg("%s: Pattern is too long\n", __func__);
  

Okay. Pattern is too log - does the test continue?

• fp = fopen(SMAP_PATH, "r");
• if (!fp)

• ksft_print_msg("%s: Failed to open file %s\n", __func__, SMAP_PATH);
  

Same commnet about root user

• if (!check_for_pattern(fp, addr_pattern, buffer))

• goto err_out;
  

• /*

• * Fetch the AnonHugePages: in the same block and check the number of
  

• * hugepages.
  

• */
  

• if (!check_for_pattern(fp, "AnonHugePages:", buffer))

• goto err_out;
  

• if (sscanf(buffer, "AnonHugePages:%10ld kB", &thp) != 1)

• ksft_print_msg("Reading smap error\n");
  

+err_out:

• fclose(fp);
• return thp;

+}

+static void test_hugepage(void) +{

• char *map;
• int i, ret;
• size_t hpage_len = read_pmd_pagesize();
• map = memalign(hpage_len, hpage_len);
• if (!map)

• ksft_exit_fail_msg("memalign failed\n");
  

• ret = madvise(map, hpage_len, MADV_HUGEPAGE);
• if (ret)

• ksft_exit_fail_msg("madvise failed %d\n", ret);
  

• for (i = 0; i < hpage_len; i++)

• map[i] = (char)i;
  

• ksft_test_result(check_huge(map), "Test %s huge page allocation\n", __func__);
• clear_all_refs();
• for (i = 0 ; i < TEST_ITERATIONS ; i++) {

• if (check_page(map, PAGE_NUM_TO_TEST, 1) == 1) {
  

• 	ksft_print_msg("dirty bit was 1, but should be 0 (i=%d)\n", i);
  

• 	break;
  

• }
  

• touch_page(map, PAGE_NUM_TO_TEST);
  

• if (check_page(map, PAGE_NUM_TO_TEST, 1) == 0) {
  

• 	ksft_print_msg("dirty bit was 0, but should be 1 (i=%d)\n", i);
  

• 	break;
  

• }
  

• }
• ksft_test_result(i == TEST_ITERATIONS, "Test %s dirty bit\n", __func__);
• munmap(map, mmap_size);

+}

+int main(int argc, char **argv) +{

• ksft_print_header();
• ksft_set_plan(5);
• pagemap = open(PAGEMAP_PATH, O_RDONLY);
• if (pagemap < 0)

• ksft_exit_fail_msg("Failed to open %s\n", PAGEMAP_PATH);
  

Can non-root user open this file? If not, when non-root user fails to open, it is a skip not fail

• clear_refs = open(CLEAR_REFS_PATH, O_WRONLY);
• if (clear_refs < 0)

• ksft_exit_fail_msg("Failed to open %s\n", CLEAR_REFS_PATH);
  

Same comment as above here

Also why would you want to define clear_refs as global? Why not define it here - make it clear that it is fd

• pagesize = getpagesize();

Why is pagesize global?

• mmap_size = 10 * pagesize;

Same here?

• test_simple();
• test_vma_reuse();
• test_hugepage();

What happens when these tests fail?

• return ksft_exit_pass();

+}

Where do CLEAR_REFS_PATH etc. get closed. Please take a look at the error paths carefully. I would like to see the output for this test. Please include it in the change log.

thanks, -- Shuah