All of lore.kernel.org
 help / color / mirror / Atom feed
* [LTP] [PATCH 1/2] madvise06: Fix segfault
@ 2022-03-11 12:01 Petr Vorel
  2022-03-11 12:01 ` [LTP] [PATCH 2/2] madvise06: Move oom_score_adj initialization to struct tst_path_val Petr Vorel
  2022-03-11 12:28 ` [LTP] [PATCH 1/2] madvise06: Fix segfault Cyril Hrubis
  0 siblings, 2 replies; 6+ messages in thread
From: Petr Vorel @ 2022-03-11 12:01 UTC (permalink / raw)
  To: ltp; +Cc: Richard Palethorpe

Test required update after 687f0cbc00 to use struct tst_path_val.
While at it, move the initialization to struct tst_path_val.

Fixes: 687f0cbc00 ("lib: enhance .save_restore to support set expected value")

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
Is it ok to define it earlier? i.e. before calling sync() ?

 testcases/kernel/syscalls/madvise/madvise06.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/testcases/kernel/syscalls/madvise/madvise06.c b/testcases/kernel/syscalls/madvise/madvise06.c
index 54391db283..b21f2cc7de 100644
--- a/testcases/kernel/syscalls/madvise/madvise06.c
+++ b/testcases/kernel/syscalls/madvise/madvise06.c
@@ -54,6 +54,7 @@
 #define MEMSW_LIMIT (2 * CHUNK_SZ)
 #define PASS_THRESHOLD (CHUNK_SZ / 4)
 #define PASS_THRESHOLD_KB (PASS_THRESHOLD / 1024)
+#define SWAPPINESS "60"
 
 static const char drop_caches_fname[] = "/proc/sys/vm/drop_caches";
 static int pg_sz, stat_refresh_sup;
@@ -123,10 +124,9 @@ static void setup(void)
 		SAFE_CG_PRINTF(tst_cg, "memory.swap.max", "%ld", MEMSW_LIMIT);
 
 	if (SAFE_CG_HAS(tst_cg, "memory.swappiness")) {
-		SAFE_CG_PRINT(tst_cg, "memory.swappiness", "60");
+		SAFE_CG_PRINT(tst_cg, "memory.swappiness", SWAPPINESS);
 	} else {
 		check_path("/proc/sys/vm/swappiness");
-		SAFE_FILE_PRINTF("/proc/sys/vm/swappiness", "%d", 60);
 	}
 
 	SAFE_CG_PRINTF(tst_cg, "cgroup.procs", "%d", getpid());
@@ -229,8 +229,8 @@ static struct tst_test test = {
 	.min_kver = "3.10.0",
 	.needs_tmpdir = 1,
 	.needs_root = 1,
-	.save_restore = (const char * const[]) {
-		"?/proc/sys/vm/swappiness",
+	.save_restore = (const struct tst_path_val const[]) {
+		{"?/proc/sys/vm/swappiness", SWAPPINESS},
 		NULL
 	},
 	.needs_cgroup_ctrls = (const char *const []){ "memory", NULL },
-- 
2.35.1


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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [LTP] [PATCH 2/2] madvise06: Move oom_score_adj initialization to struct tst_path_val
  2022-03-11 12:01 [LTP] [PATCH 1/2] madvise06: Fix segfault Petr Vorel
@ 2022-03-11 12:01 ` Petr Vorel
  2022-03-11 12:37   ` Cyril Hrubis
  2022-03-11 12:28 ` [LTP] [PATCH 1/2] madvise06: Fix segfault Cyril Hrubis
  1 sibling, 1 reply; 6+ messages in thread
From: Petr Vorel @ 2022-03-11 12:01 UTC (permalink / raw)
  To: ltp; +Cc: Richard Palethorpe

Signed-off-by: Petr Vorel <pvorel@suse.cz>
---
Is it ok to define it earlier? i.e. before calling sync() ?

 testcases/kernel/syscalls/madvise/madvise06.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/testcases/kernel/syscalls/madvise/madvise06.c b/testcases/kernel/syscalls/madvise/madvise06.c
index b21f2cc7de..15171442cb 100644
--- a/testcases/kernel/syscalls/madvise/madvise06.c
+++ b/testcases/kernel/syscalls/madvise/madvise06.c
@@ -55,6 +55,7 @@
 #define PASS_THRESHOLD (CHUNK_SZ / 4)
 #define PASS_THRESHOLD_KB (PASS_THRESHOLD / 1024)
 #define SWAPPINESS "60"
+#define OOM_SCORE_ADJ "-1000"
 
 static const char drop_caches_fname[] = "/proc/sys/vm/drop_caches";
 static int pg_sz, stat_refresh_sup;
@@ -116,9 +117,6 @@ static void setup(void)
 			2 * CHUNK_SZ);
 	}
 
-	check_path("/proc/self/oom_score_adj");
-	SAFE_FILE_PRINTF("/proc/self/oom_score_adj", "%d", -1000);
-
 	SAFE_CG_PRINTF(tst_cg, "memory.max", "%ld", MEM_LIMIT);
 	if (SAFE_CG_HAS(tst_cg, "memory.swap.max"))
 		SAFE_CG_PRINTF(tst_cg, "memory.swap.max", "%ld", MEMSW_LIMIT);
@@ -231,6 +229,7 @@ static struct tst_test test = {
 	.needs_root = 1,
 	.save_restore = (const struct tst_path_val const[]) {
 		{"?/proc/sys/vm/swappiness", SWAPPINESS},
+		{"/proc/self/oom_score_adj", OOM_SCORE_ADJ},
 		NULL
 	},
 	.needs_cgroup_ctrls = (const char *const []){ "memory", NULL },
-- 
2.35.1


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

^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [LTP] [PATCH 1/2] madvise06: Fix segfault
  2022-03-11 12:01 [LTP] [PATCH 1/2] madvise06: Fix segfault Petr Vorel
  2022-03-11 12:01 ` [LTP] [PATCH 2/2] madvise06: Move oom_score_adj initialization to struct tst_path_val Petr Vorel
@ 2022-03-11 12:28 ` Cyril Hrubis
  2022-03-11 14:46   ` Petr Vorel
  1 sibling, 1 reply; 6+ messages in thread
From: Cyril Hrubis @ 2022-03-11 12:28 UTC (permalink / raw)
  To: Petr Vorel; +Cc: Richard Palethorpe, ltp

Hi!
> Test required update after 687f0cbc00 to use struct tst_path_val.
> While at it, move the initialization to struct tst_path_val.
> 
> Fixes: 687f0cbc00 ("lib: enhance .save_restore to support set expected value")
> 
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
> Is it ok to define it earlier? i.e. before calling sync() ?
> 
>  testcases/kernel/syscalls/madvise/madvise06.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/madvise/madvise06.c b/testcases/kernel/syscalls/madvise/madvise06.c
> index 54391db283..b21f2cc7de 100644
> --- a/testcases/kernel/syscalls/madvise/madvise06.c
> +++ b/testcases/kernel/syscalls/madvise/madvise06.c
> @@ -54,6 +54,7 @@
>  #define MEMSW_LIMIT (2 * CHUNK_SZ)
>  #define PASS_THRESHOLD (CHUNK_SZ / 4)
>  #define PASS_THRESHOLD_KB (PASS_THRESHOLD / 1024)
> +#define SWAPPINESS "60"
>  
>  static const char drop_caches_fname[] = "/proc/sys/vm/drop_caches";
>  static int pg_sz, stat_refresh_sup;
> @@ -123,10 +124,9 @@ static void setup(void)
>  		SAFE_CG_PRINTF(tst_cg, "memory.swap.max", "%ld", MEMSW_LIMIT);
>  
>  	if (SAFE_CG_HAS(tst_cg, "memory.swappiness")) {
> -		SAFE_CG_PRINT(tst_cg, "memory.swappiness", "60");
> +		SAFE_CG_PRINT(tst_cg, "memory.swappiness", SWAPPINESS);
>  	} else {
>  		check_path("/proc/sys/vm/swappiness");
> -		SAFE_FILE_PRINTF("/proc/sys/vm/swappiness", "%d", 60);
>  	}

I'm not sure if we want to set the "/proc/sys/vm/swappiness"
unconditinally in the .save_restore as previously we set it only if the
cgroup was missing the swappines knob.

So maybe we should go for a minimal fix here, just change the
save_restore to match the new format and don't set the value there.

>  	SAFE_CG_PRINTF(tst_cg, "cgroup.procs", "%d", getpid());
> @@ -229,8 +229,8 @@ static struct tst_test test = {
>  	.min_kver = "3.10.0",
>  	.needs_tmpdir = 1,
>  	.needs_root = 1,
> -	.save_restore = (const char * const[]) {
> -		"?/proc/sys/vm/swappiness",
> +	.save_restore = (const struct tst_path_val const[]) {
> +		{"?/proc/sys/vm/swappiness", SWAPPINESS},
>  		NULL

This has to be terminated by {} now.

>  	},
>  	.needs_cgroup_ctrls = (const char *const []){ "memory", NULL },
> -- 
> 2.35.1
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [LTP] [PATCH 2/2] madvise06: Move oom_score_adj initialization to struct tst_path_val
  2022-03-11 12:01 ` [LTP] [PATCH 2/2] madvise06: Move oom_score_adj initialization to struct tst_path_val Petr Vorel
@ 2022-03-11 12:37   ` Cyril Hrubis
  2022-03-14  3:37     ` Li Wang
  0 siblings, 1 reply; 6+ messages in thread
From: Cyril Hrubis @ 2022-03-11 12:37 UTC (permalink / raw)
  To: Petr Vorel; +Cc: Richard Palethorpe, ltp

Hi!
> Signed-off-by: Petr Vorel <pvorel@suse.cz>
> ---
> Is it ok to define it earlier? i.e. before calling sync() ?
> 
>  testcases/kernel/syscalls/madvise/madvise06.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/madvise/madvise06.c b/testcases/kernel/syscalls/madvise/madvise06.c
> index b21f2cc7de..15171442cb 100644
> --- a/testcases/kernel/syscalls/madvise/madvise06.c
> +++ b/testcases/kernel/syscalls/madvise/madvise06.c
> @@ -55,6 +55,7 @@
>  #define PASS_THRESHOLD (CHUNK_SZ / 4)
>  #define PASS_THRESHOLD_KB (PASS_THRESHOLD / 1024)
>  #define SWAPPINESS "60"
> +#define OOM_SCORE_ADJ "-1000"
>  
>  static const char drop_caches_fname[] = "/proc/sys/vm/drop_caches";
>  static int pg_sz, stat_refresh_sup;
> @@ -116,9 +117,6 @@ static void setup(void)
>  			2 * CHUNK_SZ);
>  	}
>  
> -	check_path("/proc/self/oom_score_adj");
> -	SAFE_FILE_PRINTF("/proc/self/oom_score_adj", "%d", -1000);
> -
>  	SAFE_CG_PRINTF(tst_cg, "memory.max", "%ld", MEM_LIMIT);
>  	if (SAFE_CG_HAS(tst_cg, "memory.swap.max"))
>  		SAFE_CG_PRINTF(tst_cg, "memory.swap.max", "%ld", MEMSW_LIMIT);
> @@ -231,6 +229,7 @@ static struct tst_test test = {
>  	.needs_root = 1,
>  	.save_restore = (const struct tst_path_val const[]) {
>  		{"?/proc/sys/vm/swappiness", SWAPPINESS},
> +		{"/proc/self/oom_score_adj", OOM_SCORE_ADJ},

I do not think that this is even correct, after this change the file is
being written to by the test library and the 'self' points to a
different process as far as I can tell.

-- 
Cyril Hrubis
chrubis@suse.cz

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [LTP] [PATCH 1/2] madvise06: Fix segfault
  2022-03-11 12:28 ` [LTP] [PATCH 1/2] madvise06: Fix segfault Cyril Hrubis
@ 2022-03-11 14:46   ` Petr Vorel
  0 siblings, 0 replies; 6+ messages in thread
From: Petr Vorel @ 2022-03-11 14:46 UTC (permalink / raw)
  To: Cyril Hrubis; +Cc: Richard Palethorpe, ltp

Hi Cyril,

...
> >  	if (SAFE_CG_HAS(tst_cg, "memory.swappiness")) {
> > -		SAFE_CG_PRINT(tst_cg, "memory.swappiness", "60");
> > +		SAFE_CG_PRINT(tst_cg, "memory.swappiness", SWAPPINESS);
> >  	} else {
> >  		check_path("/proc/sys/vm/swappiness");
> > -		SAFE_FILE_PRINTF("/proc/sys/vm/swappiness", "%d", 60);
> >  	}

> I'm not sure if we want to set the "/proc/sys/vm/swappiness"
> unconditinally in the .save_restore as previously we set it only if the
> cgroup was missing the swappines knob.

> So maybe we should go for a minimal fix here, just change the
> save_restore to match the new format and don't set the value there.
Make sense.

> >  	SAFE_CG_PRINTF(tst_cg, "cgroup.procs", "%d", getpid());
> > @@ -229,8 +229,8 @@ static struct tst_test test = {
> >  	.min_kver = "3.10.0",
> >  	.needs_tmpdir = 1,
> >  	.needs_root = 1,
> > -	.save_restore = (const char * const[]) {
> > -		"?/proc/sys/vm/swappiness",
> > +	.save_restore = (const struct tst_path_val const[]) {
> > +		{"?/proc/sys/vm/swappiness", SWAPPINESS},
> >  		NULL

> This has to be terminated by {} now.
Ah, thx!

Anyway, merged only minimal change with your Reviewed-by: tag.

Thanks!

Kind regards,
Petr

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

^ permalink raw reply	[flat|nested] 6+ messages in thread
* Re: [LTP] [PATCH 2/2] madvise06: Move oom_score_adj initialization to struct tst_path_val
  2022-03-11 12:37   ` Cyril Hrubis
@ 2022-03-14  3:37     ` Li Wang
  0 siblings, 0 replies; 6+ messages in thread
From: Li Wang @ 2022-03-14  3:37 UTC (permalink / raw)
  To: Cyril Hrubis; +Cc: Richard Palethorpe, ltp


[-- Attachment #1.1: Type: text/plain, Size: 2216 bytes --]

On Fri, Mar 11, 2022 at 8:35 PM Cyril Hrubis <chrubis@suse.cz> wrote:

> Hi!
> > Signed-off-by: Petr Vorel <pvorel@suse.cz>
> > ---
> > Is it ok to define it earlier? i.e. before calling sync() ?
> >
> >  testcases/kernel/syscalls/madvise/madvise06.c | 5 ++---
> >  1 file changed, 2 insertions(+), 3 deletions(-)
> >
> > diff --git a/testcases/kernel/syscalls/madvise/madvise06.c
> b/testcases/kernel/syscalls/madvise/madvise06.c
> > index b21f2cc7de..15171442cb 100644
> > --- a/testcases/kernel/syscalls/madvise/madvise06.c
> > +++ b/testcases/kernel/syscalls/madvise/madvise06.c
> > @@ -55,6 +55,7 @@
> >  #define PASS_THRESHOLD (CHUNK_SZ / 4)
> >  #define PASS_THRESHOLD_KB (PASS_THRESHOLD / 1024)
> >  #define SWAPPINESS "60"
> > +#define OOM_SCORE_ADJ "-1000"
> >
> >  static const char drop_caches_fname[] = "/proc/sys/vm/drop_caches";
> >  static int pg_sz, stat_refresh_sup;
> > @@ -116,9 +117,6 @@ static void setup(void)
> >                       2 * CHUNK_SZ);
> >       }
> >
> > -     check_path("/proc/self/oom_score_adj");
> > -     SAFE_FILE_PRINTF("/proc/self/oom_score_adj", "%d", -1000);
>

Maybe use tst_enable_oom_protection(0) here is better.


> -
> >       SAFE_CG_PRINTF(tst_cg, "memory.max", "%ld", MEM_LIMIT);
> >       if (SAFE_CG_HAS(tst_cg, "memory.swap.max"))
> >               SAFE_CG_PRINTF(tst_cg, "memory.swap.max", "%ld",
> MEMSW_LIMIT);
> > @@ -231,6 +229,7 @@ static struct tst_test test = {
> >       .needs_root = 1,
> >       .save_restore = (const struct tst_path_val const[]) {
> >               {"?/proc/sys/vm/swappiness", SWAPPINESS},
> > +             {"/proc/self/oom_score_adj", OOM_SCORE_ADJ},
>
> I do not think that this is even correct, after this change the file is
> being written to by the test library and the 'self' points to a
> different process as far as I can tell.
>

That's right. But from the code logic, seems it try to protect the
madvise06 from being killed by OOM. So enabling the oom
protection in the test library also has the same effect because
children(madvise06) will inherit the score.

However, I agree that we shouldn't add the oom_score_adj"
knob in .svae_restore since that might bring more confusion
to users.


-- 
Regards,
Li Wang

[-- Attachment #1.2: Type: text/html, Size: 4003 bytes --]

[-- Attachment #2: Type: text/plain, Size: 60 bytes --]


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

^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-03-14  3:37 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-11 12:01 [LTP] [PATCH 1/2] madvise06: Fix segfault Petr Vorel
2022-03-11 12:01 ` [LTP] [PATCH 2/2] madvise06: Move oom_score_adj initialization to struct tst_path_val Petr Vorel
2022-03-11 12:37   ` Cyril Hrubis
2022-03-14  3:37     ` Li Wang
2022-03-11 12:28 ` [LTP] [PATCH 1/2] madvise06: Fix segfault Cyril Hrubis
2022-03-11 14:46   ` Petr Vorel

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.