Re: [LTP] [PATCH 02/13] Hugetlb: Migrating libhugetlbfs mmap-cow

[Tarun Sahu <tsahu@linux.ibm.com>]

Hi Li,
Thanks for reviewing the patch.
Please find my comments inline.

--skip
> > +static void do_work(int thread, size_t size, int fd)
> > +{
> > +       char *addr;
> > +       size_t i;
> > +       char pattern = thread+65;
> > +
> > +       addr = SAFE_MMAP(NULL, size, PROT_READ|PROT_WRITE, MAP_PRIVATE,
> > fd, 0);
> > +
> > +       tst_res(TINFO, "Thread %d (pid %d): Mapped at address %p",
> > +              thread, getpid(), addr);
> > +
> > +       for (i = 0; i < size; i++)
> > +               memcpy((char *)addr+i, &pattern, 1);
> > +
> > +       if (msync(addr, size, MS_SYNC))
> > +               tst_brk(TBROK|TERRNO, "Thread %d (pid %d): msync() failed",
> > +                               thread, getpid());
> > +
> > +       for (i = 0; i < size; i++)
> > +               if (addr[i] != pattern) {
> > +                       tst_res(TFAIL, "thread %d (pid: %d): Corruption at
> > %p; "
> > +                                  "Got %c, Expected %c", thread, getpid(),
> > +                                  &addr[i], addr[i], pattern);
> > +                       goto cleanup;
> > +               }
> > +       tst_res(TINFO, "Thread %d (pid %d): Pattern verified",
> > +                       thread, getpid());
> >
> 
> Maybe combining the address output with the content of patterns is better?
> 
> i.e.
> 
> --- a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap21.c
> +++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap21.c
> @@ -45,9 +45,6 @@ static void do_work(int thread, size_t size, int fd)
> 
>         addr = SAFE_MMAP(NULL, size, PROT_READ|PROT_WRITE, MAP_PRIVATE, fd,
> 0);
> 
> -       tst_res(TINFO, "Thread %d (pid %d): Mapped at address %p",
> -              thread, getpid(), addr);
> -
>         for (i = 0; i < size; i++)
>                 memcpy((char *)addr+i, &pattern, 1);
> 
> @@ -62,8 +59,8 @@ static void do_work(int thread, size_t size, int fd)
>                                    &addr[i], addr[i], pattern);
>                         goto cleanup;
>                 }
> -       tst_res(TINFO, "Thread %d (pid %d): Pattern verified",
> -                       thread, getpid());
> +       tst_res(TINFO, "Thread %d (pid %d): Pattern %c verified at address
> %p",
> +                       thread, getpid(), pattern, addr);
> 
>  cleanup:
>         SAFE_MUNMAP(addr, size);
> 
In case of failure, this verified comment will not get printed.
In case of success, I dont think it matters to show the content of pattern.
> 
> 
> > +
> > +cleanup:
> > +       SAFE_MUNMAP(addr, size);
> > +       exit(0);
> > +}
> > +
> > +static void run_test(void)
> > +{
> > +       char *addr;
> > +       size_t size, itr;
> > +       int i, pid;
> > +       pid_t *wait_list;
> > +
> > +       wait_list = SAFE_MALLOC(THREADS * sizeof(pid_t));
> > +       size = (NR_HUGEPAGES / (THREADS+1)) * hpage_size;
> > +
> > +
> > +       /* First, mmap the file with MAP_SHARED and fill with data
> > +        * If this is not done, then the fault handler will not be
> > +        * called in the kernel since private mappings will be
> > +        * created for the children at prefault time.
> > +        */
> > +       addr = SAFE_MMAP(NULL, size, PROT_READ|PROT_WRITE, MAP_SHARED, fd,
> > 0);
> > +
> > +       for (itr = 0; itr < size; itr += 8)
> > +               memcpy(addr+itr, "deadbeef", 8);
> > +
> > +       for (i = 0; i < THREADS; i++) {
> > +               pid = SAFE_FORK();
> > +
> > +               if (pid == 0)
> > +                       do_work(i, size, fd);
> > +
> > +               wait_list[i] = pid;
> > +       }
> > +       tst_reap_children();
> > +
> > +       SAFE_MUNMAP(addr, size);
> > +       free(wait_list);
> > +       tst_res(TPASS, "mmap COW working as expected.");
> > +}
> > +
> > +static void setup(void)
> > +{
> > +       hpage_size = SAFE_READ_MEMINFO(MEMINFO_HPAGE_SIZE)*1024;
> >
> 
> We do have a dedicated function named tst_get_hugepage_size();
> could be used for getting huge page size.
Oh right! I could have used it in all other previous tests too.
Thanks for pointing this, Will take care of it in next revision.
> 
> The rest part looks good to me.
> 
> Reviewed-by: Li Wang <liwang@redhat.com>
> 
> 
> 
> > +       fd = tst_creat_unlinked(MNTPOINT, 0);
> > +}
> > +
> > +static void cleanup(void)
> > +{
> > +       if (fd >= 1)
> > +               SAFE_CLOSE(fd);
> > +}
> > +
> > +static struct tst_test test = {
> > +       .needs_root = 1,
> > +       .mntpoint = MNTPOINT,
> > +       .needs_hugetlbfs = 1,
> > +       .needs_tmpdir = 1,
> > +       .forks_child = 1,
> > +       .setup = setup,
> > +       .cleanup = cleanup,
> > +       .test_all = run_test,
> > +       .hugepages = {NR_HUGEPAGES, TST_NEEDS},
> > +};
> > --
> > 2.31.1
> >
> >
> 
> -- 
> Regards,
> Li Wang
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Mailing list info: https://lists.linux.it/listinfo/ltp