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" +#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(); +}
On 2/24/22 2:23 PM, Muhammad Usama Anjum wrote:
This introduces three tests:
- 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
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 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 24.02.22 22:23, Muhammad Usama Anjum wrote:
This introduces three tests:
- 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.
On 2/28/22 2:37 AM, David Hildenbrand wrote:
On 24.02.22 22:23, Muhammad Usama Anjum wrote:
This introduces three tests:
- 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
Shuah Khan skhan@linuxfoundation.org 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:
- 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/87lf553z5g.fsf@collabora.com/
On 3/3/22 11:39 AM, Gabriel Krisman Bertazi wrote:
Shuah Khan skhan@linuxfoundation.org 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:
- 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.
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 skhan@linuxfoundation.org writes:
On 3/3/22 11:39 AM, Gabriel Krisman Bertazi wrote:
Shuah Khan skhan@linuxfoundation.org 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:
- 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/87lf553z5g.fsf@collabora.com/
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.
linux-kselftest-mirror@lists.linaro.org
-
David Hildenbrand -
Gabriel Krisman Bertazi -
Muhammad Usama Anjum -
Shuah Khan