* [LTP] [PATCH 1/2] checkpoints: avoid unnecesasry sleep in checkpoint_wake @ 2016-05-11 12:03 Jan Stancek 2016-05-11 12:03 ` [LTP] [PATCH 2/2] pause01: rewrite testcase Jan Stancek 2016-05-11 12:23 ` [LTP] [PATCH 1/2] checkpoints: avoid unnecesasry sleep in checkpoint_wake Cyril Hrubis 0 siblings, 2 replies; 6+ messages in thread From: Jan Stancek @ 2016-05-11 12:03 UTC (permalink / raw) To: ltp 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [LTP] [PATCH 2/2] pause01: rewrite testcase 2016-05-11 12:03 [LTP] [PATCH 1/2] checkpoints: avoid unnecesasry sleep in checkpoint_wake Jan Stancek @ 2016-05-11 12:03 ` Jan Stancek 2016-05-11 13:21 ` Cyril Hrubis 2016-05-11 12:23 ` [LTP] [PATCH 1/2] checkpoints: avoid unnecesasry sleep in checkpoint_wake Cyril Hrubis 1 sibling, 1 reply; 6+ messages in thread From: Jan Stancek @ 2016-05-11 12:03 UTC (permalink / raw) To: ltp 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [LTP] [PATCH 2/2] pause01: rewrite testcase 2016-05-11 12:03 ` [LTP] [PATCH 2/2] pause01: rewrite testcase Jan Stancek @ 2016-05-11 13:21 ` Cyril Hrubis 2016-05-12 9:11 ` Jan Stancek 0 siblings, 1 reply; 6+ messages in thread From: Cyril Hrubis @ 2016-05-11 13:21 UTC (permalink / raw) To: ltp 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [LTP] [PATCH 2/2] pause01: rewrite testcase 2016-05-11 13:21 ` Cyril Hrubis @ 2016-05-12 9:11 ` Jan Stancek 2016-05-12 9:14 ` Cyril Hrubis 0 siblings, 1 reply; 6+ messages in thread From: Jan Stancek @ 2016-05-12 9:11 UTC (permalink / raw) To: ltp ----- 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [LTP] [PATCH 2/2] pause01: rewrite testcase 2016-05-12 9:11 ` Jan Stancek @ 2016-05-12 9:14 ` Cyril Hrubis 0 siblings, 0 replies; 6+ messages in thread From: Cyril Hrubis @ 2016-05-12 9:14 UTC (permalink / raw) To: ltp 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
* [LTP] [PATCH 1/2] checkpoints: avoid unnecesasry sleep in checkpoint_wake 2016-05-11 12:03 [LTP] [PATCH 1/2] checkpoints: avoid unnecesasry sleep in checkpoint_wake Jan Stancek 2016-05-11 12:03 ` [LTP] [PATCH 2/2] pause01: rewrite testcase Jan Stancek @ 2016-05-11 12:23 ` Cyril Hrubis 1 sibling, 0 replies; 6+ messages in thread From: Cyril Hrubis @ 2016-05-11 12:23 UTC (permalink / raw) To: ltp 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 ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-05-12 9:14 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-05-11 12:03 [LTP] [PATCH 1/2] checkpoints: avoid unnecesasry sleep in checkpoint_wake Jan Stancek 2016-05-11 12:03 ` [LTP] [PATCH 2/2] pause01: rewrite testcase Jan Stancek 2016-05-11 13:21 ` Cyril Hrubis 2016-05-12 9:11 ` Jan Stancek 2016-05-12 9:14 ` Cyril Hrubis 2016-05-11 12:23 ` [LTP] [PATCH 1/2] checkpoints: avoid unnecesasry sleep in checkpoint_wake Cyril Hrubis
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.