Re: [Patch v2] selftests/mm: check content to see whether mremap corrupt data

[David Hildenbrand <david@redhat.com>]

[...]

>> @@ -390,67 +390,88 @@ static void split_pmd_thp_to_order(int order)
>>    static void split_pte_mapped_thp(void)
>>   {
>> -	char *one_page, *pte_mapped, *pte_mapped2;
>> -	size_t len = 4 * pmd_pagesize;
>> -	uint64_t thp_size;
>> +	const size_t nr_thps = 4;
>> +	const size_t thp_area_size = nr_thps * pmd_pagesize;
>> +	const size_t page_area_size = nr_thps * pagesize;
>> +	char *thp_area, *page_area = NULL, *tmp;
>>   	size_t i;
>>   -	one_page = mmap((void *)(1UL << 30), len, PROT_READ | PROT_WRITE,
>> +	thp_area = mmap((void *)(1UL << 30), thp_area_size, PROT_READ | PROT_WRITE,
>>   			MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
>> -	if (one_page == MAP_FAILED)
>> -		ksft_exit_fail_msg("Fail to allocate memory: %s\n", strerror(errno));
>> +	if (thp_area == MAP_FAILED) {
>> +		ksft_test_result_fail("Fail to allocate memory: %s\n", strerror(errno));
>> +		goto out;
> 
> thp_area mmap failed and out label will try to munmap MAP_FAILED, which is
> (void *) -1. munmap will fail with -EINVAL.

Indeed, should just be a "return;"

> 
>> +	}
>>   -	madvise(one_page, len, MADV_HUGEPAGE);
>> +	madvise(thp_area, thp_area_size, MADV_HUGEPAGE);
>>   -	for (i = 0; i < len; i++)
>> -		one_page[i] = (char)i;
>> +	for (i = 0; i < thp_area_size; i++)
>> +		thp_area[i] = (char)i;
>>   -	if (!check_huge_anon(one_page, 4, pmd_pagesize))
>> -		ksft_exit_fail_msg("No THP is allocated\n");
>> +	if (!check_huge_anon(thp_area, nr_thps, pmd_pagesize)) {
>> +		ksft_test_result_skip("Not all THPs allocated\n");
>> +		goto out;
>> +	}
>>   -	/* remap the first pagesize of first THP */
>> -	pte_mapped = mremap(one_page, pagesize, pagesize, MREMAP_MAYMOVE);
>> -
>> -	/* remap the Nth pagesize of Nth THP */
>> -	for (i = 1; i < 4; i++) {
>> -		pte_mapped2 = mremap(one_page + pmd_pagesize * i + pagesize * i,
>> -				     pagesize, pagesize,
>> -				     MREMAP_MAYMOVE|MREMAP_FIXED,
>> -				     pte_mapped + pagesize * i);
>> -		if (pte_mapped2 == MAP_FAILED)
>> -			ksft_exit_fail_msg("mremap failed: %s\n", strerror(errno));
>> -	}
>> -
>> -	/* smap does not show THPs after mremap, use kpageflags instead */
>> -	thp_size = 0;
>> -	for (i = 0; i < pagesize * 4; i++)
>> -		if (i % pagesize == 0 &&
>> -		    is_backed_by_folio(&pte_mapped[i], pmd_order, pagemap_fd, kpageflags_fd))
>> -			thp_size++;
>> -
>> -	if (thp_size != 4)
>> -		ksft_exit_fail_msg("Some THPs are missing during mremap\n");
>> -
>> -	/* split all remapped THPs */
>> -	write_debugfs(PID_FMT, getpid(), (uint64_t)pte_mapped,
>> -		      (uint64_t)pte_mapped + pagesize * 4, 0);
>> -
>> -	/* smap does not show THPs after mremap, use kpageflags instead */
>> -	thp_size = 0;
>> -	for (i = 0; i < pagesize * 4; i++) {
>> -		if (pte_mapped[i] != (char)i)
>> -			ksft_exit_fail_msg("%ld byte corrupted\n", i);
>> +	/*
>> +	 * To challenge spitting code, we will mremap page[x] of the
>> +	 * thp[x] into a smaller area, and trigger the split from that
>> +	 * smaller area. This will end up replacing the PMD mappings in
>> +	 * the thp_area by PTE mappings first, leaving the THPs unsplit.
>> +	 */
>> +	page_area = mmap(NULL, page_area_size, PROT_READ | PROT_WRITE,
>> +			MAP_ANONYMOUS | MAP_PRIVATE, -1, 0);
>> +	if (page_area == MAP_FAILED) {
>> +		ksft_test_result_fail("Fail to allocate memory: %s\n", strerror(errno));
>> +		goto out;
>> +	}
>>   -		if (i % pagesize == 0 &&
>> -		    !is_backed_by_folio(&pte_mapped[i], 0, pagemap_fd, kpageflags_fd))
>> -			thp_size++;
>> +	for (i = 0; i < nr_thps; i++) {
>> +		tmp = mremap(thp_area + pmd_pagesize * i + pagesize * i,
>> +			     pagesize, pagesize, MREMAP_MAYMOVE|MREMAP_FIXED,
>> +			     page_area + pagesize * i);
>> +		if (tmp != MAP_FAILED)
>> +			continue;
>> +		ksft_test_result_fail("mremap failed: %s\n", strerror(errno));
>> +		goto out;
>> +	}
>> +
>> +	/*
>> +	 * Verify that our THPs were not split yet. Note that
>> +	 * check_huge_anon() cannot be used as it checks for PMD mappings.
>> +	 */
>> +	for (i = 0; i < nr_thps; i++) {
>> +		if (is_backed_by_folio(page_area + i * pagesize, pmd_order,
>> +				       pagemap_fd, kpageflags_fd))
>> +			continue;
>> +		ksft_test_result_fail("THP %zu missing after mremap\n", i);
>> +		goto out;
>>   	}
>>   -	if (thp_size)
>> -		ksft_exit_fail_msg("Still %ld THPs not split\n", thp_size);
>> +	/* Split all THPs through the remapped pages. */
>> +	write_debugfs(PID_FMT, getpid(), (uint64_t)page_area,
>> +		      (uint64_t)page_area + page_area_size, 0);
>> +
>> +	/* Corruption during mremap or split? */
>> +	for (i = 0; i < page_area_size; i++) {
>> +		if (page_area[i] == (char)i)
>> +			continue;
>> +		ksft_test_result_fail("%zu byte corrupted\n", i);
>> +		goto out;
>> +	}
>> +
>> +	/* Split failed? */
>> +	for (i = 0; i < nr_thps; i++) {
>> +		if (is_backed_by_folio(&page_area[i], 0, pagemap_fd, kpageflags_fd))
> 			
> page_area + i * pagesize, like Wei pointed out in another email.
> 
>> +			continue;
>> +		ksft_test_result_fail("THP %zu not split\n", i);
>> +	}
>>    	ksft_test_result_pass("Split PTE-mapped huge pages successful\n");
>> -	munmap(one_page, len);
>> +out:
>> +	munmap(thp_area, thp_area_size);
>> +	if (page_area)
>> +		munmap(page_area, page_area_size);
>>   }
>>    static void split_file_backed_thp(int order)
>> -- 
>> 2.50.1
> 
> Otherwise, LGTM. With all the changes in this email and other email,
> feel free to add Reviewed-by: Zi Yan <ziy@nvidia.com> when you send it
> out formally.

Thanks!

I'm currently chasing why I keep getting temporary

Bail out! Some THPs are missing during mremap

Already on the old test. Something doesn't work quite right as it seems.

-- 
Cheers

David / dhildenb