Re: [PATCH v1 3/4] selftests/mm: Extract read_sysfs and write_sysfs into vm_util

[Pu Lehui <pulehui@huaweicloud.com>]

On 2025/5/30 19:48, Lorenzo Stoakes wrote:
> On Thu, May 29, 2025 at 03:56:49PM +0000, Pu Lehui wrote:
>> From: Pu Lehui <pulehui@huawei.com>
>>
>> Extract read_sysfs and write_sysfs into vm_util. Meanwhile, rename
>> the function in thuge-gen that has the same name as read_sysfs.
> 
> Nice!
> 
>>
>> Signed-off-by: Pu Lehui <pulehui@huawei.com>
> 
> Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>
> 
>> ---
>>   tools/testing/selftests/mm/ksm_tests.c | 32 ++--------------------
>>   tools/testing/selftests/mm/thuge-gen.c |  6 ++--
>>   tools/testing/selftests/mm/vm_util.c   | 38 ++++++++++++++++++++++++++
>>   tools/testing/selftests/mm/vm_util.h   |  2 ++
>>   4 files changed, 45 insertions(+), 33 deletions(-)
>>
>> diff --git a/tools/testing/selftests/mm/ksm_tests.c b/tools/testing/selftests/mm/ksm_tests.c
>> index dcdd5bb20f3d..e80deac1436b 100644
>> --- a/tools/testing/selftests/mm/ksm_tests.c
>> +++ b/tools/testing/selftests/mm/ksm_tests.c
>> @@ -58,40 +58,12 @@ int debug;
>>
>>   static int ksm_write_sysfs(const char *file_path, unsigned long val)
>>   {
>> -	FILE *f = fopen(file_path, "w");
>> -
>> -	if (!f) {
>> -		fprintf(stderr, "f %s\n", file_path);
>> -		perror("fopen");
>> -		return 1;
>> -	}
>> -	if (fprintf(f, "%lu", val) < 0) {
>> -		perror("fprintf");
>> -		fclose(f);
>> -		return 1;
>> -	}
>> -	fclose(f);
>> -
>> -	return 0;
>> +	return write_sysfs(file_path, val);
>>   }
>>
>>   static int ksm_read_sysfs(const char *file_path, unsigned long *val)
>>   {
>> -	FILE *f = fopen(file_path, "r");
>> -
>> -	if (!f) {
>> -		fprintf(stderr, "f %s\n", file_path);
>> -		perror("fopen");
>> -		return 1;
>> -	}
>> -	if (fscanf(f, "%lu", val) != 1) {
>> -		perror("fscanf");
>> -		fclose(f);
>> -		return 1;
>> -	}
>> -	fclose(f);
>> -
>> -	return 0;
>> +	return read_sysfs(file_path, val);
>>   }
>>
>>   static void ksm_print_sysfs(void)
>> diff --git a/tools/testing/selftests/mm/thuge-gen.c b/tools/testing/selftests/mm/thuge-gen.c
>> index a41bc1234b37..95b6f043a3cb 100644
>> --- a/tools/testing/selftests/mm/thuge-gen.c
>> +++ b/tools/testing/selftests/mm/thuge-gen.c
>> @@ -77,7 +77,7 @@ void show(unsigned long ps)
>>   	system(buf);
>>   }
>>
>> -unsigned long read_sysfs(int warn, char *fmt, ...)
>> +unsigned long thuge_read_sysfs(int warn, char *fmt, ...)
>>   {
> 
> I wonder if we could update these to use the newly shared functions?
> 
> Not a big deal though, perhaps a bit out of scope here, more of a nice-to-have.

Hi Lorenzo, Andrew,

Yep, we can do it more. But, actually, I am not sure about the merge 
process of mm module. Do I need to resend the whole series or just the 
diff below?


 From 13ecf9d66a0068d520be955e3ca8d9c0dc9d8393 Mon Sep 17 00:00:00 2001
From: Pu Lehui <pulehui@huawei.com>
Date: Tue, 3 Jun 2025 06:50:43 +0000
Subject: [PATCH] selftests/mm: Use generic read_sysfs in thuge-gen test

Use generic read_sysfs in thuge-gen test.

Signed-off-by: Pu Lehui <pulehui@huawei.com>
---
  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..5a32ed0ba26c 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);

-	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

> 
>>   	char *line = NULL;
>>   	size_t linelen = 0;
>> @@ -106,7 +106,7 @@ unsigned long read_sysfs(int warn, char *fmt, ...)
>>
>>   unsigned long read_free(unsigned long ps)
>>   {
>> -	return read_sysfs(ps != getpagesize(),
>> +	return thuge_read_sysfs(ps != getpagesize(),
>>   			  "/sys/kernel/mm/hugepages/hugepages-%lukB/free_hugepages",
>>   			  ps >> 10);
>>   }
>> @@ -195,7 +195,7 @@ void find_pagesizes(void)
>>   	}
>>   	globfree(&g);
>>
>> -	if (read_sysfs(0, "/proc/sys/kernel/shmmax") < NUM_PAGES * largest)
>> +	if (thuge_read_sysfs(0, "/proc/sys/kernel/shmmax") < NUM_PAGES * largest)
>>   		ksft_exit_fail_msg("Please do echo %lu > /proc/sys/kernel/shmmax",
>>   				   largest * NUM_PAGES);
>>
>> diff --git a/tools/testing/selftests/mm/vm_util.c b/tools/testing/selftests/mm/vm_util.c
>> index 1357e2d6a7b6..d899c272e0ee 100644
>> --- a/tools/testing/selftests/mm/vm_util.c
>> +++ b/tools/testing/selftests/mm/vm_util.c
>> @@ -486,3 +486,41 @@ int close_procmap(struct procmap_fd *procmap)
>>   {
>>   	return close(procmap->fd);
>>   }
>> +
>> +int write_sysfs(const char *file_path, unsigned long val)
>> +{
>> +	FILE *f = fopen(file_path, "w");
>> +
>> +	if (!f) {
>> +		fprintf(stderr, "f %s\n", file_path);
>> +		perror("fopen");
>> +		return 1;
>> +	}
>> +	if (fprintf(f, "%lu", val) < 0) {
>> +		perror("fprintf");
>> +		fclose(f);
>> +		return 1;
>> +	}
>> +	fclose(f);
>> +
>> +	return 0;
>> +}
>> +
>> +int read_sysfs(const char *file_path, unsigned long *val)
>> +{
>> +	FILE *f = fopen(file_path, "r");
>> +
>> +	if (!f) {
>> +		fprintf(stderr, "f %s\n", file_path);
>> +		perror("fopen");
>> +		return 1;
>> +	}
>> +	if (fscanf(f, "%lu", val) != 1) {
>> +		perror("fscanf");
>> +		fclose(f);
>> +		return 1;
>> +	}
>> +	fclose(f);
>> +
>> +	return 0;
>> +}
>> diff --git a/tools/testing/selftests/mm/vm_util.h b/tools/testing/selftests/mm/vm_util.h
>> index 9211ba640d9c..f84c7c4680ea 100644
>> --- a/tools/testing/selftests/mm/vm_util.h
>> +++ b/tools/testing/selftests/mm/vm_util.h
>> @@ -87,6 +87,8 @@ int open_procmap(pid_t pid, struct procmap_fd *procmap_out);
>>   int query_procmap(struct procmap_fd *procmap);
>>   bool find_vma_procmap(struct procmap_fd *procmap, void *address);
>>   int close_procmap(struct procmap_fd *procmap);
>> +int write_sysfs(const char *file_path, unsigned long val);
>> +int read_sysfs(const char *file_path, unsigned long *val);
>>
>>   static inline int open_self_procmap(struct procmap_fd *procmap_out)
>>   {
>> --
>> 2.34.1
>>