Re: [PATCH v5 01/10] selftests/mm: make file-backed THP split work by setting force option

[Zi Yan]

On Wed Jan 22, 2025 at 1:32 AM EST, Baolin Wang wrote:
>
>
> On 2025/1/17 05:10, Zi Yan wrote:
>> Commit acd7ccb284b8 ("mm: shmem: add large folio support for tmpfs")
>> changes huge=always to allocate THP/mTHP based on write size and
>> split_huge_page_test does not write PMD size data, so file-back THP is not
>> created during the test.
>> 
>> Set /sys/kernel/mm/transparent_hugepage/shmem_enabled to "force" to force
>> THP allocation.
>> 
>> Signed-off-by: Zi Yan <ziy@xxxxxxxxxx>
>> ---
>>   .../selftests/mm/split_huge_page_test.c       | 47 +++++++++++++++++--
>>   1 file changed, 43 insertions(+), 4 deletions(-)
>> 
>> diff --git a/tools/testing/selftests/mm/split_huge_page_test.c b/tools/testing/selftests/mm/split_huge_page_test.c
>> index 3f353f3d070f..eccaa347140b 100644
>> --- a/tools/testing/selftests/mm/split_huge_page_test.c
>> +++ b/tools/testing/selftests/mm/split_huge_page_test.c
>> @@ -264,15 +264,46 @@ void split_pte_mapped_thp(void)
>>   void split_file_backed_thp(void)
>>   {
>>   	int status;
>> -	int fd;
>> -	ssize_t num_written;
>> +	int fd, shmem_sysctl_fd;
>> +	ssize_t num_written, num_read;
>>   	char tmpfs_template[] = "/tmp/thp_split_XXXXXX";
>>   	const char *tmpfs_loc = mkdtemp(tmpfs_template);
>> -	char testfile[INPUT_MAX];
>> +	char testfile[INPUT_MAX], sysctl_buf[INPUT_MAX] = {0};
>>   	uint64_t pgoff_start = 0, pgoff_end = 1024;
>> +	const char *shmem_sysctl = "/sys/kernel/mm/transparent_hugepage/shmem_enabled";
>> +	char *opt1, *opt2;
>>   
>>   	ksft_print_msg("Please enable pr_debug in split_huge_pages_in_file() for more info.\n");
>>   
>> +	shmem_sysctl_fd = open(shmem_sysctl, O_RDWR);
>> +	if (shmem_sysctl_fd == -1) {
>> +		ksft_perror("cannot open shmem sysctl");
>> +		goto out;
>> +	}
>> +
>> +	num_read = read(shmem_sysctl_fd, sysctl_buf, INPUT_MAX);
>> +	if (num_read < 1) {
>> +		ksft_perror("Failed to read shmem sysctl");
>
> You should close() shmem_sysctl_fd before returning.

Ack.

>
>> +		goto out;
>> +	}
>> +
>> +	opt1 = strchr(sysctl_buf, '[');
>> +	opt2 = strchr(sysctl_buf, ']');
>> +	if (!opt1 || !opt2) {
>
> Ditto.

Ack.
>
>> +		ksft_perror("cannot read shmem sysctl config");
>> +		goto out;
>> +	}
>> +
>> +	/* get existing shmem sysctl config into sysctl_buf */
>> +	strncpy(sysctl_buf, opt1 + 1, opt2 - opt1 - 1);
>> +	memset(sysctl_buf + (opt2 - opt1 - 1), 0, INPUT_MAX);
>> +
>> +	num_written = write(shmem_sysctl_fd, "force", sizeof("force"));
>> +	if (num_written < 1) {
>> +		ksft_perror("Fail to write force to shmem sysctl");
>> +		goto out;
>> +	}
>> +
>>   	status = mount("tmpfs", tmpfs_loc, "tmpfs", 0, "huge=always,size=4m");
>>   
>>   	if (status)
>> @@ -317,13 +348,21 @@ void split_file_backed_thp(void)
>>   	if (status)
>>   		ksft_exit_fail_msg("cannot remove tmp dir: %s\n", strerror(errno));
>>   
>> +	num_written = write(shmem_sysctl_fd, sysctl_buf, strlen(sysctl_buf) + 1);
>> +	if (num_written < 1)
>> +		ksft_perror("Fail to restore shmem sysctl");
>> +
>>   	ksft_print_msg("Please check dmesg for more information\n");
>> -	ksft_test_result_pass("File-backed THP split test done\n");
>> +	ksft_test_result_pass("File-backed THP split to order %d test done\n", order);
>
> Seems the patch set split has issues. No 'order' variable in this patch.
>
> Anyway, I've fixed these issues in my local tree, and it works well. If 
> you fix them in the next version, please feel free to add:
> Reviewed-by: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>
> Tested-by: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>

Sure. Thank you for the review and testing. I will fix the above issues
and resend another version. First three patches can be merged first.
>
>
>>   	return;
>>   
>>   cleanup:
>> +	num_written = write(shmem_sysctl_fd, sysctl_buf, strlen(sysctl_buf) + 1);
>> +	if (num_written < 1)
>> +		ksft_perror("Fail to restore shmem sysctl");
>>   	umount(tmpfs_loc);
>>   	rmdir(tmpfs_loc);
>> +out:
>>   	ksft_exit_fail_msg("Error occurred\n");
>>   }
>>   




-- 
Best Regards,
Yan, Zi