Re: [PATCH V4 2/2] selftests: vm: Add test for Soft-Dirty PTE bit

[Muhammad Usama Anjum <usama.anjum@collabora.com>]

On 3/16/22 1:53 AM, Gabriel Krisman Bertazi wrote:
> Muhammad Usama Anjum <usama.anjum@collabora.com> writes:
> 
>> From: Gabriel Krisman Bertazi <krisman@collabora.com>
> 
> 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 <will@kernel.org>
>> Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.com>
>> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
>> ---
>> 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