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] [thread-next>] [day] [month] [year] [list]

Message-ID: <1fad971c-dd57-4fa9-b951-10f749829c10@lucifer.local>
Date: Wed, 11 Jun 2025 10:48:56 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Pu Lehui <pulehui@...weicloud.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 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>

> >
> > 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
> > >
>