linux-kernel - Re: [PATCH] selftests/mm: Use generic read

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <0dc9c413-c979-4d18-b804-00ed2aaa3d3c@huaweicloud.com>
Date: Wed, 11 Jun 2025 17:55:07 +0800
From: Pu Lehui <pulehui@...weicloud.com>
To: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
Cc: akpm@...ux-foundation.org, shuah@...nel.org, linux-mm@...ck.org,
 linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
 pulehui@...wei.com
Subject: Re: [PATCH] selftests/mm: Use generic read_sysfs in thuge-gen test



On 2025/6/11 17:48, Lorenzo Stoakes wrote:
> On Wed, Jun 11, 2025 at 05:40:06PM +0800, Pu Lehui wrote:
>>
>>
>> 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@...wei.com>
>>>>
>>>> As generic read_sysfs is available in vm_utils, let's
>>>> use is in thuge-gen test.
>>>>
>>>> Signed-off-by: Pu Lehui <pulehui@...wei.com>
> 
> With the fix below:
> 
> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>

will send new soon, thanks!

> 
>>>
>>> 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?
> 
> Looks good to me!
> 
>>
>> 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
>>>>
>>