Re: ksm: add mremap selftests for ksm_rmap_walk

[xu.xin16]

> > > > 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


Oh, thanks to David and Hugh's reminder, I now understand the issue Hugh was pointing out, and I also
see why my current test program incorrectly passes: before migrating the memory page, the
mremap() operation has already turned a KSM page into an anonymous page, causing the code to
take rmap_walk_anon instead of rmap_walk_ksm. Therefore, to reproduce the issue Hugh described,
we only need to add a delay of wait_ksmd_scan_finish_two_truns before migration (after mremap) 

	— simply wait for ksmd to merge the remapped page back into a KSM page.

such testing code can reproduce what Hugh points out and FAIL to migrate:

	/*
	* 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());

	printf("After meremap region: %p\n", data->region);   

	if (data->region == MAP_FAILED)
		return FAIL_ON_CHECK;

+	/* 
+	 * Wait ksmd to scan and remerge the mremaped page (since it's not KSM
+	 * page temporarily, see prep_move_vma->madvise(UNMERGEABLE)).
+	 */
+	 if (ksm_start() < 0)
+		return FAIL_ON_CHECK;
+
+	ksft_print_msg("Shared: %ld (1 expected) Sharing: %ld (1 expected)\n",
+		ksm_get_pages_shared(), ksm_get_pages_sharing());

	/* Check if we can migrate this region successfully */
	ret = try_to_move_page(data->region);
	if (ret != 0) {
		ksft_print_msg("failed to move_page\n");
		return ret;
	}

	/* Wait ksm scan two turns at least */
	if (ksm_start() < 0)
		return FAIL_ON_CHECK;

	/* 1  1 expected */
	ksft_print_msg("Shared: %ld (1 expected) Sharing: %ld (1 expected)\n",
		ksm_get_pages_shared(), ksm_get_pages_sharing());

	return 0;
}