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

[Petr Vorel <pvorel@suse.cz>]

Hi Li,

...
> > 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.

Ah, thanks for correcting me.


> > > -     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.

+1

> 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.

+1
=> go ahead and merge.


> > 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.

+1


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


> Yes, the rest two suggestions sound good.

I see you already post a patch, thx!

Kind regards,
Petr

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