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

[Baolin Wang <baolin.wang@linux.alibaba.com>]

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@nvidia.com>
> ---
>   .../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.

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

Ditto.

> +		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@linux.alibaba.com>
Tested-by: Baolin Wang <baolin.wang@linux.alibaba.com>


>   	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");
>   }
>