Re: [PATCH] selftests/mm: Use generic read_sysfs in thuge-gen test

[Pu Lehui <pulehui@huaweicloud.com>]

On 2025/6/11 17:18, Lorenzo Stoakes wrote:
> On Wed, Jun 11, 2025 at 08:40:11AM +0000, Pu Lehui wrote:
>> From: Pu Lehui <pulehui@huawei.com>
>>
>> As generic read_sysfs is available in vm_utils, let's
>> use is in thuge-gen test.
>>
>> Signed-off-by: Pu Lehui <pulehui@huawei.com>
> 
> It generally looks good, just one point about a warning below to address.
> 
>> ---
>>   tools/testing/selftests/mm/thuge-gen.c | 37 +++++++-------------------
>>   1 file changed, 9 insertions(+), 28 deletions(-)
>>
>> diff --git a/tools/testing/selftests/mm/thuge-gen.c b/tools/testing/selftests/mm/thuge-gen.c
>> index 95b6f043a3cb..e11dfbfa661b 100644
>> --- a/tools/testing/selftests/mm/thuge-gen.c
>> +++ b/tools/testing/selftests/mm/thuge-gen.c
>> @@ -77,40 +77,19 @@ void show(unsigned long ps)
>>   	system(buf);
>>   }
>>
>> -unsigned long thuge_read_sysfs(int warn, char *fmt, ...)
>> +unsigned long read_free(unsigned long ps)
>>   {
>> -	char *line = NULL;
>> -	size_t linelen = 0;
>> -	char buf[100];
>> -	FILE *f;
>> -	va_list ap;
>>   	unsigned long val = 0;
>> +	char buf[100];
>>
>> -	va_start(ap, fmt);
>> -	vsnprintf(buf, sizeof buf, fmt, ap);
>> -	va_end(ap);
>> +	snprintf(buf, sizeof(buf),
>> +		 "/sys/kernel/mm/hugepages/hugepages-%lukB/free_hugepages",
>> +		 ps >> 10);
>> +	read_sysfs(buf, &val);
> 
> We're losing all of the 'warn' logic here so if we can't find
> /sys/kernel/mm/hugepages/hugepages-%lukB/free_hugepages when ps != getpagesize()
> we no longer print a message about it.


Hi Lorenzo,

Thanks for review. Right, we should explicit that warning when ps != 
getpagesize(). How about the following modify?

diff --git a/tools/testing/selftests/mm/thuge-gen.c 
b/tools/testing/selftests/mm/thuge-gen.c
index e11dfbfa661b..8e2b08dc5762 100644
--- a/tools/testing/selftests/mm/thuge-gen.c
+++ b/tools/testing/selftests/mm/thuge-gen.c
@@ -85,7 +85,8 @@ unsigned long read_free(unsigned long ps)
         snprintf(buf, sizeof(buf),
                  "/sys/kernel/mm/hugepages/hugepages-%lukB/free_hugepages",
                  ps >> 10);
-       read_sysfs(buf, &val);
+       if (read_sysfs(buf, &val) && ps != getpagesize())
+               ksft_print_msg("missing %s\n", buf);

         return val;
  }

> 
> Should we reinstate that?
> 
> Other than this, we're ignoring errors, which by default means we return 0, but
> this is what we were doing anyway. It's only this case I think that matters.
> 
>>
>> -	f = fopen(buf, "r");
>> -	if (!f) {
>> -		if (warn)
>> -			ksft_print_msg("missing %s\n", buf);
>> -		return 0;
>> -	}
>> -	if (getline(&line, &linelen, f) > 0) {
>> -		sscanf(line, "%lu", &val);
>> -	}
>> -	fclose(f);
>> -	free(line);
>>   	return val;
>>   }
>>
>> -unsigned long read_free(unsigned long ps)
>> -{
>> -	return thuge_read_sysfs(ps != getpagesize(),
>> -			  "/sys/kernel/mm/hugepages/hugepages-%lukB/free_hugepages",
>> -			  ps >> 10);
>> -}
>> -
>>   void test_mmap(unsigned long size, unsigned flags)
>>   {
>>   	char *map;
>> @@ -173,6 +152,7 @@ void test_shmget(unsigned long size, unsigned flags)
>>   void find_pagesizes(void)
>>   {
>>   	unsigned long largest = getpagesize();
>> +	unsigned long shmmax_val = 0;
>>   	int i;
>>   	glob_t g;
>>
>> @@ -195,7 +175,8 @@ void find_pagesizes(void)
>>   	}
>>   	globfree(&g);
>>
>> -	if (thuge_read_sysfs(0, "/proc/sys/kernel/shmmax") < NUM_PAGES * largest)
>> +	read_sysfs("/proc/sys/kernel/shmmax", &shmmax_val);
>> +	if (shmmax_val < NUM_PAGES * largest)
>>   		ksft_exit_fail_msg("Please do echo %lu > /proc/sys/kernel/shmmax",
>>   				   largest * NUM_PAGES);
>>
>> --
>> 2.34.1
>>