[LTP] [PATCH 01/13] Hugetlb: Migrating libhugetlbfs mlock

[Tarun Sahu]

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