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 <pule...@huawei.com>
> > >
> > > As generic read_sysfs is available in vm_utils, let's
> > > use is in thuge-gen test.
> > >
> > > Signed-off-by: Pu Lehui <pule...@huawei.com>
With the fix below:
Reviewed-by: Lorenzo Stoakes <lorenzo.stoa...@oracle.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
> > >
>
