Re: [LTP] [PATCH] mem: make use of save_restore to simplify code

[Li Wang <liwang@redhat.com>]

On Wed, Jun 21, 2023 at 4:35 PM Petr Vorel <pvorel@suse.cz> wrote:

> Hi Li,
>
> very nice cleanup, thanks!
>
> ...
> > +++ b/testcases/kernel/mem/tunable/overcommit_memory.c
> > @@ -70,7 +70,6 @@
> >  #define EXPECT_FAIL          1
>
> >  static char *R_opt;
> > -static long old_overcommit_memory = -1;
> >  static long old_overcommit_ratio = -1;
> >  static long overcommit_ratio;
> >  static long sum_total;
> > @@ -90,16 +89,11 @@ static void setup(void)
> >       long mem_total, swap_total;
> >       struct rlimit lim;
>
> > -     if (access(PATH_SYSVM "overcommit_memory", F_OK) == -1 ||
> > -         access(PATH_SYSVM "overcommit_ratio", F_OK) == -1)
> > -             tst_brk(TCONF, "system doesn't support
> overcommit_memory");



> Here is used TCONF instead of TBROK.



> > -
> >       if (R_opt)
> >               overcommit_ratio = SAFE_STRTOL(R_opt, 0, LONG_MAX);
> >       else
> >               overcommit_ratio = DEFAULT_OVER_RATIO;
>
> > -     old_overcommit_memory = get_sys_tune("overcommit_memory");
> >       old_overcommit_ratio = get_sys_tune("overcommit_ratio");
>
> >       mem_total = SAFE_READ_MEMINFO("MemTotal:");
> > @@ -128,14 +122,6 @@ static void setup(void)
> >       tst_res(TINFO, "TotalBatchSize is %ld kB", total_batch_size);
> >  }
>
> > -static void cleanup(void)
> > -{
> > -     if (old_overcommit_memory != -1)
> > -             set_sys_tune("overcommit_memory", old_overcommit_memory,
> 0);
>


> Also third parameter of set_sys_tune() (check) is 0.
>

The checks inside set_sys_tuen() can NOT guarantee the
"overcommit_memory" knob is exist or not, it only examines if the
value was being set correctly, because set_sys_tune has first use
SAFE_FILE_PRINTF which will TBROK directly when the knob non-exist.


>
> > -     if (old_overcommit_ratio != -1)
> > -             set_sys_tune("overcommit_ratio", old_overcommit_ratio, 0);
> > -}
> > -
> >  static void overcommit_memory_test(void)
> >  {
>
> > @@ -269,6 +255,10 @@ static struct tst_test test = {
> >               {}
> >       },
> >       .setup = setup,
> > -     .cleanup = cleanup,
> >       .test_all = overcommit_memory_test,
> > +     .save_restore = (const struct tst_path_val[]) {
> > +             {"/proc/sys/vm/overcommit_memory", NULL, TST_SR_TBROK},
> > +             {"/proc/sys/vm/overcommit_ratio", NULL, TST_SR_TBROK},
> => shouldn't be here TST_SR_TCONF instead of TST_SR_TBROK?


It doesn't matter, I indeed consider this before, but after looking
through the rest mm tests they all use the function get|set_sys_tune()
which checks the knob mandatorily and run smoothly for past
many years and nobody ever complains about that.

So I think it's safe to convert this one using TBROK too, it essentially
has no difference from other oom-tests. 'overcommit_ratio' and
'overcommit_memory' are quite basic on Linux distribution.



>
> I also wonder if testcases/kernel/mem/tunable/max_map_count.c
> can replace old_max_map_count with .save_restore (with TST_SR_TCONF).
>

+1

>
> Also testcases/kernel/mem/tunable/min_free_kbytes.c could use
> .save_restore on panic_on_oom and min_free_kbytes, right?
>

No, 'panic_on_oom' is a different scenario, min_free_kbytes.c test
just checks if that was being set to 1, if yes, we have to skip the whole
test unconditionally in case of the system occurs panic.



>
> But these two can be done as a separate effort.
>

Yes, the rest two suggestions sound good.

>
> Otherwise LGTM.
> Reviewed-by: Petr Vorel <pvorel@suse.cz>
>

Thanks!

-- 
Regards,
Li Wang

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