* [PATCH v2] selftests: cachestat: Refactor test to remove duplication
@ 2025-06-11 18:09 Suresh K C
2025-06-25 4:58 ` Suresh Chandrappa
0 siblings, 1 reply; 5+ messages in thread
From: Suresh K C @ 2025-06-11 18:09 UTC (permalink / raw)
To: nphamcs, hannes, shuah
Cc: linux-mm, linux-kselftest, linux-kernel, Suresh K C
From: Suresh K C <suresh.k.chandrappa@gmail.com>
Refactored the mmap and shmem test logic into a common function
to reduce code duplication and improve maintainability
Changes in v2:
Refactored mmap and shmem tests into a common function
Renamed test function to run_cachestat_test()
Removed test for /proc/cpuinfo as a general /proc test case already exists
Signed-off-by: Suresh K C <suresh.k.chandrappa@gmail.com>
---
.../selftests/cachestat/test_cachestat.c | 97 ++++++-------------
1 file changed, 30 insertions(+), 67 deletions(-)
diff --git a/tools/testing/selftests/cachestat/test_cachestat.c b/tools/testing/selftests/cachestat/test_cachestat.c
index 81e7f6dd2279..7c2f64175943 100644
--- a/tools/testing/selftests/cachestat/test_cachestat.c
+++ b/tools/testing/selftests/cachestat/test_cachestat.c
@@ -22,7 +22,7 @@
static const char * const dev_files[] = {
"/dev/zero", "/dev/null", "/dev/urandom",
- "/proc/version","/proc/cpuinfo","/proc"
+ "/proc/version","/proc"
};
void print_cachestat(struct cachestat *cs)
@@ -33,6 +33,11 @@ void print_cachestat(struct cachestat *cs)
cs->nr_evicted, cs->nr_recently_evicted);
}
+enum file_type {
+ FILE_MMAP,
+ FILE_SHMEM
+};
+
bool write_exactly(int fd, size_t filesize)
{
int random_fd = open("/dev/urandom", O_RDONLY);
@@ -202,66 +207,8 @@ static int test_cachestat(const char *filename, bool write_random, bool create,
return ret;
}
-bool test_cachestat_mmap(void){
-
- size_t PS = sysconf(_SC_PAGESIZE);
- size_t filesize = PS * 512 * 2;;
- int syscall_ret;
- size_t compute_len = PS * 512;
- struct cachestat_range cs_range = { PS, compute_len };
- char *filename = "tmpshmcstat";
- unsigned long num_pages = compute_len / PS;
- struct cachestat cs;
- bool ret = true;
- int fd = open(filename, O_RDWR | O_CREAT | O_TRUNC, 0666);
- if (fd < 0) {
- ksft_print_msg("Unable to create mmap file.\n");
- ret = false;
- goto out;
- }
- if (ftruncate(fd, filesize)) {
- ksft_print_msg("Unable to truncate mmap file.\n");
- ret = false;
- goto close_fd;
- }
- if (!write_exactly(fd, filesize)) {
- ksft_print_msg("Unable to write to mmap file.\n");
- ret = false;
- goto close_fd;
- }
- char *map = mmap(NULL, filesize, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
- if (map == MAP_FAILED) {
- ksft_print_msg("mmap failed.\n");
- ret = false;
- goto close_fd;
- }
-
- for (int i = 0; i < filesize; i++) {
- map[i] = 'A';
- }
- map[filesize - 1] = 'X';
-
- syscall_ret = syscall(__NR_cachestat, fd, &cs_range, &cs, 0);
-
- if (syscall_ret) {
- ksft_print_msg("Cachestat returned non-zero.\n");
- ret = false;
- } else {
- print_cachestat(&cs);
- if (cs.nr_cache + cs.nr_evicted != num_pages) {
- ksft_print_msg("Total number of cached and evicted pages is off.\n");
- ret = false;
- }
- }
-
-close_fd:
- close(fd);
- unlink(filename);
-out:
- return ret;
-}
-bool test_cachestat_shmem(void)
+bool run_cachestat_test(enum file_type type)
{
size_t PS = sysconf(_SC_PAGESIZE);
size_t filesize = PS * 512 * 2; /* 2 2MB huge pages */
@@ -271,27 +218,43 @@ bool test_cachestat_shmem(void)
char *filename = "tmpshmcstat";
struct cachestat cs;
bool ret = true;
+ int fd;
unsigned long num_pages = compute_len / PS;
- int fd = shm_open(filename, O_CREAT | O_RDWR, 0600);
+ if (type == FILE_SHMEM)
+ fd = shm_open(filename, O_CREAT | O_RDWR, 0600);
+ else
+ fd = open(filename, O_RDWR | O_CREAT | O_TRUNC, 0666);
if (fd < 0) {
- ksft_print_msg("Unable to create shmem file.\n");
+ ksft_print_msg("Unable to create file.\n");
ret = false;
goto out;
}
if (ftruncate(fd, filesize)) {
- ksft_print_msg("Unable to truncate shmem file.\n");
+ ksft_print_msg("Unable to truncate file.\n");
ret = false;
goto close_fd;
}
if (!write_exactly(fd, filesize)) {
- ksft_print_msg("Unable to write to shmem file.\n");
+ ksft_print_msg("Unable to write to file.\n");
ret = false;
goto close_fd;
}
+ if (type == FILE_MMAP){
+ char *map = mmap(NULL, filesize, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
+ if (map == MAP_FAILED) {
+ ksft_print_msg("mmap failed.\n");
+ ret = false;
+ goto close_fd;
+ }
+ for (int i = 0; i < filesize; i++) {
+ map[i] = 'A';
+ }
+ map[filesize - 1] = 'X';
+ }
syscall_ret = syscall(__NR_cachestat, fd, &cs_range, &cs, 0);
if (syscall_ret) {
@@ -333,7 +296,7 @@ int main(void)
ret = 1;
}
- for (int i = 0; i < 6; i++) {
+ for (int i = 0; i < 5; i++) {
const char *dev_filename = dev_files[i];
if (test_cachestat(dev_filename, false, false, false,
@@ -367,14 +330,14 @@ int main(void)
break;
}
- if (test_cachestat_shmem())
+ if (run_cachestat_test(FILE_SHMEM))
ksft_test_result_pass("cachestat works with a shmem file\n");
else {
ksft_test_result_fail("cachestat fails with a shmem file\n");
ret = 1;
}
- if (test_cachestat_mmap())
+ if (run_cachestat_test(FILE_MMAP))
ksft_test_result_pass("cachestat works with a mmap file\n");
else {
ksft_test_result_fail("cachestat fails with a mmap file\n");
--
2.43.0
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2] selftests: cachestat: Refactor test to remove duplication
2025-06-11 18:09 [PATCH v2] selftests: cachestat: Refactor test to remove duplication Suresh K C
@ 2025-06-25 4:58 ` Suresh Chandrappa
2025-06-27 16:41 ` Nhat Pham
0 siblings, 1 reply; 5+ messages in thread
From: Suresh Chandrappa @ 2025-06-25 4:58 UTC (permalink / raw)
To: nphamcs, hannes, shuah; +Cc: linux-mm, linux-kselftest, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 6705 bytes --]
Hi @nphamcs
Can you please check the modified change
Thanks
Suresh K C
On Wed, 11 Jun 2025, 23:39 Suresh K C, <suresh.k.chandrappa@gmail.com>
wrote:
> From: Suresh K C <suresh.k.chandrappa@gmail.com>
>
> Refactored the mmap and shmem test logic into a common function
> to reduce code duplication and improve maintainability
>
> Changes in v2:
> Refactored mmap and shmem tests into a common function
> Renamed test function to run_cachestat_test()
> Removed test for /proc/cpuinfo as a general /proc test case already
> exists
>
> Signed-off-by: Suresh K C <suresh.k.chandrappa@gmail.com>
> ---
> .../selftests/cachestat/test_cachestat.c | 97 ++++++-------------
> 1 file changed, 30 insertions(+), 67 deletions(-)
>
> diff --git a/tools/testing/selftests/cachestat/test_cachestat.c
> b/tools/testing/selftests/cachestat/test_cachestat.c
> index 81e7f6dd2279..7c2f64175943 100644
> --- a/tools/testing/selftests/cachestat/test_cachestat.c
> +++ b/tools/testing/selftests/cachestat/test_cachestat.c
> @@ -22,7 +22,7 @@
>
> static const char * const dev_files[] = {
> "/dev/zero", "/dev/null", "/dev/urandom",
> - "/proc/version","/proc/cpuinfo","/proc"
> + "/proc/version","/proc"
> };
>
> void print_cachestat(struct cachestat *cs)
> @@ -33,6 +33,11 @@ void print_cachestat(struct cachestat *cs)
> cs->nr_evicted, cs->nr_recently_evicted);
> }
>
> +enum file_type {
> + FILE_MMAP,
> + FILE_SHMEM
> +};
> +
> bool write_exactly(int fd, size_t filesize)
> {
> int random_fd = open("/dev/urandom", O_RDONLY);
> @@ -202,66 +207,8 @@ static int test_cachestat(const char *filename, bool
> write_random, bool create,
> return ret;
> }
>
> -bool test_cachestat_mmap(void){
> -
> - size_t PS = sysconf(_SC_PAGESIZE);
> - size_t filesize = PS * 512 * 2;;
> - int syscall_ret;
> - size_t compute_len = PS * 512;
> - struct cachestat_range cs_range = { PS, compute_len };
> - char *filename = "tmpshmcstat";
> - unsigned long num_pages = compute_len / PS;
> - struct cachestat cs;
> - bool ret = true;
> - int fd = open(filename, O_RDWR | O_CREAT | O_TRUNC, 0666);
> - if (fd < 0) {
> - ksft_print_msg("Unable to create mmap file.\n");
> - ret = false;
> - goto out;
> - }
> - if (ftruncate(fd, filesize)) {
> - ksft_print_msg("Unable to truncate mmap file.\n");
> - ret = false;
> - goto close_fd;
> - }
> - if (!write_exactly(fd, filesize)) {
> - ksft_print_msg("Unable to write to mmap file.\n");
> - ret = false;
> - goto close_fd;
> - }
> - char *map = mmap(NULL, filesize, PROT_READ | PROT_WRITE,
> MAP_SHARED, fd, 0);
> - if (map == MAP_FAILED) {
> - ksft_print_msg("mmap failed.\n");
> - ret = false;
> - goto close_fd;
> - }
> -
> - for (int i = 0; i < filesize; i++) {
> - map[i] = 'A';
> - }
> - map[filesize - 1] = 'X';
> -
> - syscall_ret = syscall(__NR_cachestat, fd, &cs_range, &cs, 0);
> -
> - if (syscall_ret) {
> - ksft_print_msg("Cachestat returned non-zero.\n");
> - ret = false;
> - } else {
> - print_cachestat(&cs);
> - if (cs.nr_cache + cs.nr_evicted != num_pages) {
> - ksft_print_msg("Total number of cached and evicted
> pages is off.\n");
> - ret = false;
> - }
> - }
> -
> -close_fd:
> - close(fd);
> - unlink(filename);
> -out:
> - return ret;
> -}
>
> -bool test_cachestat_shmem(void)
> +bool run_cachestat_test(enum file_type type)
> {
> size_t PS = sysconf(_SC_PAGESIZE);
> size_t filesize = PS * 512 * 2; /* 2 2MB huge pages */
> @@ -271,27 +218,43 @@ bool test_cachestat_shmem(void)
> char *filename = "tmpshmcstat";
> struct cachestat cs;
> bool ret = true;
> + int fd;
> unsigned long num_pages = compute_len / PS;
> - int fd = shm_open(filename, O_CREAT | O_RDWR, 0600);
> + if (type == FILE_SHMEM)
> + fd = shm_open(filename, O_CREAT | O_RDWR, 0600);
> + else
> + fd = open(filename, O_RDWR | O_CREAT | O_TRUNC, 0666);
>
> if (fd < 0) {
> - ksft_print_msg("Unable to create shmem file.\n");
> + ksft_print_msg("Unable to create file.\n");
> ret = false;
> goto out;
> }
>
> if (ftruncate(fd, filesize)) {
> - ksft_print_msg("Unable to truncate shmem file.\n");
> + ksft_print_msg("Unable to truncate file.\n");
> ret = false;
> goto close_fd;
> }
>
> if (!write_exactly(fd, filesize)) {
> - ksft_print_msg("Unable to write to shmem file.\n");
> + ksft_print_msg("Unable to write to file.\n");
> ret = false;
> goto close_fd;
> }
>
> + if (type == FILE_MMAP){
> + char *map = mmap(NULL, filesize, PROT_READ | PROT_WRITE,
> MAP_SHARED, fd, 0);
> + if (map == MAP_FAILED) {
> + ksft_print_msg("mmap failed.\n");
> + ret = false;
> + goto close_fd;
> + }
> + for (int i = 0; i < filesize; i++) {
> + map[i] = 'A';
> + }
> + map[filesize - 1] = 'X';
> + }
> syscall_ret = syscall(__NR_cachestat, fd, &cs_range, &cs, 0);
>
> if (syscall_ret) {
> @@ -333,7 +296,7 @@ int main(void)
> ret = 1;
> }
>
> - for (int i = 0; i < 6; i++) {
> + for (int i = 0; i < 5; i++) {
> const char *dev_filename = dev_files[i];
>
> if (test_cachestat(dev_filename, false, false, false,
> @@ -367,14 +330,14 @@ int main(void)
> break;
> }
>
> - if (test_cachestat_shmem())
> + if (run_cachestat_test(FILE_SHMEM))
> ksft_test_result_pass("cachestat works with a shmem
> file\n");
> else {
> ksft_test_result_fail("cachestat fails with a shmem
> file\n");
> ret = 1;
> }
>
> - if (test_cachestat_mmap())
> + if (run_cachestat_test(FILE_MMAP))
> ksft_test_result_pass("cachestat works with a mmap
> file\n");
> else {
> ksft_test_result_fail("cachestat fails with a mmap
> file\n");
> --
> 2.43.0
>
>
[-- Attachment #2: Type: text/html, Size: 8875 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] selftests: cachestat: Refactor test to remove duplication
2025-06-25 4:58 ` Suresh Chandrappa
@ 2025-06-27 16:41 ` Nhat Pham
2025-06-30 17:37 ` Suresh Chandrappa
0 siblings, 1 reply; 5+ messages in thread
From: Nhat Pham @ 2025-06-27 16:41 UTC (permalink / raw)
To: Suresh Chandrappa; +Cc: hannes, shuah, linux-mm, linux-kselftest, linux-kernel
On Tue, Jun 24, 2025 at 9:58 PM Suresh Chandrappa
<suresh.k.chandrappa@gmail.com> wrote:
>
> Hi @nphamcs
> Can you please check the modified change
Is this supposed to be on top of the earlier patch you sent out? In
that case, you should send both together as a patch series.
>
> Thanks
> Suresh K C
>
>
> On Wed, 11 Jun 2025, 23:39 Suresh K C, <suresh.k.chandrappa@gmail.com> wrote:
>>
>> From: Suresh K C <suresh.k.chandrappa@gmail.com>
>>
>> Refactored the mmap and shmem test logic into a common function
>> to reduce code duplication and improve maintainability
>>
>> Changes in v2:
>> Refactored mmap and shmem tests into a common function
>> Renamed test function to run_cachestat_test()
>> Removed test for /proc/cpuinfo as a general /proc test case already exists
>>
>> Signed-off-by: Suresh K C <suresh.k.chandrappa@gmail.com>
>> ---
>> .../selftests/cachestat/test_cachestat.c | 97 ++++++-------------
>> 1 file changed, 30 insertions(+), 67 deletions(-)
>>
>> diff --git a/tools/testing/selftests/cachestat/test_cachestat.c b/tools/testing/selftests/cachestat/test_cachestat.c
>> index 81e7f6dd2279..7c2f64175943 100644
>> --- a/tools/testing/selftests/cachestat/test_cachestat.c
>> +++ b/tools/testing/selftests/cachestat/test_cachestat.c
>> @@ -22,7 +22,7 @@
>>
>> static const char * const dev_files[] = {
>> "/dev/zero", "/dev/null", "/dev/urandom",
>> - "/proc/version","/proc/cpuinfo","/proc"
>> + "/proc/version","/proc"
So you removed one file that you added in an earlier patch, right?
Then why bother adding it in the first place...?
Can you either:
1. Send the two patch together as a series. In the first patch, do not
add /proc/cpuinfo.
or
2. Squash them into a single patch. I'll let you decide if this is worth it.
>> };
>>
>> void print_cachestat(struct cachestat *cs)
>> @@ -33,6 +33,11 @@ void print_cachestat(struct cachestat *cs)
>> cs->nr_evicted, cs->nr_recently_evicted);
>> }
>>
>> +enum file_type {
>> + FILE_MMAP,
>> + FILE_SHMEM
>> +};
>> +
>> bool write_exactly(int fd, size_t filesize)
>> {
>> int random_fd = open("/dev/urandom", O_RDONLY);
>> @@ -202,66 +207,8 @@ static int test_cachestat(const char *filename, bool write_random, bool create,
>> return ret;
>> }
>>
>> -bool test_cachestat_mmap(void){
>> -
>> - size_t PS = sysconf(_SC_PAGESIZE);
>> - size_t filesize = PS * 512 * 2;;
>> - int syscall_ret;
>> - size_t compute_len = PS * 512;
>> - struct cachestat_range cs_range = { PS, compute_len };
>> - char *filename = "tmpshmcstat";
>> - unsigned long num_pages = compute_len / PS;
>> - struct cachestat cs;
>> - bool ret = true;
>> - int fd = open(filename, O_RDWR | O_CREAT | O_TRUNC, 0666);
>> - if (fd < 0) {
>> - ksft_print_msg("Unable to create mmap file.\n");
>> - ret = false;
>> - goto out;
>> - }
>> - if (ftruncate(fd, filesize)) {
>> - ksft_print_msg("Unable to truncate mmap file.\n");
>> - ret = false;
>> - goto close_fd;
>> - }
>> - if (!write_exactly(fd, filesize)) {
>> - ksft_print_msg("Unable to write to mmap file.\n");
>> - ret = false;
>> - goto close_fd;
>> - }
>> - char *map = mmap(NULL, filesize, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
>> - if (map == MAP_FAILED) {
>> - ksft_print_msg("mmap failed.\n");
>> - ret = false;
>> - goto close_fd;
>> - }
>> -
>> - for (int i = 0; i < filesize; i++) {
>> - map[i] = 'A';
>> - }
>> - map[filesize - 1] = 'X';
>> -
>> - syscall_ret = syscall(__NR_cachestat, fd, &cs_range, &cs, 0);
>> -
>> - if (syscall_ret) {
>> - ksft_print_msg("Cachestat returned non-zero.\n");
>> - ret = false;
>> - } else {
>> - print_cachestat(&cs);
>> - if (cs.nr_cache + cs.nr_evicted != num_pages) {
>> - ksft_print_msg("Total number of cached and evicted pages is off.\n");
>> - ret = false;
>> - }
>> - }
>> -
>> -close_fd:
>> - close(fd);
>> - unlink(filename);
>> -out:
>> - return ret;
>> -}
>>
>> -bool test_cachestat_shmem(void)
>> +bool run_cachestat_test(enum file_type type)
Can you just call this function test_cachestat(enum file_type type) ?
>> {
>> size_t PS = sysconf(_SC_PAGESIZE);
>> size_t filesize = PS * 512 * 2; /* 2 2MB huge pages */
>> @@ -271,27 +218,43 @@ bool test_cachestat_shmem(void)
>> char *filename = "tmpshmcstat";
>> struct cachestat cs;
>> bool ret = true;
>> + int fd;
>> unsigned long num_pages = compute_len / PS;
>> - int fd = shm_open(filename, O_CREAT | O_RDWR, 0600);
>> + if (type == FILE_SHMEM)
>> + fd = shm_open(filename, O_CREAT | O_RDWR, 0600);
>> + else
>> + fd = open(filename, O_RDWR | O_CREAT | O_TRUNC, 0666);
>>
>> if (fd < 0) {
>> - ksft_print_msg("Unable to create shmem file.\n");
>> + ksft_print_msg("Unable to create file.\n");
>> ret = false;
>> goto out;
>> }
>>
>> if (ftruncate(fd, filesize)) {
>> - ksft_print_msg("Unable to truncate shmem file.\n");
>> + ksft_print_msg("Unable to truncate file.\n");
>> ret = false;
>> goto close_fd;
>> }
>>
>> if (!write_exactly(fd, filesize)) {
>> - ksft_print_msg("Unable to write to shmem file.\n");
>> + ksft_print_msg("Unable to write to file.\n");
>> ret = false;
>> goto close_fd;
>> }
>>
>> + if (type == FILE_MMAP){
>> + char *map = mmap(NULL, filesize, PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
>> + if (map == MAP_FAILED) {
>> + ksft_print_msg("mmap failed.\n");
>> + ret = false;
>> + goto close_fd;
>> + }
>> + for (int i = 0; i < filesize; i++) {
>> + map[i] = 'A';
>> + }
>> + map[filesize - 1] = 'X';
>> + }
>> syscall_ret = syscall(__NR_cachestat, fd, &cs_range, &cs, 0);
>>
>> if (syscall_ret) {
>> @@ -333,7 +296,7 @@ int main(void)
>> ret = 1;
>> }
>>
>> - for (int i = 0; i < 6; i++) {
>> + for (int i = 0; i < 5; i++) {
>> const char *dev_filename = dev_files[i];
>>
>> if (test_cachestat(dev_filename, false, false, false,
>> @@ -367,14 +330,14 @@ int main(void)
>> break;
>> }
>>
>> - if (test_cachestat_shmem())
>> + if (run_cachestat_test(FILE_SHMEM))
>> ksft_test_result_pass("cachestat works with a shmem file\n");
>> else {
>> ksft_test_result_fail("cachestat fails with a shmem file\n");
>> ret = 1;
>> }
>>
>> - if (test_cachestat_mmap())
>> + if (run_cachestat_test(FILE_MMAP))
>> ksft_test_result_pass("cachestat works with a mmap file\n");
>> else {
>> ksft_test_result_fail("cachestat fails with a mmap file\n");
>> --
>> 2.43.0
>>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] selftests: cachestat: Refactor test to remove duplication
2025-06-27 16:41 ` Nhat Pham
@ 2025-06-30 17:37 ` Suresh Chandrappa
2025-06-30 17:47 ` Suresh Chandrappa
0 siblings, 1 reply; 5+ messages in thread
From: Suresh Chandrappa @ 2025-06-30 17:37 UTC (permalink / raw)
To: Nhat Pham; +Cc: hannes, shuah, linux-mm
[-- Attachment #1: Type: text/plain, Size: 8341 bytes --]
Hi Nhat,
Sure i will squash them to a single patch and will share it.
Thanks
Suresh K C
On Fri, Jun 27, 2025 at 10:11 PM Nhat Pham <nphamcs@gmail.com> wrote:
> On Tue, Jun 24, 2025 at 9:58 PM Suresh Chandrappa
> <suresh.k.chandrappa@gmail.com> wrote:
> >
> > Hi @nphamcs
> > Can you please check the modified change
>
> Is this supposed to be on top of the earlier patch you sent out? In
> that case, you should send both together as a patch series.
>
> >
> > Thanks
> > Suresh K C
> >
> >
> > On Wed, 11 Jun 2025, 23:39 Suresh K C, <suresh.k.chandrappa@gmail.com>
> wrote:
> >>
> >> From: Suresh K C <suresh.k.chandrappa@gmail.com>
> >>
> >> Refactored the mmap and shmem test logic into a common function
> >> to reduce code duplication and improve maintainability
> >>
> >> Changes in v2:
> >> Refactored mmap and shmem tests into a common function
> >> Renamed test function to run_cachestat_test()
> >> Removed test for /proc/cpuinfo as a general /proc test case already
> exists
> >>
> >> Signed-off-by: Suresh K C <suresh.k.chandrappa@gmail.com>
> >> ---
> >> .../selftests/cachestat/test_cachestat.c | 97 ++++++-------------
> >> 1 file changed, 30 insertions(+), 67 deletions(-)
> >>
> >> diff --git a/tools/testing/selftests/cachestat/test_cachestat.c
> b/tools/testing/selftests/cachestat/test_cachestat.c
> >> index 81e7f6dd2279..7c2f64175943 100644
> >> --- a/tools/testing/selftests/cachestat/test_cachestat.c
> >> +++ b/tools/testing/selftests/cachestat/test_cachestat.c
> >> @@ -22,7 +22,7 @@
> >>
> >> static const char * const dev_files[] = {
> >> "/dev/zero", "/dev/null", "/dev/urandom",
> >> - "/proc/version","/proc/cpuinfo","/proc"
> >> + "/proc/version","/proc"
>
> So you removed one file that you added in an earlier patch, right?
> Then why bother adding it in the first place...?
>
> Can you either:
>
> 1. Send the two patch together as a series. In the first patch, do not
> add /proc/cpuinfo.
>
> or
>
> 2. Squash them into a single patch. I'll let you decide if this is worth
> it.
>
>
> >> };
> >>
> >> void print_cachestat(struct cachestat *cs)
> >> @@ -33,6 +33,11 @@ void print_cachestat(struct cachestat *cs)
> >> cs->nr_evicted, cs->nr_recently_evicted);
> >> }
> >>
> >> +enum file_type {
> >> + FILE_MMAP,
> >> + FILE_SHMEM
> >> +};
> >> +
> >> bool write_exactly(int fd, size_t filesize)
> >> {
> >> int random_fd = open("/dev/urandom", O_RDONLY);
> >> @@ -202,66 +207,8 @@ static int test_cachestat(const char *filename,
> bool write_random, bool create,
> >> return ret;
> >> }
> >>
> >> -bool test_cachestat_mmap(void){
> >> -
> >> - size_t PS = sysconf(_SC_PAGESIZE);
> >> - size_t filesize = PS * 512 * 2;;
> >> - int syscall_ret;
> >> - size_t compute_len = PS * 512;
> >> - struct cachestat_range cs_range = { PS, compute_len };
> >> - char *filename = "tmpshmcstat";
> >> - unsigned long num_pages = compute_len / PS;
> >> - struct cachestat cs;
> >> - bool ret = true;
> >> - int fd = open(filename, O_RDWR | O_CREAT | O_TRUNC, 0666);
> >> - if (fd < 0) {
> >> - ksft_print_msg("Unable to create mmap file.\n");
> >> - ret = false;
> >> - goto out;
> >> - }
> >> - if (ftruncate(fd, filesize)) {
> >> - ksft_print_msg("Unable to truncate mmap file.\n");
> >> - ret = false;
> >> - goto close_fd;
> >> - }
> >> - if (!write_exactly(fd, filesize)) {
> >> - ksft_print_msg("Unable to write to mmap file.\n");
> >> - ret = false;
> >> - goto close_fd;
> >> - }
> >> - char *map = mmap(NULL, filesize, PROT_READ | PROT_WRITE,
> MAP_SHARED, fd, 0);
> >> - if (map == MAP_FAILED) {
> >> - ksft_print_msg("mmap failed.\n");
> >> - ret = false;
> >> - goto close_fd;
> >> - }
> >> -
> >> - for (int i = 0; i < filesize; i++) {
> >> - map[i] = 'A';
> >> - }
> >> - map[filesize - 1] = 'X';
> >> -
> >> - syscall_ret = syscall(__NR_cachestat, fd, &cs_range, &cs, 0);
> >> -
> >> - if (syscall_ret) {
> >> - ksft_print_msg("Cachestat returned non-zero.\n");
> >> - ret = false;
> >> - } else {
> >> - print_cachestat(&cs);
> >> - if (cs.nr_cache + cs.nr_evicted != num_pages) {
> >> - ksft_print_msg("Total number of cached and
> evicted pages is off.\n");
> >> - ret = false;
> >> - }
> >> - }
> >> -
> >> -close_fd:
> >> - close(fd);
> >> - unlink(filename);
> >> -out:
> >> - return ret;
> >> -}
> >>
> >> -bool test_cachestat_shmem(void)
> >> +bool run_cachestat_test(enum file_type type)
>
> Can you just call this function test_cachestat(enum file_type type) ?
>
> >> {
> >> size_t PS = sysconf(_SC_PAGESIZE);
> >> size_t filesize = PS * 512 * 2; /* 2 2MB huge pages */
> >> @@ -271,27 +218,43 @@ bool test_cachestat_shmem(void)
> >> char *filename = "tmpshmcstat";
> >> struct cachestat cs;
> >> bool ret = true;
> >> + int fd;
> >> unsigned long num_pages = compute_len / PS;
> >> - int fd = shm_open(filename, O_CREAT | O_RDWR, 0600);
> >> + if (type == FILE_SHMEM)
> >> + fd = shm_open(filename, O_CREAT | O_RDWR, 0600);
> >> + else
> >> + fd = open(filename, O_RDWR | O_CREAT | O_TRUNC, 0666);
> >>
> >> if (fd < 0) {
> >> - ksft_print_msg("Unable to create shmem file.\n");
> >> + ksft_print_msg("Unable to create file.\n");
> >> ret = false;
> >> goto out;
> >> }
> >>
> >> if (ftruncate(fd, filesize)) {
> >> - ksft_print_msg("Unable to truncate shmem file.\n");
> >> + ksft_print_msg("Unable to truncate file.\n");
> >> ret = false;
> >> goto close_fd;
> >> }
> >>
> >> if (!write_exactly(fd, filesize)) {
> >> - ksft_print_msg("Unable to write to shmem file.\n");
> >> + ksft_print_msg("Unable to write to file.\n");
> >> ret = false;
> >> goto close_fd;
> >> }
> >>
> >> + if (type == FILE_MMAP){
> >> + char *map = mmap(NULL, filesize, PROT_READ |
> PROT_WRITE, MAP_SHARED, fd, 0);
> >> + if (map == MAP_FAILED) {
> >> + ksft_print_msg("mmap failed.\n");
> >> + ret = false;
> >> + goto close_fd;
> >> + }
> >> + for (int i = 0; i < filesize; i++) {
> >> + map[i] = 'A';
> >> + }
> >> + map[filesize - 1] = 'X';
> >> + }
> >> syscall_ret = syscall(__NR_cachestat, fd, &cs_range, &cs, 0);
> >>
> >> if (syscall_ret) {
> >> @@ -333,7 +296,7 @@ int main(void)
> >> ret = 1;
> >> }
> >>
> >> - for (int i = 0; i < 6; i++) {
> >> + for (int i = 0; i < 5; i++) {
> >> const char *dev_filename = dev_files[i];
> >>
> >> if (test_cachestat(dev_filename, false, false, false,
> >> @@ -367,14 +330,14 @@ int main(void)
> >> break;
> >> }
> >>
> >> - if (test_cachestat_shmem())
> >> + if (run_cachestat_test(FILE_SHMEM))
> >> ksft_test_result_pass("cachestat works with a shmem
> file\n");
> >> else {
> >> ksft_test_result_fail("cachestat fails with a shmem
> file\n");
> >> ret = 1;
> >> }
> >>
> >> - if (test_cachestat_mmap())
> >> + if (run_cachestat_test(FILE_MMAP))
> >> ksft_test_result_pass("cachestat works with a mmap
> file\n");
> >> else {
> >> ksft_test_result_fail("cachestat fails with a mmap
> file\n");
> >> --
> >> 2.43.0
> >>
>
[-- Attachment #2: Type: text/html, Size: 11659 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2] selftests: cachestat: Refactor test to remove duplication
2025-06-30 17:37 ` Suresh Chandrappa
@ 2025-06-30 17:47 ` Suresh Chandrappa
0 siblings, 0 replies; 5+ messages in thread
From: Suresh Chandrappa @ 2025-06-30 17:47 UTC (permalink / raw)
To: Nhat Pham; +Cc: hannes, shuah, linux-mm
[-- Attachment #1: Type: text/plain, Size: 9045 bytes --]
Hi Nhat,
>> -bool test_cachestat_shmem(void)
>> +bool run_cachestat_test(enum file_type type)
Can you just call this function test_cachestat(enum file_type type) ?
We already have a function named *test_cachestat* that was introduced
earlier in the codebase, so to avoid confusion or naming conflicts, I’ve
named this one run_cachestat_test
On Mon, Jun 30, 2025 at 11:07 PM Suresh Chandrappa <
suresh.k.chandrappa@gmail.com> wrote:
> Hi Nhat,
> Sure i will squash them to a single patch and will share it.
>
> Thanks
> Suresh K C
>
> On Fri, Jun 27, 2025 at 10:11 PM Nhat Pham <nphamcs@gmail.com> wrote:
>
>> On Tue, Jun 24, 2025 at 9:58 PM Suresh Chandrappa
>> <suresh.k.chandrappa@gmail.com> wrote:
>> >
>> > Hi @nphamcs
>> > Can you please check the modified change
>>
>> Is this supposed to be on top of the earlier patch you sent out? In
>> that case, you should send both together as a patch series.
>>
>> >
>> > Thanks
>> > Suresh K C
>> >
>> >
>> > On Wed, 11 Jun 2025, 23:39 Suresh K C, <suresh.k.chandrappa@gmail.com>
>> wrote:
>> >>
>> >> From: Suresh K C <suresh.k.chandrappa@gmail.com>
>> >>
>> >> Refactored the mmap and shmem test logic into a common function
>> >> to reduce code duplication and improve maintainability
>> >>
>> >> Changes in v2:
>> >> Refactored mmap and shmem tests into a common function
>> >> Renamed test function to run_cachestat_test()
>> >> Removed test for /proc/cpuinfo as a general /proc test case
>> already exists
>> >>
>> >> Signed-off-by: Suresh K C <suresh.k.chandrappa@gmail.com>
>> >> ---
>> >> .../selftests/cachestat/test_cachestat.c | 97 ++++++-------------
>> >> 1 file changed, 30 insertions(+), 67 deletions(-)
>> >>
>> >> diff --git a/tools/testing/selftests/cachestat/test_cachestat.c
>> b/tools/testing/selftests/cachestat/test_cachestat.c
>> >> index 81e7f6dd2279..7c2f64175943 100644
>> >> --- a/tools/testing/selftests/cachestat/test_cachestat.c
>> >> +++ b/tools/testing/selftests/cachestat/test_cachestat.c
>> >> @@ -22,7 +22,7 @@
>> >>
>> >> static const char * const dev_files[] = {
>> >> "/dev/zero", "/dev/null", "/dev/urandom",
>> >> - "/proc/version","/proc/cpuinfo","/proc"
>> >> + "/proc/version","/proc"
>>
>> So you removed one file that you added in an earlier patch, right?
>> Then why bother adding it in the first place...?
>>
>> Can you either:
>>
>> 1. Send the two patch together as a series. In the first patch, do not
>> add /proc/cpuinfo.
>>
>> or
>>
>> 2. Squash them into a single patch. I'll let you decide if this is worth
>> it.
>>
>>
>> >> };
>> >>
>> >> void print_cachestat(struct cachestat *cs)
>> >> @@ -33,6 +33,11 @@ void print_cachestat(struct cachestat *cs)
>> >> cs->nr_evicted, cs->nr_recently_evicted);
>> >> }
>> >>
>> >> +enum file_type {
>> >> + FILE_MMAP,
>> >> + FILE_SHMEM
>> >> +};
>> >> +
>> >> bool write_exactly(int fd, size_t filesize)
>> >> {
>> >> int random_fd = open("/dev/urandom", O_RDONLY);
>> >> @@ -202,66 +207,8 @@ static int test_cachestat(const char *filename,
>> bool write_random, bool create,
>> >> return ret;
>> >> }
>> >>
>> >> -bool test_cachestat_mmap(void){
>> >> -
>> >> - size_t PS = sysconf(_SC_PAGESIZE);
>> >> - size_t filesize = PS * 512 * 2;;
>> >> - int syscall_ret;
>> >> - size_t compute_len = PS * 512;
>> >> - struct cachestat_range cs_range = { PS, compute_len };
>> >> - char *filename = "tmpshmcstat";
>> >> - unsigned long num_pages = compute_len / PS;
>> >> - struct cachestat cs;
>> >> - bool ret = true;
>> >> - int fd = open(filename, O_RDWR | O_CREAT | O_TRUNC, 0666);
>> >> - if (fd < 0) {
>> >> - ksft_print_msg("Unable to create mmap file.\n");
>> >> - ret = false;
>> >> - goto out;
>> >> - }
>> >> - if (ftruncate(fd, filesize)) {
>> >> - ksft_print_msg("Unable to truncate mmap file.\n");
>> >> - ret = false;
>> >> - goto close_fd;
>> >> - }
>> >> - if (!write_exactly(fd, filesize)) {
>> >> - ksft_print_msg("Unable to write to mmap file.\n");
>> >> - ret = false;
>> >> - goto close_fd;
>> >> - }
>> >> - char *map = mmap(NULL, filesize, PROT_READ | PROT_WRITE,
>> MAP_SHARED, fd, 0);
>> >> - if (map == MAP_FAILED) {
>> >> - ksft_print_msg("mmap failed.\n");
>> >> - ret = false;
>> >> - goto close_fd;
>> >> - }
>> >> -
>> >> - for (int i = 0; i < filesize; i++) {
>> >> - map[i] = 'A';
>> >> - }
>> >> - map[filesize - 1] = 'X';
>> >> -
>> >> - syscall_ret = syscall(__NR_cachestat, fd, &cs_range, &cs, 0);
>> >> -
>> >> - if (syscall_ret) {
>> >> - ksft_print_msg("Cachestat returned non-zero.\n");
>> >> - ret = false;
>> >> - } else {
>> >> - print_cachestat(&cs);
>> >> - if (cs.nr_cache + cs.nr_evicted != num_pages) {
>> >> - ksft_print_msg("Total number of cached and
>> evicted pages is off.\n");
>> >> - ret = false;
>> >> - }
>> >> - }
>> >> -
>> >> -close_fd:
>> >> - close(fd);
>> >> - unlink(filename);
>> >> -out:
>> >> - return ret;
>> >> -}
>> >>
>> >> -bool test_cachestat_shmem(void)
>> >> +bool run_cachestat_test(enum file_type type)
>>
>> Can you just call this function test_cachestat(enum file_type type) ?
>>
>> >> {
>> >> size_t PS = sysconf(_SC_PAGESIZE);
>> >> size_t filesize = PS * 512 * 2; /* 2 2MB huge pages */
>> >> @@ -271,27 +218,43 @@ bool test_cachestat_shmem(void)
>> >> char *filename = "tmpshmcstat";
>> >> struct cachestat cs;
>> >> bool ret = true;
>> >> + int fd;
>> >> unsigned long num_pages = compute_len / PS;
>> >> - int fd = shm_open(filename, O_CREAT | O_RDWR, 0600);
>> >> + if (type == FILE_SHMEM)
>> >> + fd = shm_open(filename, O_CREAT | O_RDWR, 0600);
>> >> + else
>> >> + fd = open(filename, O_RDWR | O_CREAT | O_TRUNC, 0666);
>> >>
>> >> if (fd < 0) {
>> >> - ksft_print_msg("Unable to create shmem file.\n");
>> >> + ksft_print_msg("Unable to create file.\n");
>> >> ret = false;
>> >> goto out;
>> >> }
>> >>
>> >> if (ftruncate(fd, filesize)) {
>> >> - ksft_print_msg("Unable to truncate shmem file.\n");
>> >> + ksft_print_msg("Unable to truncate file.\n");
>> >> ret = false;
>> >> goto close_fd;
>> >> }
>> >>
>> >> if (!write_exactly(fd, filesize)) {
>> >> - ksft_print_msg("Unable to write to shmem file.\n");
>> >> + ksft_print_msg("Unable to write to file.\n");
>> >> ret = false;
>> >> goto close_fd;
>> >> }
>> >>
>> >> + if (type == FILE_MMAP){
>> >> + char *map = mmap(NULL, filesize, PROT_READ |
>> PROT_WRITE, MAP_SHARED, fd, 0);
>> >> + if (map == MAP_FAILED) {
>> >> + ksft_print_msg("mmap failed.\n");
>> >> + ret = false;
>> >> + goto close_fd;
>> >> + }
>> >> + for (int i = 0; i < filesize; i++) {
>> >> + map[i] = 'A';
>> >> + }
>> >> + map[filesize - 1] = 'X';
>> >> + }
>> >> syscall_ret = syscall(__NR_cachestat, fd, &cs_range, &cs, 0);
>> >>
>> >> if (syscall_ret) {
>> >> @@ -333,7 +296,7 @@ int main(void)
>> >> ret = 1;
>> >> }
>> >>
>> >> - for (int i = 0; i < 6; i++) {
>> >> + for (int i = 0; i < 5; i++) {
>> >> const char *dev_filename = dev_files[i];
>> >>
>> >> if (test_cachestat(dev_filename, false, false, false,
>> >> @@ -367,14 +330,14 @@ int main(void)
>> >> break;
>> >> }
>> >>
>> >> - if (test_cachestat_shmem())
>> >> + if (run_cachestat_test(FILE_SHMEM))
>> >> ksft_test_result_pass("cachestat works with a shmem
>> file\n");
>> >> else {
>> >> ksft_test_result_fail("cachestat fails with a shmem
>> file\n");
>> >> ret = 1;
>> >> }
>> >>
>> >> - if (test_cachestat_mmap())
>> >> + if (run_cachestat_test(FILE_MMAP))
>> >> ksft_test_result_pass("cachestat works with a mmap
>> file\n");
>> >> else {
>> >> ksft_test_result_fail("cachestat fails with a mmap
>> file\n");
>> >> --
>> >> 2.43.0
>> >>
>>
>
[-- Attachment #2: Type: text/html, Size: 12646 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2025-06-30 17:47 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-11 18:09 [PATCH v2] selftests: cachestat: Refactor test to remove duplication Suresh K C
2025-06-25 4:58 ` Suresh Chandrappa
2025-06-27 16:41 ` Nhat Pham
2025-06-30 17:37 ` Suresh Chandrappa
2025-06-30 17:47 ` Suresh Chandrappa
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).