Muhammad Usama Anjum <[email protected]> writes:
> From: Gabriel Krisman Bertazi <[email protected]>
Hi Usama,
Please, cc me on the whole thread. I didn't get the patch 1/2 or the
cover letter.
> This introduces three tests:
> 1) Sanity check soft dirty basic semantics: allocate area, clean, dirty,
> check if the SD bit is 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]>
> ---
> V3 of this patch is in Andrew's tree. Please drop that.
v3 is still in linux-next and this note is quite hidden in the middle of
the commit message.
>
> Changes in V4:
> Cosmetic changes
> Removed global variables
> Replaced ksft_print_msg with ksft_exit_fail_msg to exit the program at
> once
> Some other minor changes
> Correct the authorship of the patch
>
> Tests of soft dirty bit in this patch and in madv_populate.c are
> non-overlapping. madv_populate.c has only one soft-dirty bit test in the
> context of different advise (MADV_POPULATE_READ and
> MADV_POPULATE_WRITE). This new test adds more tests.
>
> Tab width of 8 has been used to align the macros. This alignment may look
> odd in shell or email. But it looks alright in editors.
I'm curious if you tested reverting 912efa17e512. Did the new versions
of this patch still catch the original issue?
> Test output:
> TAP version 13
> 1..5
> ok 1 Test test_simple
> ok 2 Test test_vma_reuse reused memory location
> ok 3 Test test_vma_reuse dirty bit of previous page
> ok 4 Test test_hugepage huge page allocation
> ok 5 Test test_hugepage huge page dirty bit
> # Totals: pass:5 fail:0 xfail:0 xpass:0 skip:0 error:0
>
> Or
>
> TAP version 13
> 1..5
> ok 1 Test test_simple
> ok 2 Test test_vma_reuse reused memory location
> ok 3 Test test_vma_reuse dirty bit of previous page
> ok 4 # SKIP Test test_hugepage huge page allocation
> ok 5 # SKIP Test test_hugepage huge page dirty bit
> # Totals: pass:3 fail:0 xfail:0 xpass:0 skip:2 error:0
>
> Changes in V3:
> Move test to selftests/vm
> Use kselftest macros
> Minor updates to make code more maintainable
> Add configurations in config file
>
> V2 of this patch:
> https://lore.kernel.org/lkml/[email protected]/
> ---
> tools/testing/selftests/vm/.gitignore | 1 +
> tools/testing/selftests/vm/Makefile | 2 +
> tools/testing/selftests/vm/config | 2 +
> tools/testing/selftests/vm/soft-dirty.c | 146 ++++++++++++++++++++++++
> 4 files changed, 151 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 d7507f3c7c76a..3cb4fa771ec2a 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 4e68edb26d6b6..f25eb30b5f0cb 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_PROGS += soft-dirty
> TEST_GEN_PROGS += split_huge_page_test
> TEST_GEN_FILES += ksm_tests
>
> @@ -92,6 +93,7 @@ KSFT_KHDR_INSTALL := 1
> include ../lib.mk
>
> $(OUTPUT)/madv_populate: vm_util.c
> +$(OUTPUT)/soft-dirty: vm_util.c
> $(OUTPUT)/split_huge_page_test: vm_util.c
>
> ifeq ($(MACHINE),x86_64)
> diff --git a/tools/testing/selftests/vm/config b/tools/testing/selftests/vm/config
> index 60e82da0de850..be087c4bc3961 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 0000000000000..2d50ed3472206
> --- /dev/null
> +++ b/tools/testing/selftests/vm/soft-dirty.c
> @@ -0,0 +1,146 @@
> +// 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"
> +#include "vm_util.h"
> +
> +#define PAGEMAP "/proc/self/pagemap"
> +#define CLEAR_REFS "/proc/self/clear_refs"
> +#define MAX_LINE_LENGTH 512
MAX_LINE_LENGTH is no longer used after check_for_pattern was dropped.
Can't the previous defines and file handling functions also go the
vm_util.h?
> +#define TEST_ITERATIONS 10000
> +
> +static void test_simple(int pagemap_fd, int pagesize)
> +{
> + int i;
> + char *map;
> +
> + map = aligned_alloc(pagesize, pagesize);
> + if (!map)
> + ksft_exit_fail_msg("mmap failed\n");
> +
> + clear_softdirty();
> +
> + for (i = 0 ; i < TEST_ITERATIONS; i++) {
> + if (pagemap_is_softdirty(pagemap_fd, map) == 1) {
> + ksft_print_msg("dirty bit was 1, but should be 0 (i=%d)\n", i);
> + break;
> + }
> +
> + clear_softdirty();
> + map[0]++;
This will overflow several times during TEST_ITERATIONS. While it is
not broken, since we care about causing the page fault, it is not
obvious. Can you add a comment or do something like this instead?
map[0] = !map[0];
--
Gabriel Krisman Bertazi
On 3/16/22 1:53 AM, Gabriel Krisman Bertazi wrote:
> Muhammad Usama Anjum <[email protected]> writes:
>
>> From: Gabriel Krisman Bertazi <[email protected]>
>
> Hi Usama,
>
> Please, cc me on the whole thread. I didn't get the patch 1/2 or the
> cover letter.
>
Sorry, I'll correct it.
>> This introduces three tests:
>> 1) Sanity check soft dirty basic semantics: allocate area, clean, dirty,
>> check if the SD bit is 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]>
>> ---
>> V3 of this patch is in Andrew's tree. Please drop that.
>
> v3 is still in linux-next and this note is quite hidden in the middle of
> the commit message.
I've tried to put this message at the top of the changelog. I can add
"Note" in the start of it. What can be some other way to highlight this
kind of important message?
>>
>> Changes in V4:
>> Cosmetic changes
>> Removed global variables
>> Replaced ksft_print_msg with ksft_exit_fail_msg to exit the program at
>> once
>> Some other minor changes
>> Correct the authorship of the patch
>>
>> Tests of soft dirty bit in this patch and in madv_populate.c are
>> non-overlapping. madv_populate.c has only one soft-dirty bit test in the
>> context of different advise (MADV_POPULATE_READ and
>> MADV_POPULATE_WRITE). This new test adds more tests.
>>
>> Tab width of 8 has been used to align the macros. This alignment may look
>> odd in shell or email. But it looks alright in editors.
>
> I'm curious if you tested reverting 912efa17e512. Did the new versions
> of this patch still catch the original issue?
Yeah, it did after I reverted the patch and fixed build errors because
of some function's signature change and one test failed and hence issue
is caught:
TAP version 13
1..5
# dirty bit was 0, but should be 1 (i=1)
not ok 1 Test test_simple
ok 2 Test test_vma_reuse reused memory location
ok 3 Test test_vma_reuse dirty bit of previous page
ok 4 # SKIP Test test_hugepage huge page allocation
ok 5 # SKIP Test test_hugepage huge page dirty bit
# Totals: pass:2 fail:1 xfail:0 xpass:0 skip:2 error:0
>> Test output:
>> TAP version 13
>> 1..5
>> ok 1 Test test_simple
>> ok 2 Test test_vma_reuse reused memory location
>> ok 3 Test test_vma_reuse dirty bit of previous page
>> ok 4 Test test_hugepage huge page allocation
>> ok 5 Test test_hugepage huge page dirty bit
>> # Totals: pass:5 fail:0 xfail:0 xpass:0 skip:0 error:0
>>
>> Or
>>
>> TAP version 13
>> 1..5
>> ok 1 Test test_simple
>> ok 2 Test test_vma_reuse reused memory location
>> ok 3 Test test_vma_reuse dirty bit of previous page
>> ok 4 # SKIP Test test_hugepage huge page allocation
>> ok 5 # SKIP Test test_hugepage huge page dirty bit
>> # Totals: pass:3 fail:0 xfail:0 xpass:0 skip:2 error:0
[..]
>> +
>> +#define PAGEMAP "/proc/self/pagemap"
>> +#define CLEAR_REFS "/proc/self/clear_refs"
>> +#define MAX_LINE_LENGTH 512
>
> MAX_LINE_LENGTH is no longer used after check_for_pattern was dropped.
>
> Can't the previous defines and file handling functions also go the
> vm_util.h?
>
I don't want to make changes in other two tests. I just want to move
some functions which we need for this test into vm_util.h while keeping
changes less.
>> +#define TEST_ITERATIONS 10000
>> +
>> +static void test_simple(int pagemap_fd, int pagesize)
>> +{
>> + int i;
>> + char *map;
>> +
>> + map = aligned_alloc(pagesize, pagesize);
>> + if (!map)
>> + ksft_exit_fail_msg("mmap failed\n");
>> +
>> + clear_softdirty();
>> +
>> + for (i = 0 ; i < TEST_ITERATIONS; i++) {
>> + if (pagemap_is_softdirty(pagemap_fd, map) == 1) {
>> + ksft_print_msg("dirty bit was 1, but should be 0 (i=%d)\n", i);
>> + break;
>> + }
>> +
>> + clear_softdirty();
>> + map[0]++;
>
>
> This will overflow several times during TEST_ITERATIONS. While it is
> not broken, since we care about causing the page fault, it is not
> obvious. Can you add a comment or do something like this instead?
>
> map[0] = !map[0];
Yeah, it is less obvious. I'll add a comment
--
Muhammad Usama Anjum