[LTP] [PATCH 06/13] Hugetlb: Migrating libhugetlbfs mremap-fixed-normal-near-huge

[Li Wang]

On Sat, Dec 31, 2022 at 1:08 PM Tarun Sahu <tsahu@linux.ibm.com> wrote:

> Hi,
>
> Li Wang <liwang@redhat.com> writes:
>
> > On Fri, Dec 30, 2022 at 3:06 AM Tarun Sahu <tsahu@linux.ibm.com> wrote:
> >
> >> Hi,
> >>
> >>
> >> Tarun Sahu <tsahu@linux.ibm.com> writes:
> >>
> >> > Hi Li,
> >> > Thanks for reviewing the patch.
> >> > I will update it in next revision.
> >> >
> >> > --skip
> >> >>> +static int do_readback(void *p, size_t size, const char *stage)
> >> >>> +{
> >> >>> +       unsigned int *q = p;
> >> >>> +       size_t i;
> >> >>> +
> >> >>> +       tst_res(TINFO, "%s(%p, 0x%lx, \"%s\")", __func__, p,
> >> >>> +              (unsigned long)size, stage);
> >> >>> +
> >> >>> +       for (i = 0; i < (size / sizeof(*q)); i++)
> >> >>> +               q[i] = RANDOM_CONSTANT ^ i;
> >> >>> +
> >> >>> +       for (i = 0; i < (size / sizeof(*q)); i++) {
> >> >>> +               if (q[i] != (RANDOM_CONSTANT ^ i)) {
> >> >>> +                       tst_res(TFAIL, "Stage \"%s\": Mismatch at
> >> offset
> >> >>> 0x%lx: 0x%x "
> >> >>> +                                       "instead of 0x%lx", stage,
> i,
> >> >>> q[i], RANDOM_CONSTANT ^ i);
> >> >>> +                       return -1;
> >> >>> +               }
> >> >>> +       }
> >> >>> +       return 0;
> >> >>> +}
> >> >>> +
> >> >>> +static int do_remap(void *target)
> >> >>> +{
> >> >>> +       void *a, *b;
> >> >>> +       int ret;
> >> >>> +
> >> >>> +       a = SAFE_MMAP(NULL, page_size, PROT_READ|PROT_WRITE,
> >> >>> +                 MAP_SHARED|MAP_ANONYMOUS, -1, 0);
> >> >>> +
> >> >>> +       ret = do_readback(a, page_size, "base normal");
> >> >>> +       if (ret)
> >> >>> +               goto cleanup;
> >> >>> +       b = mremap(a, page_size, page_size, MREMAP_MAYMOVE |
> >> MREMAP_FIXED,
> >> >>> +                  target);
> >> >>> +
> >> >>> +       if (b != MAP_FAILED) {
> >> >>> +               do_readback(b, page_size, "remapped");
> >> >>> +               a = b;
> >> >>> +       } else
> >> >>> +               tst_res(TINFO|TERRNO, "mremap(MAYMOVE|FIXED)
> >> disallowed");
> >> >>> +
> >> >>> +cleanup:
> >> >>> +       SAFE_MUNMAP(a, page_size);
> >> >>> +       return ret;
> >> >>> +}
> >> >>>
> >> >>
> >> >> Those two functions do_readback() and do_remap() are
> >> >> copy&past from hugemmap2[4|5].c, what about extracting
> >> >> them into a common header file(tst_hugetlb.h or mem.h) for
> >> >> easy reusing?
> >> >>
> >> > ok, I think, hugetlb.h/.c will be better place to keep them.
> >> >
> >> These two functions are very specific to tests (specially the do_remap).
> >> Also, they use values that are defined only inside the test.
> >> Though do_readback is more general which can be put in hugetlb.h/c.
> >>
> >> WDYT?
> >>
> >
> > Ah yes, I didn't realize the difference.
> > Or can we pass the specific arguments based on the variable part?
> > e.g.
> >
> >     static int do_remap(int fd, size_t page_size; int prot, void
> *target);
> >
> This can be done this way, but What concerned me is mremap function
> itself. it is defined in sys/mmap under _GNU_SOURCE. I am not sure
> whether it will be ok to use _GNU_SOURCE specific function in
> lib/hugetlb.c/h file.
>

Good catch.

>
> please confirm if it is ok to define _GNU_SOURCE in hugetlb.c/h.
>

Well, I guess we should avoid doing that because it will bring
more uncertain factors to other tests that include this header file.

You could make an informed trade-off about review comments
according to actual conditions.

-- 
Regards,
Li Wang