[01/13] Hugetlb: Migrating libhugetlbfs mlock

Migrating the libhugetlbfs/testcases/mlock.c test

Test Description: The test checks that mlocking hugetlb areas works
with all combinations of MAP_PRIVATE and MAP_SHARED with and without
MAP_LOCKED specified.

Signed-off-by: Tarun Sahu <tsahu@linux.ibm.com>
---
 runtest/hugetlb                               |  1 +
 testcases/kernel/mem/.gitignore               |  1 +
 .../kernel/mem/hugetlb/hugemmap/hugemmap20.c  | 88 +++++++++++++++++++
 3 files changed, 90 insertions(+)
 create mode 100644 testcases/kernel/mem/hugetlb/hugemmap/hugemmap20.c

On Sun, Dec 25, 2022 at 11:42 PM Tarun Sahu <tsahu@linux.ibm.com> wrote:

> Migrating the libhugetlbfs/testcases/mlock.c test
>
> Test Description: The test checks that mlocking hugetlb areas works
> with all combinations of MAP_PRIVATE and MAP_SHARED with and without
> MAP_LOCKED specified.
>
> Signed-off-by: Tarun Sahu <tsahu@linux.ibm.com>
> ---
>  runtest/hugetlb                               |  1 +
>  testcases/kernel/mem/.gitignore               |  1 +
>  .../kernel/mem/hugetlb/hugemmap/hugemmap20.c  | 88 +++++++++++++++++++
>  3 files changed, 90 insertions(+)
>  create mode 100644 testcases/kernel/mem/hugetlb/hugemmap/hugemmap20.c
>
> diff --git a/runtest/hugetlb b/runtest/hugetlb
> index 4da1525a7..2dffa8421 100644
> --- a/runtest/hugetlb
> +++ b/runtest/hugetlb
> @@ -21,6 +21,7 @@ hugemmap16 hugemmap16
>  hugemmap17 hugemmap17
>  hugemmap18 hugemmap18
>  hugemmap19 hugemmap19
> +hugemmap20 hugemmap20
>  hugemmap05_1 hugemmap05 -m
>  hugemmap05_2 hugemmap05 -s
>  hugemmap05_3 hugemmap05 -s -m
> diff --git a/testcases/kernel/mem/.gitignore
> b/testcases/kernel/mem/.gitignore
> index b6b3e5ddd..dfd372892 100644
> --- a/testcases/kernel/mem/.gitignore
> +++ b/testcases/kernel/mem/.gitignore
> @@ -20,6 +20,7 @@
>  /hugetlb/hugemmap/hugemmap17
>  /hugetlb/hugemmap/hugemmap18
>  /hugetlb/hugemmap/hugemmap19
> +/hugetlb/hugemmap/hugemmap20
>  /hugetlb/hugeshmat/hugeshmat01
>  /hugetlb/hugeshmat/hugeshmat02
>  /hugetlb/hugeshmat/hugeshmat03
> diff --git a/testcases/kernel/mem/hugetlb/hugemmap/hugemmap20.c
> b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap20.c
> new file mode 100644
> index 000000000..9607d58b7
> --- /dev/null
> +++ b/testcases/kernel/mem/hugetlb/hugemmap/hugemmap20.c
> @@ -0,0 +1,88 @@
> +// SPDX-License-Identifier: LGPL-2.1-or-later
> +/*
> + * Copyright (C) 2005-2006 David Gibson & Adam Litke, IBM Corporation.
> + * Author: David Gibson & Adam Litke
> + */
> +
> +/*\
> + * [Description]
> + *
> + * The test checks that mlocking hugetlb areas works with all combinations
> + * of MAP_PRIVATE and MAP_SHARED with and without MAP_LOCKED specified.
> + */
> +
> +#include <stdio.h>
> +#include <sys/mount.h>
> +#include <limits.h>
> +#include <sys/param.h>
> +#include <sys/types.h>
> +#include <sys/resource.h>
> +
> +#include "hugetlb.h"
> +
> +#define MNTPOINT "hugetlbfs/"
> +static int  fd = -1;
> +static unsigned long hpage_size;
> +
> +static void test_simple_mlock(int flags, char *flags_str)
> +{
> +       void *p;
> +       int ret;
> +
> +       fd = tst_creat_unlinked(MNTPOINT, 0);
> +       p = SAFE_MMAP(0, hpage_size, PROT_READ|PROT_WRITE, flags, fd, 0);
> +
> +       ret = mlock(p, hpage_size);
> +       if (ret) {
> +               tst_res(TFAIL|TERRNO, "mlock() failed (flags %s)",
> flags_str);
> +               goto cleanup;
> +       }
> +
> +       ret = munlock(p, hpage_size);
> +       if (ret)
> +               tst_res(TFAIL|TERRNO, "munlock() failed (flags %s)",
> flags_str);
> +       else
> +               tst_res(TPASS, "mlock/munlock with %s hugetlb mmap",
> +                               flags_str);
> +cleanup:
> +       SAFE_MUNMAP(p, hpage_size);
> +       SAFE_CLOSE(fd);
> +}
> +
> +static void run_test(void)
> +{
>



> +       struct rlimit limit_info;
> +
> +       if (getrlimit(RLIMIT_MEMLOCK, &limit_info))
> +               tst_res(TWARN|TERRNO, "Unable to read locked memory
> rlimit");
> +       if (limit_info.rlim_cur < hpage_size)
> +               tst_brk(TCONF, "Locked memory ulimit set below huge page
> size");
>

These lines are better for moving into setup() phase. And, I'd propose
printing the value of 'limit_info.rlim_cur' and 'hpage_size' when TCONF.

The default value of max-locked-memory is smaller than hpage_size on
both RHEL8 and 9, which means this test will TCONF and skip running.
I'm hesitating should we temporally cancel the limitations and make
this test can really perform then restore that value to the original,
is this change make sense? WDYT?



> +
> +       test_simple_mlock(MAP_PRIVATE, "MAP_PRIVATE");
> +       test_simple_mlock(MAP_SHARED, "MAP_SHARED");
> +       test_simple_mlock(MAP_PRIVATE|MAP_LOCKED,
> "MAP_PRIVATE|MAP_LOCKED");
> +       test_simple_mlock(MAP_SHARED|MAP_LOCKED, "MAP_SHARED|MAP_LOCKED");
>

If we define an additional function like flags_to_str(int flags) for
converting
the flag into a string, which will be more simple for reading.

static char *flags_to_str(int flags)
{
       ...
}

static void test_simple_mlock(int flags)
{
        char *flags_str = flags_to_str(flags);
        ...
}



> +
> +}
> +
> +static void setup(void)
> +{
> +       hpage_size = SAFE_READ_MEMINFO(MEMINFO_HPAGE_SIZE)*1024;
> +}
> +
> +static void cleanup(void)
> +{
> +       if (fd >= 0)
> +               SAFE_CLOSE(fd);
> +}
> +
> +static struct tst_test test = {
> +       .needs_root = 1,
> +       .mntpoint = MNTPOINT,
> +       .needs_hugetlbfs = 1,
> +       .needs_tmpdir = 1,
> +       .setup = setup,
> +       .cleanup = cleanup,
> +       .test_all = run_test,
> +       .hugepages = {1, TST_NEEDS},
> +};
> --
> 2.31.1
>
>

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

--skip
On Dec 26 2022, Li Wang wrote:
> On Sun, Dec 25, 2022 at 11:42 PM Tarun Sahu <tsahu@linux.ibm.com> wrote:
> > +static void run_test(void)
> > +{
> >
> 
> 
> 
> > +       struct rlimit limit_info;
> > +
> > +       if (getrlimit(RLIMIT_MEMLOCK, &limit_info))
> > +               tst_res(TWARN|TERRNO, "Unable to read locked memory
> > rlimit");
> > +       if (limit_info.rlim_cur < hpage_size)
> > +               tst_brk(TCONF, "Locked memory ulimit set below huge page
> > size");
> >
> 
> These lines are better for moving into setup() phase. And, I'd propose
> printing the value of 'limit_info.rlim_cur' and 'hpage_size' when TCONF.
> 
> The default value of max-locked-memory is smaller than hpage_size on
> both RHEL8 and 9, which means this test will TCONF and skip running.
> I'm hesitating should we temporally cancel the limitations and make
> this test can really perform then restore that value to the original,
> is this change make sense? WDYT?
> 
Yeah, Incase if limit is smaller than expected, we can change it temporarily
to run the test. Will update in next revision. Also will change,
getrlimit/setrlimit function to SAFE_*.


> 
> 
> > +
> > +       test_simple_mlock(MAP_PRIVATE, "MAP_PRIVATE");
> > +       test_simple_mlock(MAP_SHARED, "MAP_SHARED");
> > +       test_simple_mlock(MAP_PRIVATE|MAP_LOCKED,
> > "MAP_PRIVATE|MAP_LOCKED");
> > +       test_simple_mlock(MAP_SHARED|MAP_LOCKED, "MAP_SHARED|MAP_LOCKED");
> >
> 
> If we define an additional function like flags_to_str(int flags) for
> converting
> the flag into a string, which will be more simple for reading.
> 
> static char *flags_to_str(int flags)
> {
>        ...
> }
> 
> static void test_simple_mlock(int flags)
> {
>         char *flags_str = flags_to_str(flags);
>         ...
> }
> 
It was not used so often and only some specific flags are used. I think
if there will be more general use case being some more mmap flags used in
application, then it would be good to change this to function.

Another way is to define a macro like

#define FLAGS_STR(flag) #flag

and pass it like: test_simple_mlock(flag, FLAGS_STR(flag));

But I thought, it is like passing the string itself.

what do you think?

> 
> 
> > +
> > +}
> > +
> > +static void setup(void)
> > +{
> > +       hpage_size = SAFE_READ_MEMINFO(MEMINFO_HPAGE_SIZE)*1024;
> > +}
> > +
> > +static void cleanup(void)
> > +{
> > +       if (fd >= 0)
> > +               SAFE_CLOSE(fd);
> > +}
> > +
> > +static struct tst_test test = {
> > +       .needs_root = 1,
> > +       .mntpoint = MNTPOINT,
> > +       .needs_hugetlbfs = 1,
> > +       .needs_tmpdir = 1,
> > +       .setup = setup,
> > +       .cleanup = cleanup,
> > +       .test_all = run_test,
> > +       .hugepages = {1, TST_NEEDS},
> > +};
> > --
> > 2.31.1
> >
> >
> 
> -- 
> Regards,
> Li Wang
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

On Mon, Dec 26, 2022 at 5:39 PM Tarun Sahu <tsahu@linux.ibm.com> wrote:

> Hi Li,
> Thanks for reviewing the patch.
> Please find my comments inline.
>
> --skip
> On Dec 26 2022, Li Wang wrote:
> > On Sun, Dec 25, 2022 at 11:42 PM Tarun Sahu <tsahu@linux.ibm.com> wrote:
> > > +static void run_test(void)
> > > +{
> > >
> >
> >
> >
> > > +       struct rlimit limit_info;
> > > +
> > > +       if (getrlimit(RLIMIT_MEMLOCK, &limit_info))
> > > +               tst_res(TWARN|TERRNO, "Unable to read locked memory
> > > rlimit");
> > > +       if (limit_info.rlim_cur < hpage_size)
> > > +               tst_brk(TCONF, "Locked memory ulimit set below huge
> page
> > > size");
> > >
> >
> > These lines are better for moving into setup() phase. And, I'd propose
> > printing the value of 'limit_info.rlim_cur' and 'hpage_size' when TCONF.
> >
> > The default value of max-locked-memory is smaller than hpage_size on
> > both RHEL8 and 9, which means this test will TCONF and skip running.
> > I'm hesitating should we temporally cancel the limitations and make
> > this test can really perform then restore that value to the original,
> > is this change make sense? WDYT?
> >
> Yeah, Incase if limit is smaller than expected, we can change it
> temporarily
> to run the test. Will update in next revision. Also will change,
> getrlimit/setrlimit function to SAFE_*.
>
>
> >
> >
> > > +
> > > +       test_simple_mlock(MAP_PRIVATE, "MAP_PRIVATE");
> > > +       test_simple_mlock(MAP_SHARED, "MAP_SHARED");
> > > +       test_simple_mlock(MAP_PRIVATE|MAP_LOCKED,
> > > "MAP_PRIVATE|MAP_LOCKED");
> > > +       test_simple_mlock(MAP_SHARED|MAP_LOCKED,
> "MAP_SHARED|MAP_LOCKED");
> > >
> >
> > If we define an additional function like flags_to_str(int flags) for
> > converting
> > the flag into a string, which will be more simple for reading.
> >
> > static char *flags_to_str(int flags)
> > {
> >        ...
> > }
> >
> > static void test_simple_mlock(int flags)
> > {
> >         char *flags_str = flags_to_str(flags);
> >         ...
> > }
> >
> It was not used so often and only some specific flags are used. I think
> if there will be more general use case being some more mmap flags used in
> application, then it would be good to change this to function.
>
> Another way is to define a macro like
>
> #define FLAGS_STR(flag) #flag
>
> and pass it like: test_simple_mlock(flag, FLAGS_STR(flag));
>
> But I thought, it is like passing the string itself.
>
> what do you think?
>

Yes sure, and I think of we have TST_TO_STR() in LTP as well.

$ git grep TST_TO_STR
tst_common.h:#define TST_TO_STR_(s) #s
tst_common.h:#define TST_TO_STR(s) TST_TO_STR_(s)