Re: [PATCH v8 3/3] ksm: add mremap selftests for ksm_rmap_walk

[xu.xin16]

> > +static void prepare_pages(struct global_data *data, int nr_pages)
> > +{
> > +	/* Allocate exactly pages for the test */
> > +	data->mapsize = nr_pages * 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");
> > +
> > +	/* Fill all pages with identical content to encourage KSM merging */
> > +	memset(data->region, 0x77, data->mapsize);
> > +}
> > +
> > +static int mremap_merge_and_migrate(struct global_data *data)
> > +{
> > +	int ret;
> > +	void *old_region;
> > +	void *new_region;
> > +	int nr_pages = 32;
> 
> Likely you should do
> 
> const unsigned int nr_pages = 16;
> 
> and then simply multiply it by 2 for the temporary larger mmap.

Ok, thanks.

> 
> > +	long merging_pages;
> > +
> > +	prepare_pages(data, nr_pages);
> 
> Is it really worth moving that into a helper function?
> 
> > +
> > +	if (ksm_start() < 0)
> > +		return FAIL_ON_CHECK;
> 
> Why the merge here? Wouldn't we want to merge after mremap()? mremap() will
> just unshare either way?

It maight be redundant, which was introduced from the last version v7 to calculate
the base KSM counters.

> 
> > +
> > +	old_region = data->region;
> > +	/*
> > +	 * Mremap the second half region to the first half location (FIXED).
> > +	 */
> 
> Okay, so we create a VMA and populated anonymous pages. Might be from THPs, do
> we care? I guess due to the split-on-dedup, this should be ok.
> 
> Now we mremap half of it over the other half.
> 
> 
> > +	new_region = mremap(old_region + data->mapsize / 2, data->mapsize / 2,
> > +			    data->mapsize / 2, MREMAP_MAYMOVE | MREMAP_FIXED,
> > +			    old_region);
> > +	if (new_region == MAP_FAILED) {
> > +		ksft_print_msg("mremap failed: %s\n", strerror(errno));
> > +		return FAIL_ON_CHECK;
> > +	}
> > +	data->region = new_region;
> > +	data->mapsize /= 2;	/* mapping is now half of original */
> 
> Let's avoid the use of trailing comments where possible.

OK, thanks.
> 
> > +
> > +	if (ksm_start() < 0)
> > +		return FAIL_ON_CHECK;
> > +
> > +	/* Attempt to migrate the merged KSM page */
> > +	ret = try_to_move_page(data->region);
> > +	if (ret != 0) {
> > +		ksft_print_msg("migration of KSM page after mremap failed\n");
> > +		return FAIL_ON_CHECK;
> > +	}
> > +
> > +	/* Ensure ksmd scan two turns at least to update ksm counters */
> > +	if (ksm_start() < 0)
> > +		return FAIL_ON_CHECK;
> 
> Why do we have to re-run KSM? ksm_start() already performed two runs. To remove
> any leftover rmap entries?

The second run-ksm is necessary since we want to check if the counter
ksm_merging_pages is correct after mremap.

> 
> But fundamentally, wouldn't it better to query before+after the
> try_to_move_page() the PFN of all pages, to (a) make sure that they are the same
> before (all merged, if not -> skip) and (b) make sure that all were migrated (if
> still old PFN -> not migrated -> BUG).

That makes sense.

> 
> > +
> > +	merging_pages = ksm_get_self_merging_pages();
> > +	printf("merging_pages:%ld\n", merging_pages);
> > +	if (merging_pages != nr_pages / 2) {
> 
> 
> [...]
> 
> > +TEST_F(migrate, ksm_and_mremap)
> > +{
> > +	struct global_data *data = &self->data;
> > +	int ret;
> > +
> > +	/* Skip if KSM is not available */
> > +	if (ksm_stop() < 0)
> > +		SKIP(return, "accessing \"/sys/kernel/mm/ksm/run\" failed");
> > +	if (ksm_get_full_scans() < 0)
> > +		SKIP(return, "accessing \"/sys/kernel/mm/ksm/full_scan\" failed");
> > +
> > +	ret = prctl(PR_SET_MEMORY_MERGE, 1, 0, 0, 0);
> > +	if (ret < 0 && errno == EINVAL)
> > +		SKIP(return, "PR_SET_MEMORY_MERGE not supported");
> > +	else if (ret)
> > +		ksft_exit_fail_perror("PR_SET_MEMORY_MERGE=1 failed");
> 
> Why are we not simply using MADV_MERGEABLE instead of the prctl?

No special reason here, I can switch to use MADV_MERGEABLE instead of the prctl if you think it's better.