[LTP] [PATCH 1/2] checkpoints: avoid unnecesasry sleep in checkpoint_wake

[LTP] [PATCH 1/2] checkpoints: avoid unnecesasry sleep in checkpoint_wake

[Jan Stancek]

checkpoint_wake is currently sleeping also in case when
it wakes up all processes. This creates problem for combination
of checkpoints and process_state_wait, because it creates
false impression, that child is sleeping on operation
following checkpoint_wake (#2), while it really sleeps inside
checkpoint_wake (#1).

      child                      parent
TST_CHECKPOINT_WAKE #1
                               TST_CHECKPOINT_WAIT
                               TST_PROCESS_STATE_WAIT(child, 'S')
                               kill(child, SIGINT)
sleep() #2

This patch avoids sleep in checkpoint_wake if all processes has been
already woken up.

Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
 lib/tst_checkpoint.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/tst_checkpoint.c b/lib/tst_checkpoint.c
index 8f23ead26a8a..128e658153c6 100644
--- a/lib/tst_checkpoint.c
+++ b/lib/tst_checkpoint.c
@@ -109,6 +109,10 @@ int tst_checkpoint_wake(unsigned int id, unsigned int nr_wake,
 	do {
 		waked += syscall(SYS_futex, &tst_futexes[id], FUTEX_WAKE,
 				 INT_MAX, NULL);
+
+		if (waked == nr_wake)
+			break;
+
 		usleep(1000);
 		msecs++;
 
-- 
1.8.3.1

[LTP] [PATCH 2/2] pause01: rewrite testcase

[Jan Stancek]

This is a rewrite to avoid running into hang reported by Yury,
which is based on newlib.

Test is changed to do following:
1. parent will fork a child and wait
2. just before child calls pause() it signals parent
3. parent calls tst_process_state_wait() to wait until
   child process falls asleep (on pause() call)
4. parent sends signal to child
5. child checks that pause() exited with EINTR

Reported-by: Yury Norov <ynorov@caviumnetworks.com>
Signed-off-by: Jan Stancek <jstancek@redhat.com>
---
 testcases/kernel/syscalls/pause/pause01.c | 132 +++++++++++++-----------------
 1 file changed, 56 insertions(+), 76 deletions(-)

diff --git a/testcases/kernel/syscalls/pause/pause01.c b/testcases/kernel/syscalls/pause/pause01.c
index c967d8ec4f6a..091860830696 100644
--- a/testcases/kernel/syscalls/pause/pause01.c
+++ b/testcases/kernel/syscalls/pause/pause01.c
@@ -1,95 +1,75 @@
 /*
- * Copyright (c) 2000 Silicon Graphics, Inc.  All Rights Reserved.
- *    AUTHOR		: William Roske
- *    CO-PILOT		: Dave Fenner
+ * Copyright (c) 2016 Linux Test Project
  *
- * This program is free software; you can redistribute it and/or modify it
- * under the terms of version 2 of the GNU General Public License as
- * published by the Free Software Foundation.
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.
  *
- * This program is distributed in the hope that it would be useful, but
- * WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
+ * This program is distributed in the hope that it would be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
  *
- * Further, this software is distributed without any warranty that it is
- * free of the rightful claim of any third person regarding infringement
- * or the like.  Any license provided herein, whether implied or
- * otherwise, applies only to this software file.  Patent licenses, if
- * any, provided herein do not apply to combinations of this program with
- * other software, or any other product whatsoever.
- *
- * You should have received a copy of the GNU General Public License along
- * with this program; if not, write the Free Software Foundation, Inc.,
- * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
- *
- * Contact information: Silicon Graphics, Inc., 1600 Amphitheatre Pkwy,
- * Mountain View, CA  94043, or:
- *
- * http://www.sgi.com
- *
- * For further information regarding this notice, see:
- *
- * http://oss.sgi.com/projects/GenInfo/NoticeExplan/
- */
-/*
- * Setup alarm() signal, wait in pause() expect to be woken up.
+ * Check that pause() returns on signal with errno == EINTR.
  */
 #include <errno.h>
 #include <signal.h>
-#include "test.h"
+#include <stdlib.h>
+#include "tst_test.h"
 
-char *TCID = "pause01";
-int TST_TOTAL = 1;
-
-static void setup(void);
-
-int main(int ac, char **av)
+static void sig_handler(int sig LTP_ATTRIBUTE_UNUSED)
 {
-	int lc;
-	struct itimerval it = {
-		.it_interval = {.tv_sec = 0, .tv_usec = 0},
-		.it_value = {.tv_sec = 0, .tv_usec = 1000},
-	};
-
-	tst_parse_opts(ac, av, NULL, NULL);
+}
 
-	setup();
+static void do_child(void)
+{
+	if (signal(SIGINT, sig_handler) == SIG_ERR)
+		tst_brk(TBROK | TERRNO, "signal");
 
-	for (lc = 0; TEST_LOOPING(lc); lc++) {
-		tst_count = 0;
+	TST_CHECKPOINT_WAKE(0);
 
-		if (setitimer(ITIMER_REAL, &it, NULL))
-			tst_brkm(TBROK | TERRNO, NULL, "setitimer() failed");
+	TEST(pause());
+	if (TEST_RETURN != -1)
+		tst_res(TFAIL, "pause() succeeded unexpectedly");
+	else if (TEST_ERRNO == EINTR)
+		tst_res(TPASS, "pause() interrupted with EINTR");
+	else
+		tst_res(TFAIL | TTERRNO, "pause() unexpected errno");
 
-		TEST(pause());
+	TST_CHECKPOINT_WAKE(0);
+	exit(0);
+}
 
-		if (TEST_RETURN != -1) {
-			tst_resm(TFAIL,
-				 "pause() returned WITHOUT an error return code : %d",
-				 TEST_ERRNO);
-		} else {
-			if (TEST_ERRNO == EINTR)
-				tst_resm(TPASS, "pause() returned %ld",
-					 TEST_RETURN);
-			else
-				tst_resm(TFAIL,
-					 "pause() returned %ld. Expected %d (EINTR)",
-					 TEST_RETURN, EINTR);
-		}
+static void do_test(void)
+{
+	int pid, status;
+
+	pid = SAFE_FORK();
+	switch (pid) {
+	case 0:
+		do_child();
+		break;
+	case -1:
+		tst_brk(TBROK | TERRNO, "fork() failed");
+		break;
 	}
 
-	tst_exit();
-}
+	TST_CHECKPOINT_WAIT(0);
+	TST_PROCESS_STATE_WAIT(pid, 'S');
+	kill(pid, SIGINT);
 
-static void go(int sig)
-{
-	(void)sig;
+	/*
+	 * TST_CHECKPOINT_WAIT has built-in timeout, if pause() doesn't return,
+	 * this checkpoint call will reliably end the test.
+	 */
+	TST_CHECKPOINT_WAIT(0);
+	SAFE_WAIT(&status);
 }
 
-void setup(void)
-{
-	tst_sig(NOFORK, DEF_HANDLER, NULL);
-	(void)signal(SIGALRM, go);
-
-	TEST_PAUSE;
-}
+static struct tst_test test = {
+	.tid = "pause01",
+	.forks_child = 1,
+	.needs_checkpoints = 1,
+	.test_all = do_test,
+};
-- 
1.8.3.1

[LTP] [PATCH 1/2] checkpoints: avoid unnecesasry sleep in checkpoint_wake

[Cyril Hrubis]

Hi!
> checkpoint_wake is currently sleeping also in case when
> it wakes up all processes. This creates problem for combination
> of checkpoints and process_state_wait, because it creates
> false impression, that child is sleeping on operation
> following checkpoint_wake (#2), while it really sleeps inside
> checkpoint_wake (#1).
> 
>       child                      parent
> TST_CHECKPOINT_WAKE #1
>                                TST_CHECKPOINT_WAIT
>                                TST_PROCESS_STATE_WAIT(child, 'S')
>                                kill(child, SIGINT)
> sleep() #2
> 
> This patch avoids sleep in checkpoint_wake if all processes has been
> already woken up.

Good catch.

> Signed-off-by: Jan Stancek <jstancek@redhat.com>
> ---
>  lib/tst_checkpoint.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/lib/tst_checkpoint.c b/lib/tst_checkpoint.c
> index 8f23ead26a8a..128e658153c6 100644
> --- a/lib/tst_checkpoint.c
> +++ b/lib/tst_checkpoint.c
> @@ -109,6 +109,10 @@ int tst_checkpoint_wake(unsigned int id, unsigned int nr_wake,
>  	do {
>  		waked += syscall(SYS_futex, &tst_futexes[id], FUTEX_WAKE,
>  				 INT_MAX, NULL);
> +
> +		if (waked == nr_wake)
> +			break;
> +
>  		usleep(1000);
>  		msecs++;

I guess that we can also change the do { } while (waked != nr_wake) loop
to for (;;) { } now.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/2] pause01: rewrite testcase

[Cyril Hrubis]

Hi!
> This is a rewrite to avoid running into hang reported by Yury,
> which is based on newlib.
> 
> Test is changed to do following:
> 1. parent will fork a child and wait
> 2. just before child calls pause() it signals parent
> 3. parent calls tst_process_state_wait() to wait until
>    child process falls asleep (on pause() call)
> 4. parent sends signal to child
> 5. child checks that pause() exited with EINTR
> 
> Reported-by: Yury Norov <ynorov@caviumnetworks.com>
> Signed-off-by: Jan Stancek <jstancek@redhat.com>
> ---
>  testcases/kernel/syscalls/pause/pause01.c | 132 +++++++++++++-----------------
>  1 file changed, 56 insertions(+), 76 deletions(-)
> 
> diff --git a/testcases/kernel/syscalls/pause/pause01.c b/testcases/kernel/syscalls/pause/pause01.c
> index c967d8ec4f6a..091860830696 100644
> --- a/testcases/kernel/syscalls/pause/pause01.c
> +++ b/testcases/kernel/syscalls/pause/pause01.c
> @@ -1,95 +1,75 @@
>  /*
> - * Copyright (c) 2000 Silicon Graphics, Inc.  All Rights Reserved.
> - *    AUTHOR		: William Roske
> - *    CO-PILOT		: Dave Fenner
> + * Copyright (c) 2016 Linux Test Project
>   *
> - * This program is free software; you can redistribute it and/or modify it
> - * under the terms of version 2 of the GNU General Public License as
> - * published by the Free Software Foundation.
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
>   *
> - * This program is distributed in the hope that it would be useful, but
> - * WITHOUT ANY WARRANTY; without even the implied warranty of
> - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.
> + * This program is distributed in the hope that it would be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
>   *
> - * Further, this software is distributed without any warranty that it is
> - * free of the rightful claim of any third person regarding infringement
> - * or the like.  Any license provided herein, whether implied or
> - * otherwise, applies only to this software file.  Patent licenses, if
> - * any, provided herein do not apply to combinations of this program with
> - * other software, or any other product whatsoever.
> - *
> - * You should have received a copy of the GNU General Public License along
> - * with this program; if not, write the Free Software Foundation, Inc.,
> - * 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
> - *
> - * Contact information: Silicon Graphics, Inc., 1600 Amphitheatre Pkwy,
> - * Mountain View, CA  94043, or:
> - *
> - * http://www.sgi.com
> - *
> - * For further information regarding this notice, see:
> - *
> - * http://oss.sgi.com/projects/GenInfo/NoticeExplan/
> - */
> -/*
> - * Setup alarm() signal, wait in pause() expect to be woken up.
> + * Check that pause() returns on signal with errno == EINTR.
>   */
>  #include <errno.h>
>  #include <signal.h>
> -#include "test.h"
> +#include <stdlib.h>
> +#include "tst_test.h"
>  
> -char *TCID = "pause01";
> -int TST_TOTAL = 1;
> -
> -static void setup(void);
> -
> -int main(int ac, char **av)
> +static void sig_handler(int sig LTP_ATTRIBUTE_UNUSED)
>  {
> -	int lc;
> -	struct itimerval it = {
> -		.it_interval = {.tv_sec = 0, .tv_usec = 0},
> -		.it_value = {.tv_sec = 0, .tv_usec = 1000},
> -	};
> -
> -	tst_parse_opts(ac, av, NULL, NULL);
> +}
>  
> -	setup();
> +static void do_child(void)
> +{
> +	if (signal(SIGINT, sig_handler) == SIG_ERR)
> +		tst_brk(TBROK | TERRNO, "signal");

We should probably add SAFE_SIGNAL() to the newlib.

> -	for (lc = 0; TEST_LOOPING(lc); lc++) {
> -		tst_count = 0;
> +	TST_CHECKPOINT_WAKE(0);
>  
> -		if (setitimer(ITIMER_REAL, &it, NULL))
> -			tst_brkm(TBROK | TERRNO, NULL, "setitimer() failed");
> +	TEST(pause());
> +	if (TEST_RETURN != -1)
> +		tst_res(TFAIL, "pause() succeeded unexpectedly");
> +	else if (TEST_ERRNO == EINTR)
> +		tst_res(TPASS, "pause() interrupted with EINTR");
> +	else
> +		tst_res(TFAIL | TTERRNO, "pause() unexpected errno");
>  
> -		TEST(pause());
> +	TST_CHECKPOINT_WAKE(0);
> +	exit(0);
> +}
>  
> -		if (TEST_RETURN != -1) {
> -			tst_resm(TFAIL,
> -				 "pause() returned WITHOUT an error return code : %d",
> -				 TEST_ERRNO);
> -		} else {
> -			if (TEST_ERRNO == EINTR)
> -				tst_resm(TPASS, "pause() returned %ld",
> -					 TEST_RETURN);
> -			else
> -				tst_resm(TFAIL,
> -					 "pause() returned %ld. Expected %d (EINTR)",
> -					 TEST_RETURN, EINTR);
> -		}
> +static void do_test(void)
> +{
> +	int pid, status;
> +
> +	pid = SAFE_FORK();
> +	switch (pid) {
> +	case 0:
> +		do_child();
> +		break;
> +	case -1:
> +		tst_brk(TBROK | TERRNO, "fork() failed");
> +		break;
>  	}

Unlike tst_fork() SAFE_FORK() cannot fail, it call tst_brk() in case of
failure in the test library.

> -	tst_exit();
> -}
> +	TST_CHECKPOINT_WAIT(0);
> +	TST_PROCESS_STATE_WAIT(pid, 'S');
> +	kill(pid, SIGINT);
>  
> -static void go(int sig)
> -{
> -	(void)sig;
> +	/*
> +	 * TST_CHECKPOINT_WAIT has built-in timeout, if pause() doesn't return,
> +	 * this checkpoint call will reliably end the test.
> +	 */
> +	TST_CHECKPOINT_WAIT(0);
> +	SAFE_WAIT(&status);

Hmm, maybe it would be better if we add timeout logic to the new test
library instead of adding it to selected few testcases.

>  }
>  
> -void setup(void)
> -{
> -	tst_sig(NOFORK, DEF_HANDLER, NULL);
> -	(void)signal(SIGALRM, go);
> -
> -	TEST_PAUSE;
> -}
> +static struct tst_test test = {
> +	.tid = "pause01",
> +	.forks_child = 1,
> +	.needs_checkpoints = 1,
> +	.test_all = do_test,
> +};

Otherwise it looks good.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH 2/2] pause01: rewrite testcase

[Jan Stancek]

----- Original Message -----
> From: "Cyril Hrubis" <chrubis@suse.cz>
> To: "Jan Stancek" <jstancek@redhat.com>
> Cc: ltp@lists.linux.it
> Sent: Wednesday, 11 May, 2016 3:21:36 PM
> Subject: Re: [LTP] [PATCH 2/2] pause01: rewrite testcase

> > -static void go(int sig)
> > -{
> > -	(void)sig;
> > +	/*
> > +	 * TST_CHECKPOINT_WAIT has built-in timeout, if pause() doesn't return,
> > +	 * this checkpoint call will reliably end the test.
> > +	 */
> > +	TST_CHECKPOINT_WAIT(0);
> > +	SAFE_WAIT(&status);
> 
> Hmm, maybe it would be better if we add timeout logic to the new test
> library instead of adding it to selected few testcases.

I pushed it with all your suggestions, except for the timeout logic.

Did you have something specific in mind how to implement this?
My first thought was a new field to tst_test struct, and forking
a watchdog process.

Regards,
Jan

[LTP] [PATCH 2/2] pause01: rewrite testcase

[Cyril Hrubis]

Hi!
> > > -static void go(int sig)
> > > -{
> > > -	(void)sig;
> > > +	/*
> > > +	 * TST_CHECKPOINT_WAIT has built-in timeout, if pause() doesn't return,
> > > +	 * this checkpoint call will reliably end the test.
> > > +	 */
> > > +	TST_CHECKPOINT_WAIT(0);
> > > +	SAFE_WAIT(&status);
> > 
> > Hmm, maybe it would be better if we add timeout logic to the new test
> > library instead of adding it to selected few testcases.
> 
> I pushed it with all your suggestions, except for the timeout logic.
> 
> Did you have something specific in mind how to implement this?
> My first thought was a new field to tst_test struct, and forking
> a watchdog process.

I was thinking of forking watchdog process for every test since you
cannot really tell which testcases will block. Then have defined default
timeout in the test library with possible override in the tst_test
structure. But perhaps this is too heavy approach, I will have to
measure how much will this slow down the test execution.

-- 
Cyril Hrubis
chrubis@suse.cz