ltp.lists.linux.it archive mirror
 help / color / mirror / Atom feed
* [LTP] [PATCH] mem: make use of save_restore to simplify code
@ 2023-06-01 10:51 Li Wang
  2023-06-21  8:35 ` Petr Vorel
  0 siblings, 1 reply; 5+ messages in thread
From: Li Wang @ 2023-06-01 10:51 UTC (permalink / raw)
  To: ltp

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(-)

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},
+		{}
+	},
 };
-- 
2.40.1


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

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [LTP] [PATCH] mem: make use of save_restore to simplify code
  2023-06-01 10:51 [LTP] [PATCH] mem: make use of save_restore to simplify code Li Wang
@ 2023-06-21  8:35 ` Petr Vorel
  2023-06-22  8:53   ` Li Wang
  0 siblings, 1 reply; 5+ messages in thread
From: Petr Vorel @ 2023-06-21  8:35 UTC (permalink / raw)
  To: Li Wang; +Cc: ltp

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

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [LTP] [PATCH] mem: make use of save_restore to simplify code
  2023-06-21  8:35 ` Petr Vorel
@ 2023-06-22  8:53   ` Li Wang
  2023-06-22 10:05     ` Petr Vorel
  0 siblings, 1 reply; 5+ messages in thread
From: Li Wang @ 2023-06-22  8:53 UTC (permalink / raw)
  To: Petr Vorel; +Cc: ltp

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [LTP] [PATCH] mem: make use of save_restore to simplify code
  2023-06-22  8:53   ` Li Wang
@ 2023-06-22 10:05     ` Petr Vorel
  2023-06-23  1:34       ` Li Wang
  0 siblings, 1 reply; 5+ messages in thread
From: Petr Vorel @ 2023-06-22 10:05 UTC (permalink / raw)
  To: Li Wang; +Cc: ltp

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [LTP] [PATCH] mem: make use of save_restore to simplify code
  2023-06-22 10:05     ` Petr Vorel
@ 2023-06-23  1:34       ` Li Wang
  0 siblings, 0 replies; 5+ messages in thread
From: Li Wang @ 2023-06-23  1:34 UTC (permalink / raw)
  To: Petr Vorel; +Cc: ltp

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!


-- 
Regards,
Li Wang

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

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2023-06-23  1:34 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-06-01 10:51 [LTP] [PATCH] mem: make use of save_restore to simplify code Li Wang
2023-06-21  8:35 ` Petr Vorel
2023-06-22  8:53   ` Li Wang
2023-06-22 10:05     ` Petr Vorel
2023-06-23  1:34       ` Li Wang

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).