[RFC,1/3] min_free_kbytes: Fix child exit status check conditions
diff mbox series

Message ID 20190319093858.584-1-liwang@redhat.com
State New
Headers show
Series
  • [RFC,1/3] min_free_kbytes: Fix child exit status check conditions
Related show

Commit Message

Li Wang March 19, 2019, 9:38 a.m. UTC
From: Vipin K Parashar <vipin@linux.vnet.ibm.com>

Fixes: #349

min_free_kbytes test has badly formed if conditions in mem_tune()
for child exit status check. This is causing test to declare as FAILED
despite that not being the case. Fix child exit status check conditions.

---ERROR LOG---
<..snip..>
mem.c:839: INFO: set overcommit_memory to 1
mem.c:839: INFO: set min_free_kbytes to 11580
memfree is 6974720 kB before eatup mem
memfree is 15488 kB after eatup mem
min_free_kbytes.c:135: FAIL: child unexpectedly failed: 0
mem.c:839: INFO: set min_free_kbytes to 23160
memfree is 7104128 kB before eatup mem
memfree is 26560 kB after eatup mem
min_free_kbytes.c:135: FAIL: child unexpectedly failed: 0
mem.c:839: INFO: set min_free_kbytes to 145812
memfree is 7101504 kB before eatup mem
memfree is 215872 kB after eatup mem
min_free_kbytes.c:135: FAIL: child unexpectedly failed: 0
min_free_kbytes.c:81: PASS: min_free_kbytes test pass
mem.c:839: INFO: set min_free_kbytes to 11580
---------------

Signed-off-by: Vipin K Parashar <vipin@linux.vnet.ibm.com>
Signed-off-by: Li Wang <liwang@redhat.com>
Cc: Jan Stancek <jstancek@redhat.com>
---
 .../kernel/mem/tunable/min_free_kbytes.c      | 40 +++++--------------
 1 file changed, 11 insertions(+), 29 deletions(-)

Comments

Jan Stancek March 19, 2019, 4:30 p.m. UTC | #1
----- Original Message -----
> From: Vipin K Parashar <vipin@linux.vnet.ibm.com>
> 
> Fixes: #349
> 
> min_free_kbytes test has badly formed if conditions in mem_tune()
> for child exit status check. 

Can you please elaborate what's wrong about it?

> This is causing test to declare as FAILED
> despite that not being the case.

Why? Shouldn't mmap succeed with overcommit == 1?

> Fix child exit status check conditions.
> 
> ---ERROR LOG---
> <..snip..>
> mem.c:839: INFO: set overcommit_memory to 1
> mem.c:839: INFO: set min_free_kbytes to 11580
> memfree is 6974720 kB before eatup mem
> memfree is 15488 kB after eatup mem
> min_free_kbytes.c:135: FAIL: child unexpectedly failed: 0
> mem.c:839: INFO: set min_free_kbytes to 23160
> memfree is 7104128 kB before eatup mem
> memfree is 26560 kB after eatup mem
> min_free_kbytes.c:135: FAIL: child unexpectedly failed: 0
> mem.c:839: INFO: set min_free_kbytes to 145812
> memfree is 7101504 kB before eatup mem
> memfree is 215872 kB after eatup mem
> min_free_kbytes.c:135: FAIL: child unexpectedly failed: 0
> min_free_kbytes.c:81: PASS: min_free_kbytes test pass
> mem.c:839: INFO: set min_free_kbytes to 11580
> ---------------
> 
> Signed-off-by: Vipin K Parashar <vipin@linux.vnet.ibm.com>
> Signed-off-by: Li Wang <liwang@redhat.com>
> Cc: Jan Stancek <jstancek@redhat.com>
> ---
>  .../kernel/mem/tunable/min_free_kbytes.c      | 40 +++++--------------
>  1 file changed, 11 insertions(+), 29 deletions(-)
> 
> diff --git a/testcases/kernel/mem/tunable/min_free_kbytes.c
> b/testcases/kernel/mem/tunable/min_free_kbytes.c
> index f114dc493..d2378a700 100644
> --- a/testcases/kernel/mem/tunable/min_free_kbytes.c
> +++ b/testcases/kernel/mem/tunable/min_free_kbytes.c
> @@ -119,39 +119,21 @@ static void test_tune(unsigned long overcommit_policy)
>  
>  		SAFE_WAITPID(pid[i], &status, WUNTRACED | WCONTINUED);
>  
> -		if (overcommit_policy == 2) {
> -			if (!WIFEXITED(status) || WEXITSTATUS(status) != 0)
> -				tst_res(TFAIL,
> -					 "child unexpectedly failed: %d",
> -					 status);
> -		} else if (overcommit_policy == 1) {
> -			if (!WIFSIGNALED(status) || WTERMSIG(status) != SIGKILL)
> +		if (WIFEXITED(status) && WEXITSTATUS(status) != 0) {
> +			tst_res(TFAIL,
> +				"child unexpectedly failed: %d", status);
> +		} else if (WIFSIGNALED(status) && WTERMSIG(status) != SIGKILL) {
>  #if __WORDSIZE == 32
> -			{
> -				if (total_mem < 3145728UL)
> +			if (total_mem < 3145728UL)
>  #endif
> -					tst_res(TFAIL,
> -						 "child unexpectedly failed: %d",
> -						 status);
> +				tst_res(TFAIL,
> +					"child unexpectedly failed: %d", status);
>  #if __WORDSIZE == 32
> -				/* in 32-bit system, a process allocate about 3Gb memory at most */
> -				else
> -					tst_res(TINFO, "Child can't allocate "
> -						 ">3Gb memory in 32bit system");
> -			}
> +			/* in 32-bit system, a process allocate about 3Gb memory at most */
> +			else
> +				tst_res(TINFO, "Child can't allocate "
> +					">3Gb memory in 32bit system");
>  #endif
> -		} else {
> -			if (WIFEXITED(status)) {
> -				if (WEXITSTATUS(status) != 0) {
> -					tst_res(TFAIL, "child unexpectedly "
> -						 "failed: %d", status);
> -				}
> -			} else if (!WIFSIGNALED(status) ||
> -				   WTERMSIG(status) != SIGKILL) {
> -				tst_res(TFAIL,
> -					 "child unexpectedly failed: %d",
> -					 status);
> -			}
>  		}
>  	}
>  }
> --
> 2.20.1
> 
>
Li Wang March 20, 2019, 6:11 a.m. UTC | #2
@Vipin, is there any possible you can have a look Jan's question?

On Wed, Mar 20, 2019 at 12:30 AM Jan Stancek <jstancek@redhat.com> wrote:

>
>
> ----- Original Message -----
> > From: Vipin K Parashar <vipin@linux.vnet.ibm.com>
> >
> > Fixes: #349
> >
> > min_free_kbytes test has badly formed if conditions in mem_tune()
> > for child exit status check.
>
> Can you please elaborate what's wrong about it?
>

I‘m not sure if Vipin could help to explain this. I just nocited that this
pull request was pending there more than half year, so I re-send this patch
to ML for more discussion.
https://github.com/linux-test-project/ltp/pull/350


> > This is causing test to declare as FAILED
> > despite that not being the case.
>
> Why? Shouldn't mmap succeed with overcommit == 1?
>

I was unable to reproduce this. But AFAIK, 'overcommit == 1' cann't
guarantee the MAP_FAILED(with ENOMEM) before oom. So I roughly made change
in the original patch to catch up both oom and map_failed scenario for any
'overcommit'.

Beside that, as I was pointed out in patch 3/3, this test has an obviously
defect in design, I'm also not sure if MemFree + (1/10 *min_free_kbytes) is
safty enough, but that's only what I can do for the case remedy.


> > Fix child exit status check conditions.
> >
> > ---ERROR LOG---
> > <..snip..>
> > mem.c:839: INFO: set overcommit_memory to 1
> > mem.c:839: INFO: set min_free_kbytes to 11580
> > memfree is 6974720 kB before eatup mem
> > memfree is 15488 kB after eatup mem
> > min_free_kbytes.c:135: FAIL: child unexpectedly failed: 0
> > mem.c:839: INFO: set min_free_kbytes to 23160
> > memfree is 7104128 kB before eatup mem
> > memfree is 26560 kB after eatup mem
> > min_free_kbytes.c:135: FAIL: child unexpectedly failed: 0
> > mem.c:839: INFO: set min_free_kbytes to 145812
> > memfree is 7101504 kB before eatup mem
> > memfree is 215872 kB after eatup mem
> > min_free_kbytes.c:135: FAIL: child unexpectedly failed: 0
> > min_free_kbytes.c:81: PASS: min_free_kbytes test pass
> > mem.c:839: INFO: set min_free_kbytes to 11580
> > ---------------
> >
> > Signed-off-by: Vipin K Parashar <vipin@linux.vnet.ibm.com>
> > Signed-off-by: Li Wang <liwang@redhat.com>
> > Cc: Jan Stancek <jstancek@redhat.com>
> > ---
> >  .../kernel/mem/tunable/min_free_kbytes.c      | 40 +++++--------------
> >  1 file changed, 11 insertions(+), 29 deletions(-)
> >
> > diff --git a/testcases/kernel/mem/tunable/min_free_kbytes.c
> > b/testcases/kernel/mem/tunable/min_free_kbytes.c
> > index f114dc493..d2378a700 100644
> > --- a/testcases/kernel/mem/tunable/min_free_kbytes.c
> > +++ b/testcases/kernel/mem/tunable/min_free_kbytes.c
> > @@ -119,39 +119,21 @@ static void test_tune(unsigned long
> overcommit_policy)
> >
> >               SAFE_WAITPID(pid[i], &status, WUNTRACED | WCONTINUED);
> >
> > -             if (overcommit_policy == 2) {
> > -                     if (!WIFEXITED(status) || WEXITSTATUS(status) != 0)
> > -                             tst_res(TFAIL,
> > -                                      "child unexpectedly failed: %d",
> > -                                      status);
> > -             } else if (overcommit_policy == 1) {
> > -                     if (!WIFSIGNALED(status) || WTERMSIG(status) !=
> SIGKILL)
> > +             if (WIFEXITED(status) && WEXITSTATUS(status) != 0) {
> > +                     tst_res(TFAIL,
> > +                             "child unexpectedly failed: %d", status);
> > +             } else if (WIFSIGNALED(status) && WTERMSIG(status) !=
> SIGKILL) {
> >  #if __WORDSIZE == 32
> > -                     {
> > -                             if (total_mem < 3145728UL)
> > +                     if (total_mem < 3145728UL)
> >  #endif
> > -                                     tst_res(TFAIL,
> > -                                              "child unexpectedly
> failed: %d",
> > -                                              status);
> > +                             tst_res(TFAIL,
> > +                                     "child unexpectedly failed: %d",
> status);
> >  #if __WORDSIZE == 32
> > -                             /* in 32-bit system, a process allocate
> about 3Gb memory at most */
> > -                             else
> > -                                     tst_res(TINFO, "Child can't
> allocate "
> > -                                              ">3Gb memory in 32bit
> system");
> > -                     }
> > +                     /* in 32-bit system, a process allocate about 3Gb
> memory at most */
> > +                     else
> > +                             tst_res(TINFO, "Child can't allocate "
> > +                                     ">3Gb memory in 32bit system");
> >  #endif
> > -             } else {
> > -                     if (WIFEXITED(status)) {
> > -                             if (WEXITSTATUS(status) != 0) {
> > -                                     tst_res(TFAIL, "child unexpectedly
> "
> > -                                              "failed: %d", status);
> > -                             }
> > -                     } else if (!WIFSIGNALED(status) ||
> > -                                WTERMSIG(status) != SIGKILL) {
> > -                             tst_res(TFAIL,
> > -                                      "child unexpectedly failed: %d",
> > -                                      status);
> > -                     }
> >               }
> >       }
> >  }
> > --
> > 2.20.1
> >
> >
>
Jan Stancek March 20, 2019, 8:10 a.m. UTC | #3
----- Original Message -----
> @Vipin, is there any possible you can have a look Jan's question?
> 
> On Wed, Mar 20, 2019 at 12:30 AM Jan Stancek <jstancek@redhat.com> wrote:
> 
> >
> >
> > ----- Original Message -----
> > > From: Vipin K Parashar <vipin@linux.vnet.ibm.com>
> > >
> > > Fixes: #349
> > >
> > > min_free_kbytes test has badly formed if conditions in mem_tune()
> > > for child exit status check.
> >
> > Can you please elaborate what's wrong about it?
> >
> 
> I‘m not sure if Vipin could help to explain this. I just nocited that this
> pull request was pending there more than half year, so I re-send this patch
> to ML for more discussion.
> https://github.com/linux-test-project/ltp/pull/350
> 
> 
> > > This is causing test to declare as FAILED
> > > despite that not being the case.
> >
> > Why? Shouldn't mmap succeed with overcommit == 1?
> >
> 
> I was unable to reproduce this. But AFAIK, 'overcommit == 1' cann't
> guarantee the MAP_FAILED(with ENOMEM) before oom.

With 'overcommit == 1' test always expects OOM. Test output below
suggests that child ended normally, because mmap failed.

> So I roughly made change
> in the original patch to catch up both oom and map_failed scenario for any
> 'overcommit'.
> 
> Beside that, as I was pointed out in patch 3/3, this test has an obviously
> defect in design, I'm also not sure if MemFree + (1/10 *min_free_kbytes) is
> safty enough, but that's only what I can do for the case remedy.
> 
> 
> > > Fix child exit status check conditions.
> > >
> > > ---ERROR LOG---
> > > <..snip..>
> > > mem.c:839: INFO: set overcommit_memory to 1
> > > mem.c:839: INFO: set min_free_kbytes to 11580
> > > memfree is 6974720 kB before eatup mem
> > > memfree is 15488 kB after eatup mem
> > > min_free_kbytes.c:135: FAIL: child unexpectedly failed: 0
> > > mem.c:839: INFO: set min_free_kbytes to 23160
> > > memfree is 7104128 kB before eatup mem
> > > memfree is 26560 kB after eatup mem
> > > min_free_kbytes.c:135: FAIL: child unexpectedly failed: 0
> > > mem.c:839: INFO: set min_free_kbytes to 145812
> > > memfree is 7101504 kB before eatup mem
> > > memfree is 215872 kB after eatup mem
> > > min_free_kbytes.c:135: FAIL: child unexpectedly failed: 0
> > > min_free_kbytes.c:81: PASS: min_free_kbytes test pass
> > > mem.c:839: INFO: set min_free_kbytes to 11580
> > > ---------------
> > >
> > > Signed-off-by: Vipin K Parashar <vipin@linux.vnet.ibm.com>
> > > Signed-off-by: Li Wang <liwang@redhat.com>
> > > Cc: Jan Stancek <jstancek@redhat.com>
> > > ---
> > >  .../kernel/mem/tunable/min_free_kbytes.c      | 40 +++++--------------
> > >  1 file changed, 11 insertions(+), 29 deletions(-)
> > >
> > > diff --git a/testcases/kernel/mem/tunable/min_free_kbytes.c
> > > b/testcases/kernel/mem/tunable/min_free_kbytes.c
> > > index f114dc493..d2378a700 100644
> > > --- a/testcases/kernel/mem/tunable/min_free_kbytes.c
> > > +++ b/testcases/kernel/mem/tunable/min_free_kbytes.c
> > > @@ -119,39 +119,21 @@ static void test_tune(unsigned long
> > overcommit_policy)
> > >
> > >               SAFE_WAITPID(pid[i], &status, WUNTRACED | WCONTINUED);
> > >
> > > -             if (overcommit_policy == 2) {
> > > -                     if (!WIFEXITED(status) || WEXITSTATUS(status) != 0)
> > > -                             tst_res(TFAIL,
> > > -                                      "child unexpectedly failed: %d",
> > > -                                      status);
> > > -             } else if (overcommit_policy == 1) {
> > > -                     if (!WIFSIGNALED(status) || WTERMSIG(status) !=
> > SIGKILL)
> > > +             if (WIFEXITED(status) && WEXITSTATUS(status) != 0) {
> > > +                     tst_res(TFAIL,
> > > +                             "child unexpectedly failed: %d", status);
> > > +             } else if (WIFSIGNALED(status) && WTERMSIG(status) !=
> > SIGKILL) {
> > >  #if __WORDSIZE == 32
> > > -                     {
> > > -                             if (total_mem < 3145728UL)
> > > +                     if (total_mem < 3145728UL)
> > >  #endif
> > > -                                     tst_res(TFAIL,
> > > -                                              "child unexpectedly
> > failed: %d",
> > > -                                              status);
> > > +                             tst_res(TFAIL,
> > > +                                     "child unexpectedly failed: %d",
> > status);
> > >  #if __WORDSIZE == 32
> > > -                             /* in 32-bit system, a process allocate
> > about 3Gb memory at most */
> > > -                             else
> > > -                                     tst_res(TINFO, "Child can't
> > allocate "
> > > -                                              ">3Gb memory in 32bit
> > system");
> > > -                     }
> > > +                     /* in 32-bit system, a process allocate about 3Gb
> > memory at most */
> > > +                     else
> > > +                             tst_res(TINFO, "Child can't allocate "
> > > +                                     ">3Gb memory in 32bit system");
> > >  #endif
> > > -             } else {
> > > -                     if (WIFEXITED(status)) {
> > > -                             if (WEXITSTATUS(status) != 0) {
> > > -                                     tst_res(TFAIL, "child unexpectedly
> > "
> > > -                                              "failed: %d", status);
> > > -                             }
> > > -                     } else if (!WIFSIGNALED(status) ||
> > > -                                WTERMSIG(status) != SIGKILL) {
> > > -                             tst_res(TFAIL,
> > > -                                      "child unexpectedly failed: %d",
> > > -                                      status);
> > > -                     }
> > >               }
> > >       }
> > >  }
> > > --
> > > 2.20.1
> > >
> > >
> >
> 
> 
> --
> Regards,
> Li Wang
>

Patch
diff mbox series

diff --git a/testcases/kernel/mem/tunable/min_free_kbytes.c b/testcases/kernel/mem/tunable/min_free_kbytes.c
index f114dc493..d2378a700 100644
--- a/testcases/kernel/mem/tunable/min_free_kbytes.c
+++ b/testcases/kernel/mem/tunable/min_free_kbytes.c
@@ -119,39 +119,21 @@  static void test_tune(unsigned long overcommit_policy)
 
 		SAFE_WAITPID(pid[i], &status, WUNTRACED | WCONTINUED);
 
-		if (overcommit_policy == 2) {
-			if (!WIFEXITED(status) || WEXITSTATUS(status) != 0)
-				tst_res(TFAIL,
-					 "child unexpectedly failed: %d",
-					 status);
-		} else if (overcommit_policy == 1) {
-			if (!WIFSIGNALED(status) || WTERMSIG(status) != SIGKILL)
+		if (WIFEXITED(status) && WEXITSTATUS(status) != 0) {
+			tst_res(TFAIL,
+				"child unexpectedly failed: %d", status);
+		} else if (WIFSIGNALED(status) && WTERMSIG(status) != SIGKILL) {
 #if __WORDSIZE == 32
-			{
-				if (total_mem < 3145728UL)
+			if (total_mem < 3145728UL)
 #endif
-					tst_res(TFAIL,
-						 "child unexpectedly failed: %d",
-						 status);
+				tst_res(TFAIL,
+					"child unexpectedly failed: %d", status);
 #if __WORDSIZE == 32
-				/* in 32-bit system, a process allocate about 3Gb memory at most */
-				else
-					tst_res(TINFO, "Child can't allocate "
-						 ">3Gb memory in 32bit system");
-			}
+			/* in 32-bit system, a process allocate about 3Gb memory at most */
+			else
+				tst_res(TINFO, "Child can't allocate "
+					">3Gb memory in 32bit system");
 #endif
-		} else {
-			if (WIFEXITED(status)) {
-				if (WEXITSTATUS(status) != 0) {
-					tst_res(TFAIL, "child unexpectedly "
-						 "failed: %d", status);
-				}
-			} else if (!WIFSIGNALED(status) ||
-				   WTERMSIG(status) != SIGKILL) {
-				tst_res(TFAIL,
-					 "child unexpectedly failed: %d",
-					 status);
-			}
 		}
 	}
 }