Re: [PATCH v5 6/7] selftests: mm: Add a test for remapping within a range

From: Joel Fernandes <joel@joelfernandes.org>
To: Lorenzo Stoakes <lstoakes@gmail.com>
Cc: linux-kernel@vger.kernel.org, linux-kselftest@vger.kernel.org,
	linux-mm@kvack.org, Shuah Khan <shuah@kernel.org>,
	Vlastimil Babka <vbabka@suse.cz>, Michal Hocko <mhocko@suse.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Kirill A Shutemov <kirill@shutemov.name>,
	"Liam R. Howlett" <liam.howlett@oracle.com>,
	"Paul E. McKenney" <paulmck@kernel.org>,
	Suren Baghdasaryan <surenb@google.com>,
	Kalesh Singh <kaleshsingh@google.com>,
	Lokesh Gidra <lokeshgidra@google.com>
Subject: Re: [PATCH v5 6/7] selftests: mm: Add a test for remapping within a range
Date: Mon, 28 Aug 2023 18:59:12 +0000	[thread overview]
Message-ID: <20230828185912.GD1621761@google.com> (raw)
In-Reply-To: <fb429d82-4576-42d7-b51f-7a72f7f8caec@lucifer.local>

On Sun, Aug 27, 2023 at 11:15:20AM +0100, Lorenzo Stoakes wrote:
> On Sun, Aug 27, 2023 at 10:57:59AM +0100, Lorenzo Stoakes wrote:
> [snip]
> 
> > > +/*
> > > + * Verify that an mremap within a range does not cause corruption
> > > + * of unrelated part of range.
> > > + *
> > > + * Consider the following range which is 2MB aligned and is
> > > + * a part of a larger 10MB range which is not shown. Each
> > > + * character is 256KB below making the source and destination
> 
> Just noticed, I think you misspeak here, as this test doens't seem to
> offset by 256 KiB? That is the strategy for mremap_move_1mb_from_start()
> rather than this test so perhaps comment needs to be moved around?
> 
>  * 2MB each. The lower case letters are moved (s to d) and the
>  * upper case letters are not moved. The below test verifies
>  * that the upper case S letters are not corrupted by the
>  * adjacent mremap.
>  *
>  * |DDDDddddSSSSssss|
>  */
>  static void mremap_move_within_range(char pattern_seed)

Here we are moving 1MB within a 4MB zone of a large mapping. Each character
's' or 'd' is 256KB. The 256KB there is just for illustration and not really
significant as such. The 'ssss' is moved to 'dddd' 1MB each. Here we make
sure that this move did not accidentally corrupt 'SSSS' and 'DDDD' due to
alignment optimization. Basically to protect from this, we check in the code
that the source address is beginning of the VMA:
+	if (vma->vm_start != addr_to_align)
+		return false;

But you did point an issue which is I need to change the comment from 'larger
10MB' to 'larger 20MB'.

In the mremap_move_1mb_from_start() test, I request for an alignment of
1.25MB so that when I align down, I fall no mapping. This is to catch a bug
that Linus found which is that just because an aligned down address did not
fall on a mapping, that doesn't mean we can just move it at PMD-level
otherwise we destroy the mapping.

I do need to update the test name in mremap_move_1mb_from_start() to: "mremap
move 1mb from start at 1MB+256KB aligned src". So thanks for point this!

Would that sort it out or is there still something in the comment I am
missing?

Thanks!

 - Joel

> 
> [snip]