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 <[email protected]>
Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
Signed-off-by: Muhammad Usama Anjum <[email protected]>
---
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/[email protected]/
---
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"
+#define CLEAR_REFS_PATH "/proc/self/clear_refs"
+#define SMAP_PATH "/proc/self/smaps"
+#define PMD_SIZE_PATH "/sys/kernel/mm/transparent_hugepage/hpage_pmd_size"
+#define MAX_LINE_LENGTH 512
+#define TEST_ITERATIONS 10000
+#define PAGE_NUM_TO_TEST 2
+
+int clear_refs;
+int pagemap;
+
+int pagesize;
+int mmap_size; /* Size of test region */
+
+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);
+}
+
+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();
+
+ 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'
+ */
+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__);
+
+ fp = fopen(SMAP_PATH, "r");
+ if (!fp)
+ ksft_print_msg("%s: Failed to open file %s\n", __func__, SMAP_PATH);
+
+ 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);
+
+ clear_refs = open(CLEAR_REFS_PATH, O_WRONLY);
+ if (clear_refs < 0)
+ ksft_exit_fail_msg("Failed to open %s\n", CLEAR_REFS_PATH);
+
+ pagesize = getpagesize();
+ mmap_size = 10 * pagesize;
+
+ test_simple();
+ test_vma_reuse();
+ test_hugepage();
+
+ return ksft_exit_pass();
+}
--
2.30.2
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 <[email protected]>
> Signed-off-by: Gabriel Krisman Bertazi <[email protected]>
> Signed-off-by: Muhammad Usama Anjum <[email protected]>
> ---
> 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/[email protected]/
> ---
> 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
Hi,
Andrew had already accepted the patch. I'll send an iteration.
On 2/26/22 5:35 AM, Shuah Khan wrote:
>> +#define PAGEMAP_PATH "/proc/self/pagemap"
>
> Why is this names PATH - it is the file name right?
I'll update the names of the macros.
>> +
>> +int clear_refs;
>> +int pagemap;
>> +
>
> Get rid of these globals and pass these in - please find name
> that clearly indicates them as fds
>
I'll update their names to indicate fds. This is a standalone test
application. Shouldn't the usage of global variables be fine?
>> +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.
>
Sure.
>> +}
>> +
>> +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?
I'll fix this.
>> +/*
>> + * 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
I'll update the comment.
>> +
>> +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
Yes, non-root user can open this file. I'll check the usage of skip
macros as well.
>> + test_simple();
>> + test_vma_reuse();
>> + test_hugepage();
>
> What happens when these tests fail?
They are independent. Each of them marks the test pass or fail on its
own. If one of them fails, others will keep on executing next.
>> +
>> + 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.
I'll update and include the output as well.
> thanks,
> -- Shuah
On 24.02.22 22:23, 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.
>
A note that madv_populate.c already contains some SOFTDIRTY tests
regarding MADV_POPULATE. Eventually we want to factor out
softdirty/pagemap handling+checks for easier reuse.
--
Thanks,
David / dhildenb
On 2/27/22 11:55 PM, Muhammad Usama Anjum wrote:
> Hi,
>
> Andrew had already accepted the patch. I'll send an iteration.
>
> On 2/26/22 5:35 AM, Shuah Khan wrote:
>>> +#define PAGEMAP_PATH "/proc/self/pagemap"
>>
>> Why is this names PATH - it is the file name right?
> I'll update the names of the macros.
>
>>> +
>>> +int clear_refs;
>>> +int pagemap;
>>> +
>>
>> Get rid of these globals and pass these in - please find name
>> that clearly indicates them as fds
>>
> I'll update their names to indicate fds. This is a standalone test
> application. Shouldn't the usage of global variables be fine?
>
It makes it harder to maintain. Unless there is reason, avoid globals.
I am not seeing a need for globals in this case, other than convenience.
thanks,
-- Shuah
On 2/28/22 2:37 AM, David Hildenbrand wrote:
> On 24.02.22 22:23, 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.
>>
>
> A note that madv_populate.c already contains some SOFTDIRTY tests
> regarding MADV_POPULATE. Eventually we want to factor out
> softdirty/pagemap handling+checks for easier reuse.
>
>
Is this patch unnecessary then?
thanks,
-- Shuah
On 3/3/22 11:39 AM, Gabriel Krisman Bertazi wrote:
> Shuah Khan <[email protected]> writes:
>
>> On 2/28/22 2:37 AM, David Hildenbrand wrote:
>>> On 24.02.22 22:23, 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.
>>>>
>>> A note that madv_populate.c already contains some SOFTDIRTY tests
>>> regarding MADV_POPULATE. Eventually we want to factor out
>>> softdirty/pagemap handling+checks for easier reuse.
>>>
>>
>> Is this patch unnecessary then?
>
> It is not unnecessary since the madv test doesn't cover the bug tested
> here, afaik. But, as mentioned when I originally submitted this patch,
> it should be merged into selftests/vm/madv_populate.c or, at least,
> reuse that existing infrastructure.
>
> https://lore.kernel.org/lkml/[email protected]/
>
Oops this one came in a few months ago and appears to have slipped
through and didn't get the right attention. Sorry about that.
Please resend the patch and cc all the everybody on this thread.
I would like to have your patch reviewed and looked at first. This
patch needs rework sine it has several comments to be addressed.
thanks,
-- Shuah
Shuah Khan <[email protected]> writes:
> On 2/28/22 2:37 AM, David Hildenbrand wrote:
>> On 24.02.22 22:23, 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.
>>>
>> A note that madv_populate.c already contains some SOFTDIRTY tests
>> regarding MADV_POPULATE. Eventually we want to factor out
>> softdirty/pagemap handling+checks for easier reuse.
>>
>
> Is this patch unnecessary then?
It is not unnecessary since the madv test doesn't cover the bug tested
here, afaik. But, as mentioned when I originally submitted this patch,
it should be merged into selftests/vm/madv_populate.c or, at least,
reuse that existing infrastructure.
https://lore.kernel.org/lkml/[email protected]/
--
Gabriel Krisman Bertazi
Shuah Khan <[email protected]> writes:
> On 3/3/22 11:39 AM, Gabriel Krisman Bertazi wrote:
>> Shuah Khan <[email protected]> writes:
>>
>>> On 2/28/22 2:37 AM, David Hildenbrand wrote:
>>>> On 24.02.22 22:23, 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.
>>>>>
>>>> A note that madv_populate.c already contains some SOFTDIRTY tests
>>>> regarding MADV_POPULATE. Eventually we want to factor out
>>>> softdirty/pagemap handling+checks for easier reuse.
>>>>
>>>
>>> Is this patch unnecessary then?
>> It is not unnecessary since the madv test doesn't cover the bug tested
>> here, afaik. But, as mentioned when I originally submitted this patch,
>> it should be merged into selftests/vm/madv_populate.c or, at least,
>> reuse that existing infrastructure.
>> https://lore.kernel.org/lkml/[email protected]/
>>
>
> Oops this one came in a few months ago and appears to have slipped
> through and didn't get the right attention. Sorry about that.
>
> Please resend the patch and cc all the everybody on this thread.
>
> I would like to have your patch reviewed and looked at first. This
> patch needs rework sine it has several comments to be addressed.
Hi Shuah,
The patch being discussed in that thread is the same that Usama is
proposing here, minus a few modifications. Usama has taken over the work
to upstream it.
We just spoke, and he will follow up with a new version that addresses
the coding issues and reuses the infrastructure from madv_populate.c
Sorry for the noise.
--
Gabriel Krisman Bertazi