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

[xu.xin16]

> I think there are various improvements and simplifications we can perform. In particular,
>  I don't think we need the errors messages or use data-> members.
> 
> What about the following simplification, to move this over the finishing line? (untested)

Agreed. Your version is cleaner and tests successfully.

> 
> There is the low chance of page compaction migrating the page while we check for it. Not sure 
> if we should handle it (but it would involve retrying on PFN mismatch).

It is really possible. To improve robustness, how about double-checking in range_maps_pfn() and
rename range_maps_pfn as range_maps_the_same_pfn() which caculate the fisrt PFN itself without
the input PFN as follows:

static bool range_maps_the_same_pfn(int pagemap_fd, void *region, int nr_pages)
{
        int i;
        int second_times = 0;
        unsigned long first_pfn;

again:
        first_pfn = pagemap_get_pfn(pagemap_fd, region);
        for (i = 0; i < nr_pages; i++) {
                if (pagemap_get_pfn(pagemap_fd, region + i * getpagesize()) != first_pfn) {
                        if (second_times)
                                return false;
                        else {
                                /*
                                 * In case of low chance the low chance of page compaction
                                 * migrating the page while we check for pfn.
                                 */
                                second_times++;
                                goto again;
                        }
                }
        }

        return true;
}


> 
> 
> diff --git a/tools/testing/selftests/mm/rmap.c b/tools/testing/selftests/mm/rmap.c
> index 53f2058b0ef2b..1b7ab46a520cf 100644
> --- a/tools/testing/selftests/mm/rmap.c
> +++ b/tools/testing/selftests/mm/rmap.c
> @@ -430,4 +430,68 @@ TEST_F(migrate, ksm)
>  	propagate_children(_metadata, data);
>  }
>  
> +static bool range_maps_pfn(int pagemap_fd, void *region, int nr_pages,
> +		unsigned long pfn)
> +{
> +	int i;
> +
> +	for (i = 0; i < nr_pages; i++)
> +		if (pagemap_get_pfn(pagemap_fd, region + i * getpagesize()) != pfn)
> +			return false;
> +	return true;
> +}
> +
> +TEST_F(migrate, ksm_and_mremap)
> +{
> +	unsigned long old_pfn, new_pfn;
> +	void *region, *mremap_region;
> +	const int nr_pages = 16;
> +	size_t mmap_size;
> +	int pagemap_fd;
> +
> +	/* 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");
> +
> +	pagemap_fd = open("/proc/self/pagemap", O_RDONLY);
> +	if (pagemap_fd < 0)
> +		SKIP(return, "opening pagemap failed");
> +
> +	/* Allocate and populate twice the anon pages initially. */
> +	mmap_size = 2 * nr_pages * getpagesize();
> +	region = mmap(NULL, mmap_size, PROT_READ | PROT_WRITE,
> +		      MAP_PRIVATE | MAP_ANON, -1, 0);
> +	ASSERT_NE(region, MAP_FAILED);
> +	memset(region, 0x77, mmap_size);
> +
> +	/* mremap the second half over the first half, to stress rmap handling */
> +	mmap_size /= 2;
> +	mremap_region = mremap(region + mmap_size, mmap_size, mmap_size,
> +			       MREMAP_MAYMOVE | MREMAP_FIXED, region);
> +	ASSERT_EQ(mremap_region, region);
> +
> +	/* Merge all pages into a single KSM page. */
> +	madvise(region, mmap_size, MADV_MERGEABLE);
> +	ASSERT_EQ(ksm_start(), 0);
> +
> +	/* The whole range should map the same KSM page. */
> +	old_pfn = pagemap_get_pfn(pagemap_fd, region);
> +	if (old_pfn == -1ul)
> +		SKIP(return, "Obtaining PFN failed");
> +	ASSERT_TRUE(range_maps_pfn(pagemap_fd, region, nr_pages, old_pfn));
> +
> +	/*
> +	 * Migrate the KSM page; the whole range should map the new (migrated)
> +	 * KSM page.
> +	 */
> +	ASSERT_EQ(try_to_move_page(region), 0);
> +	new_pfn = pagemap_get_pfn(pagemap_fd, region);
> +	if (new_pfn == -1ul)
> +		SKIP(return, "Obtaining PFN failed");
> +	ASSERT_NE(new_pfn, old_pfn);
> +	ASSERT_TRUE(range_maps_pfn(pagemap_fd, region, nr_pages, new_pfn));
> +}
> +
>  TEST_HARNESS_MAIN
> -- 
> 2.43.0
> 
> -- 
> Cheers,
> 
> David