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 wrote: > >> On Tue, Jun 24, 2025 at 9:58 PM Suresh Chandrappa >> 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, >> wrote: >> >> >> >> From: Suresh K C >> >> >> >> 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 >> >> --- >> >> .../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 >> >> >> >