Re: ksm: add mremap selftests for ksm_rmap_walk

["David Hildenbrand (Arm)" <david@kernel.org>]

On 4/8/26 02:08, Hugh Dickins wrote:
> On Tue, 7 Apr 2026, David Hildenbrand (Arm) wrote:
>> On 4/7/26 08:08, xu.xin16@zte.com.cn wrote:
>>> From: xu xin <xu.xin16@zte.com.cn>
>>>
>>> The existing tools/testing/selftests/mm/rmap.c has already one testcase
>>> for ksm_rmap_walk in TEST_F(migrate, ksm), which takes use of migration
>>> of page from one NUMA node to another NUMA node. However, it just lacks
>>> the senario of mremapped VMAs.
>>>
>>> Before migrating, we add the calling of mremap() to address mapped with KSM
>>> pages, which is specailly to test a optimization which is introduced by this
>>> patch ("ksm: Optimize rmap_walk_ksm by passing a suitable address range")
>>> https://lore.kernel.org/all/20260212193045556CbzCX8p9gDu73tQ2nvHEI@zte.com.cn/
>>>
>>> Result:
>>> TAP version 13
>>> 1..5
>>> ok 1 migrate.anon
>>> ok 2 migrate.shm
>>> ok 3 migrate.file # SKIP Failed in worker
>>> ok 4 migrate.ksm
>>> ok 5 migrate.ksm_and_mremap
>>>
>>> Signed-off-by: xu xin <xu.xin16@zte.com.cn>
>>> ---
>>>  tools/testing/selftests/mm/rmap.c    | 69 ++++++++++++++++++++++++++++
>>>  tools/testing/selftests/mm/vm_util.c | 38 +++++++++++++++
>>>  tools/testing/selftests/mm/vm_util.h |  2 +
>>>  3 files changed, 109 insertions(+)
>>>
>>> diff --git a/tools/testing/selftests/mm/rmap.c b/tools/testing/selftests/mm/rmap.c
>>> index 53f2058b0ef2..65470def2bf1 100644
>>> --- a/tools/testing/selftests/mm/rmap.c
>>> +++ b/tools/testing/selftests/mm/rmap.c
>>> @@ -430,4 +430,73 @@ TEST_F(migrate, ksm)
>>>  	propagate_children(_metadata, data);
>>>  }
>>>
>>> +/* To test if ksm page can be migrated when it's mremapped */
>>> +int merge_mremap_and_migrate(struct global_data *data)
>>> +{
>>> +	int ret = 0;
>>> +	/* Allocate range and set the same data */
>>> +	data->mapsize = 3*getpagesize();
>>> +	data->region = mmap(NULL, data->mapsize, PROT_READ|PROT_WRITE,
>>> +			   MAP_PRIVATE|MAP_ANON, -1, 0);
>>> +	if (data->region == MAP_FAILED)
>>> +		ksft_exit_fail_perror("mmap failed");
>>> +
>>> +	memset(data->region, 0x77, data->mapsize);
> 
> (Not crucial at all, but to avoid confusion between our results, I'll
> point out that my testcase only memset 2*getpagesize() there, leaving
> the last page unpopulated - just one less complication.)
> 
>>
>> What happens if you mremap() after faulting, but before merging?
>>
>> rmap_item->address always holds the user space address of the entry in
>> the parent process. It must match the one in the child process, because
>> mremap() will unmerge/unshare in the child.
>>
>> And it must match the one in the parent, as mremap() would similarly
>> unmerge/unshare.
>>
>> Maybe doing the mremap() before merging (but after faulting) would
>> trigger what Hugh described.
>>
>> break_cow() and friends don't care about the rmap, as they simply jump
>> directly to the user space address in the process.
>>
>> In rmap_walk_ksm(), I think the concern Hugh raised is that we are using
>>
>> 	const pgoff_t pgoff = rmap_item->address >> PAGE_SHIFT;
>>
>> but we'd actually need a pgoff into the anon_vma. Without mremap, it
>> does not matter, they are the same (tests keep passing). But with mremap
>> it's not longer the same.
>>
>> I think one could store it in the ksm_rmap_item, but that would increase
>> it's size. It's essentially the folio->index of the original page we are
>> replacing.
>>
>> Or we could just remember "pgoff is not that simple because mremap was
>> involved, so walk the whole damn thing".
>>
>> We could also just try walking all involved processes, looking only at
>> that user space address (but that gets more tricky with rmap locking etc
>> ...).
>>
>> Anyhow, I think that's the concern Hugh raised, IIUC.
> 
> Yes, you and Lorenzo are seeing the same seed for doubt as I saw:
> as you say, "pgoff is not that simple because mremap involved";
> but it is confusing, so hard for us to be sure about it.
> 
>>
>>> +
>>> +	if (ksm_start() < 0)
>>> +		return FAIL_ON_CHECK;
>>> +
>>> +	/* 1  2 expected */
>>> +	ksft_print_msg("Shared: %ld (1 expected) Sharing: %ld (2 expected)\n",
>>> +		ksm_get_pages_shared(), ksm_get_pages_sharing());
>>> +
>>> +	/*
>>> +	 * Mremap the second pagesize address range into the third pagesize
>>> +	 * address.
>>> +	 */
>>> +	data->region = mremap(data->region + getpagesize(), getpagesize(), getpagesize(),
>>> +			 MREMAP_MAYMOVE|MREMAP_FIXED, data->region + 2*getpagesize());
>>
>> There would not be a KSM page after this mremap(), no?
> 
> Oh, good thinking: yes, a side-effect of mremap's non-persistent
> MADV_UNMERGEABLE would be that there's no KSM page in the mremapped
> "subregion" at this instant, so the try_to_move_page() which follows
> is likely to have no trouble succeeding; so, as it stands, this test
> is not testing what's required.
> 
> Sorry, I won't be able to give this more attention for a week: I
> wanted to advertise my doubt before 7.1 merge window, while awkwardly
> knowing I'd have to back out of ensuing discussion and testing for a
> few days.  It seems agreed that we won't endanger 7.1 until this is
> resolved: I just hope I'm not guilty of raising a false alarm.

Not at all. During my review I primarily focused on the "do we handle
fork'ed child processes in all cases correctly".

Given that our tests keep passing, I was assuming that non-fork should
be fine. But seems like we might be missing some test coverage even
there, with respect to mremap().

-- 
Cheers,

David