Re: [PATCH v2] selftests: cachestat: Refactor test to remove duplication

[Suresh Chandrappa <suresh.k.chandrappa@gmail.com>]

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