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

[Li Wang]

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)

-- 
Regards,
Li Wang