Re: ksm: add mremap selftests for ksm_rmap_walk

[Hugh Dickins <hughd@google.com>]

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.

Hugh