| Message ID | 20230601105127.55155-1-liwang@redhat.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | mem: make use of save_restore to simplify code | expand |
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. > - 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? 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). Also testcases/kernel/mem/tunable/min_free_kbytes.c could use .save_restore on panic_on_oom and min_free_kbytes, right? But these two can be done as a separate effort. Otherwise LGTM. Reviewed-by: Petr Vorel <pvorel@suse.cz> Kind regards, Petr
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!
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
On Thu, Jun 22, 2023 at 6:06 PM Petr Vorel <pvorel@suse.cz> wrote: > 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. > Both pushed, thank you!
diff --git a/testcases/kernel/mem/include/mem.h b/testcases/kernel/mem/include/mem.h index 776809113..cdc3ca146 100644 --- a/testcases/kernel/mem/include/mem.h +++ b/testcases/kernel/mem/include/mem.h @@ -42,7 +42,6 @@ static inline void clean_node(unsigned long *array) #define MLOCK 2 #define KSM 3 -extern long overcommit; void oom(int testcase, int lite, int retcode, int allow_sigkill); void testoom(int mempolicy, int lite, int retcode, int allow_sigkill); diff --git a/testcases/kernel/mem/lib/mem.c b/testcases/kernel/mem/lib/mem.c index 8ddd7adf7..fbfeef026 100644 --- a/testcases/kernel/mem/lib/mem.c +++ b/testcases/kernel/mem/lib/mem.c @@ -27,8 +27,6 @@ /* OOM */ -long overcommit = -1; - static int alloc_mem(long int length, int testcase) { char *s; diff --git a/testcases/kernel/mem/oom/oom01.c b/testcases/kernel/mem/oom/oom01.c index 9f7d76587..b13699898 100644 --- a/testcases/kernel/mem/oom/oom01.c +++ b/testcases/kernel/mem/oom/oom01.c @@ -49,22 +49,13 @@ static void verify_oom(void) testoom(0, 0, ENOMEM, 1); } -static void setup(void) -{ - overcommit = get_sys_tune("overcommit_memory"); -} - -static void cleanup(void) -{ - if (overcommit != -1) - set_sys_tune("overcommit_memory", overcommit, 0); -} - static struct tst_test test = { .needs_root = 1, .forks_child = 1, .max_runtime = TST_UNLIMITED_RUNTIME, - .setup = setup, - .cleanup = cleanup, .test_all = verify_oom, + .save_restore = (const struct tst_path_val[]) { + {"/proc/sys/vm/overcommit_memory", NULL, TST_SR_TBROK}, + {} + }, }; diff --git a/testcases/kernel/mem/oom/oom02.c b/testcases/kernel/mem/oom/oom02.c index b3719f723..8d565d12a 100644 --- a/testcases/kernel/mem/oom/oom02.c +++ b/testcases/kernel/mem/oom/oom02.c @@ -56,15 +56,6 @@ static void setup(void) { if (!is_numa(NULL, NH_MEMS, 2)) tst_brk(TCONF, "The case need a NUMA system."); - - overcommit = get_sys_tune("overcommit_memory"); - set_sys_tune("overcommit_memory", 1, 1); -} - -static void cleanup(void) -{ - if (overcommit != -1) - set_sys_tune("overcommit_memory", overcommit, 0); } static struct tst_test test = { @@ -72,8 +63,11 @@ static struct tst_test test = { .forks_child = 1, .max_runtime = TST_UNLIMITED_RUNTIME, .setup = setup, - .cleanup = cleanup, .test_all = verify_oom, + .save_restore = (const struct tst_path_val[]) { + {"/proc/sys/vm/overcommit_memory", "1", TST_SR_TBROK}, + {} + }, }; #else diff --git a/testcases/kernel/mem/oom/oom03.c b/testcases/kernel/mem/oom/oom03.c index 0882c9bbe..778055d03 100644 --- a/testcases/kernel/mem/oom/oom03.c +++ b/testcases/kernel/mem/oom/oom03.c @@ -79,27 +79,21 @@ static void verify_oom(void) static void setup(void) { - overcommit = get_sys_tune("overcommit_memory"); - set_sys_tune("overcommit_memory", 1, 1); - SAFE_CG_PRINTF(tst_cg, "cgroup.procs", "%d", getpid()); SAFE_CG_PRINTF(tst_cg, "memory.max", "%lu", TESTMEM); } -static void cleanup(void) -{ - if (overcommit != -1) - set_sys_tune("overcommit_memory", overcommit, 0); -} - static struct tst_test test = { .needs_root = 1, .forks_child = 1, .max_runtime = TST_UNLIMITED_RUNTIME, .setup = setup, - .cleanup = cleanup, .test_all = verify_oom, .needs_cgroup_ctrls = (const char *const []){ "memory", NULL }, + .save_restore = (const struct tst_path_val[]) { + {"/proc/sys/vm/overcommit_memory", "1", TST_SR_TBROK}, + {} + }, }; #else diff --git a/testcases/kernel/mem/oom/oom04.c b/testcases/kernel/mem/oom/oom04.c index ad39f7e77..d27d9d9e7 100644 --- a/testcases/kernel/mem/oom/oom04.c +++ b/testcases/kernel/mem/oom/oom04.c @@ -65,9 +65,6 @@ static void setup(void) if (!is_numa(NULL, NH_MEMS, 1)) tst_brk(TCONF, "requires NUMA with at least 1 node"); - overcommit = get_sys_tune("overcommit_memory"); - set_sys_tune("overcommit_memory", 1, 1); - /* * Some nodes do not contain memory, so use * get_allowed_nodes(NH_MEMS) to get a memory @@ -82,20 +79,17 @@ static void setup(void) SAFE_CG_PRINTF(tst_cg, "cgroup.procs", "%d", getpid()); } -static void cleanup(void) -{ - if (overcommit != -1) - set_sys_tune("overcommit_memory", overcommit, 0); -} - static struct tst_test test = { .needs_root = 1, .forks_child = 1, .max_runtime = TST_UNLIMITED_RUNTIME, .setup = setup, - .cleanup = cleanup, .test_all = verify_oom, .needs_cgroup_ctrls = (const char *const []){ "cpuset", NULL }, + .save_restore = (const struct tst_path_val[]) { + {"/proc/sys/vm/overcommit_memory", "1", TST_SR_TBROK}, + {} + }, }; #else diff --git a/testcases/kernel/mem/oom/oom05.c b/testcases/kernel/mem/oom/oom05.c index e31146e7a..eb1a64267 100644 --- a/testcases/kernel/mem/oom/oom05.c +++ b/testcases/kernel/mem/oom/oom05.c @@ -85,9 +85,6 @@ void setup(void) if (!is_numa(NULL, NH_MEMS, 1)) tst_brk(TCONF, "requires NUMA with at least 1 node"); - overcommit = get_sys_tune("overcommit_memory"); - set_sys_tune("overcommit_memory", 1, 1); - /* * Some nodes do not contain memory, so use * get_allowed_nodes(NH_MEMS) to get a memory @@ -104,22 +101,19 @@ void setup(void) SAFE_CG_PRINTF(tst_cg, "memory.max", "%lu", TESTMEM); } -void cleanup(void) -{ - if (overcommit != -1) - set_sys_tune("overcommit_memory", overcommit, 0); -} - static struct tst_test test = { .needs_root = 1, .forks_child = 1, .max_runtime = TST_UNLIMITED_RUNTIME, .setup = setup, - .cleanup = cleanup, .test_all = verify_oom, .needs_cgroup_ctrls = (const char *const []){ "memory", "cpuset", NULL }, + .save_restore = (const struct tst_path_val[]) { + {"/proc/sys/vm/overcommit_memory", "1", TST_SR_TBROK}, + {} + }, }; #else diff --git a/testcases/kernel/mem/tunable/max_map_count.c b/testcases/kernel/mem/tunable/max_map_count.c index caf8972e0..82a69a61e 100644 --- a/testcases/kernel/mem/tunable/max_map_count.c +++ b/testcases/kernel/mem/tunable/max_map_count.c @@ -54,21 +54,16 @@ #define MAX_MAP_COUNT 65536L static long old_max_map_count = -1; -static long old_overcommit = -1; static void setup(void) { SAFE_ACCESS(PATH_SYSVM "max_map_count", F_OK); old_max_map_count = get_sys_tune("max_map_count"); - old_overcommit = get_sys_tune("overcommit_memory"); - set_sys_tune("overcommit_memory", 0, 1); } static void cleanup(void) { - if (old_overcommit != -1) - set_sys_tune("overcommit_memory", old_overcommit, 0); if (old_max_map_count != -1) set_sys_tune("max_map_count", old_max_map_count, 0); } @@ -213,4 +208,8 @@ static struct tst_test test = { .setup = setup, .cleanup = cleanup, .test_all = max_map_count_test, + .save_restore = (const struct tst_path_val[]) { + {"/proc/sys/vm/overcommit_memory", "0", TST_SR_TBROK}, + {} + }, }; diff --git a/testcases/kernel/mem/tunable/min_free_kbytes.c b/testcases/kernel/mem/tunable/min_free_kbytes.c index eab6c8b2e..dd07a0323 100644 --- a/testcases/kernel/mem/tunable/min_free_kbytes.c +++ b/testcases/kernel/mem/tunable/min_free_kbytes.c @@ -41,7 +41,6 @@ volatile int end; static long default_tune = -1; -static long orig_overcommit = -1; static unsigned long total_mem; static void test_tune(unsigned long overcommit_policy); @@ -217,15 +216,12 @@ static void setup(void) total_mem = SAFE_READ_MEMINFO("MemTotal:") + SAFE_READ_MEMINFO("SwapTotal:"); default_tune = get_sys_tune("min_free_kbytes"); - orig_overcommit = get_sys_tune("overcommit_memory"); } static void cleanup(void) { if (default_tune != -1) set_sys_tune("min_free_kbytes", default_tune, 0); - if (orig_overcommit != -1) - set_sys_tune("overcommit_memory", orig_overcommit, 0); } static struct tst_test test = { @@ -235,4 +231,8 @@ static struct tst_test test = { .setup = setup, .cleanup = cleanup, .test_all = min_free_kbytes_test, + .save_restore = (const struct tst_path_val[]) { + {"/proc/sys/vm/overcommit_memory", NULL, TST_SR_TBROK}, + {} + }, }; diff --git a/testcases/kernel/mem/tunable/overcommit_memory.c b/testcases/kernel/mem/tunable/overcommit_memory.c index 7fe8fe14c..ffb7a6d9d 100644 --- a/testcases/kernel/mem/tunable/overcommit_memory.c +++ 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"); - 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); - 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}, + {} + }, };
Signed-off-by: Li Wang <liwang@redhat.com> --- testcases/kernel/mem/include/mem.h | 1 - testcases/kernel/mem/lib/mem.c | 2 -- testcases/kernel/mem/oom/oom01.c | 17 ++++------------ testcases/kernel/mem/oom/oom02.c | 14 ++++--------- testcases/kernel/mem/oom/oom03.c | 14 ++++--------- testcases/kernel/mem/oom/oom04.c | 14 ++++--------- testcases/kernel/mem/oom/oom05.c | 14 ++++--------- testcases/kernel/mem/tunable/max_map_count.c | 9 ++++----- .../kernel/mem/tunable/min_free_kbytes.c | 8 ++++---- .../kernel/mem/tunable/overcommit_memory.c | 20 +++++-------------- 10 files changed, 33 insertions(+), 80 deletions(-)