| Message ID | 20190726162226.252750-2-surenb@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/2] tests: move common definitions and functions into pidfd.h | expand |
On Fri, Jul 26, 2019 at 09:22:26AM -0700, Suren Baghdasaryan wrote: > This adds testing for polling on pidfd of a process being killed. Test runs > 10000 iterations by default to stress test pidfd polling functionality. > It accepts an optional command-line parameter to override the number or > iterations to run. > Specifically, it tests for: > - pidfd_open on a child process succeeds > - pidfd_send_signal on a child process succeeds > - polling on pidfd succeeds and returns exactly one event > - returned event is POLLIN > - event is received within 3 secs of the process being killed > > 10000 iterations was chosen because of the race condition being tested > which is not consistently reproducible but usually is revealed after less > than 2000 iterations. > Reveals race fixed by commit b191d6491be6 ("pidfd: fix a poll race when setting exit_state") > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > --- > + close(pidfd); > + // Wait for child to prevent zombies Comment style should be /* */, but I think Christian would be kind enough to fix that when he applies the patch so shouldn't need a resend. Thanks Suren and Christian! Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
On Fri, Jul 26, 2019 at 10:07 AM Joel Fernandes <joel@joelfernandes.org> wrote: > > On Fri, Jul 26, 2019 at 09:22:26AM -0700, Suren Baghdasaryan wrote: > > This adds testing for polling on pidfd of a process being killed. Test runs > > 10000 iterations by default to stress test pidfd polling functionality. > > It accepts an optional command-line parameter to override the number or > > iterations to run. > > Specifically, it tests for: > > - pidfd_open on a child process succeeds > > - pidfd_send_signal on a child process succeeds > > - polling on pidfd succeeds and returns exactly one event > > - returned event is POLLIN > > - event is received within 3 secs of the process being killed > > > > 10000 iterations was chosen because of the race condition being tested > > which is not consistently reproducible but usually is revealed after less > > than 2000 iterations. > > Reveals race fixed by commit b191d6491be6 ("pidfd: fix a poll race when setting exit_state") > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > > --- > > > + close(pidfd); > > + // Wait for child to prevent zombies > > Comment style should be /* */, but I think Christian would be kind enough to > fix that when he applies the patch so shouldn't need a resend. Agrh! That's what I get for constantly switching between kernel and userspace development :\ Christian please let me know if I should respin this or you can fix it when taking this patch. Thanks! > > Thanks Suren and Christian! > > Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org> >
On Fri, Jul 26, 2019 at 01:07:28PM -0400, Joel Fernandes wrote: > On Fri, Jul 26, 2019 at 09:22:26AM -0700, Suren Baghdasaryan wrote: > > This adds testing for polling on pidfd of a process being killed. Test runs > > 10000 iterations by default to stress test pidfd polling functionality. > > It accepts an optional command-line parameter to override the number or > > iterations to run. > > Specifically, it tests for: > > - pidfd_open on a child process succeeds > > - pidfd_send_signal on a child process succeeds > > - polling on pidfd succeeds and returns exactly one event > > - returned event is POLLIN > > - event is received within 3 secs of the process being killed > > > > 10000 iterations was chosen because of the race condition being tested > > which is not consistently reproducible but usually is revealed after less > > than 2000 iterations. > > Reveals race fixed by commit b191d6491be6 ("pidfd: fix a poll race when setting exit_state") > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > > --- > > > + close(pidfd); > > + // Wait for child to prevent zombies > > Comment style should be /* */, but I think Christian would be kind enough to > fix that when he applies the patch so shouldn't need a resend. No, I will force you to resend this whole thing. :) Jk, I'll replace the // when I apply it to my tree; no problem. :) > > Thanks Suren and Christian! > > Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org> >
Hi, Le vendredi 26 juillet 2019 à 09:22 -0700, Suren Baghdasaryan a écrit : > This adds testing for polling on pidfd of a process being killed. Test runs > 10000 iterations by default to stress test pidfd polling functionality. > It accepts an optional command-line parameter to override the number or > iterations to run. > Specifically, it tests for: > - pidfd_open on a child process succeeds > - pidfd_send_signal on a child process succeeds > - polling on pidfd succeeds and returns exactly one event > - returned event is POLLIN > - event is received within 3 secs of the process being killed > > 10000 iterations was chosen because of the race condition being tested > which is not consistently reproducible but usually is revealed after less > than 2000 iterations. > Reveals race fixed by commit b191d6491be6 ("pidfd: fix a poll race when setting exit_state") > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > --- > tools/testing/selftests/pidfd/.gitignore | 1 + > tools/testing/selftests/pidfd/Makefile | 2 +- > .../testing/selftests/pidfd/pidfd_poll_test.c | 117 ++++++++++++++++++ > 3 files changed, 119 insertions(+), 1 deletion(-) > create mode 100644 tools/testing/selftests/pidfd/pidfd_poll_test.c > > diff --git a/tools/testing/selftests/pidfd/.gitignore b/tools/testing/selftests/pidfd/.gitignore > index 16d84d117bc0..a67896347d34 100644 > --- a/tools/testing/selftests/pidfd/.gitignore > +++ b/tools/testing/selftests/pidfd/.gitignore > @@ -1,2 +1,3 @@ > pidfd_open_test > +pidfd_poll_test > pidfd_test > diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile > index 720b2d884b3c..ed58b7108d18 100644 > --- a/tools/testing/selftests/pidfd/Makefile > +++ b/tools/testing/selftests/pidfd/Makefile > @@ -1,7 +1,7 @@ > # SPDX-License-Identifier: GPL-2.0-only > CFLAGS += -g -I../../../../usr/include/ -lpthread > > -TEST_GEN_PROGS := pidfd_test pidfd_open_test > +TEST_GEN_PROGS := pidfd_test pidfd_open_test pidfd_poll_test > > include ../lib.mk > > diff --git a/tools/testing/selftests/pidfd/pidfd_poll_test.c b/tools/testing/selftests/pidfd/pidfd_poll_test.c > new file mode 100644 > index 000000000000..f1b62b91e53e > --- /dev/null > +++ b/tools/testing/selftests/pidfd/pidfd_poll_test.c > @@ -0,0 +1,117 @@ > +// SPDX-License-Identifier: GPL-2.0 > + > +#define _GNU_SOURCE > +#include <errno.h> > +#include <linux/types.h> > +#include <linux/wait.h> > +#include <poll.h> > +#include <signal.h> > +#include <stdbool.h> > +#include <stdio.h> > +#include <stdlib.h> > +#include <string.h> > +#include <syscall.h> > +#include <sys/wait.h> > +#include <unistd.h> > + > +#include "pidfd.h" > +#include "../kselftest.h" > + > +static bool timeout; > + > +static void handle_alarm(int sig) > +{ > + timeout = true; > +} Not needed if poll() timeout is used instead. > + > +int main(int argc, char **argv) > +{ > + struct pollfd fds; > + int iter, nevents; > + int nr_iterations = 10000; > + > + fds.events = POLLIN; > + > + if (argc > 2) > + ksft_exit_fail_msg("Unexpected command line argument\n"); > + > + if (argc == 2) { > + nr_iterations = atoi(argv[1]); > + if (nr_iterations <= 0) > + ksft_exit_fail_msg("invalid input parameter %s\n", > + argv[1]); > + } > + > + ksft_print_msg("running pidfd poll test for %d iterations\n", > + nr_iterations); > + > + for (iter = 0; iter < nr_iterations; iter++) { > + int pidfd; > + int child_pid = fork(); > + > + if (child_pid < 0) { > + if (errno == EAGAIN) { > + iter--; > + continue; > + } > + ksft_exit_fail_msg( > + "%s - failed to fork a child process\n", > + strerror(errno)); > + } > + > + if (child_pid == 0) { > + /* Child process just sleeps for a min and exits */ > + sleep(60); Instead of relying on a timer for no reason, I would use the following: while (1) pause(); > + exit(EXIT_SUCCESS); > + } > + > + /* Parent kills the child and waits for its death */ > + pidfd = sys_pidfd_open(child_pid, 0); > + if (pidfd < 0) > + ksft_exit_fail_msg("%s - pidfd_open failed\n", > + strerror(errno)); > + > + /* Setup 3 sec alarm - plenty of time */ > + if (signal(SIGALRM, handle_alarm) == SIG_ERR) > + ksft_exit_fail_msg("%s - signal failed\n", > + strerror(errno)); > + alarm(3); > + Would the poll() timeout be more simpler to use than relying on SIGALRM: no need to setup signal, no need for handler, no need for timeout variable. > + /* Send SIGKILL to the child */ > + if (sys_pidfd_send_signal(pidfd, SIGKILL, NULL, 0)) > + ksft_exit_fail_msg("%s - pidfd_send_signal failed\n", > + strerror(errno)); > + > + /* Wait for the death notification */ > + fds.fd = pidfd; > + nevents = poll(&fds, 1, -1); > + With nevents = poll(&fds, 1, 3000); > + /* Check for error conditions */ > + if (nevents < 0) > + ksft_exit_fail_msg("%s - poll failed\n", > + strerror(errno)); > + And if (nevents == 0) ksft_exit_fail_msg( "death notification wait timeout\n"); > + if (nevents != 1) > + ksft_exit_fail_msg("unexpected poll result: %d\n", > + nevents); > + > + if (!(fds.revents & POLLIN)) > + ksft_exit_fail_msg( > + "unexpected event type received: 0x%x\n", > + fds.revents); > + > + if (timeout) > + ksft_exit_fail_msg( > + "death notification wait timeout\n"); > + > + close(pidfd); > + // Wait for child to prevent zombies > + if (waitpid(child_pid, NULL, 0) < 0) > + ksft_exit_fail_msg("%s - waitpid failed\n", > + strerror(errno)); I feel safer now you defeated the zombies army :) Thanks. > + > + } > + > + ksft_test_result_pass("pidfd poll test: pass\n"); > + return ksft_exit_pass(); > +} Regards
On Fri, Jul 26, 2019 at 09:22:26AM -0700, Suren Baghdasaryan wrote: > This adds testing for polling on pidfd of a process being killed. Test runs > 10000 iterations by default to stress test pidfd polling functionality. > It accepts an optional command-line parameter to override the number or > iterations to run. > Specifically, it tests for: > - pidfd_open on a child process succeeds > - pidfd_send_signal on a child process succeeds > - polling on pidfd succeeds and returns exactly one event > - returned event is POLLIN > - event is received within 3 secs of the process being killed > > 10000 iterations was chosen because of the race condition being tested > which is not consistently reproducible but usually is revealed after less > than 2000 iterations. > Reveals race fixed by commit b191d6491be6 ("pidfd: fix a poll race when setting exit_state") > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> Applied-to: https://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git/log/?h=pidfd Tracked-in: https://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux.git/log/?h=for-next
On Sat, Jul 27, 2019 at 11:24 AM Yann Droneaud <ydroneaud@opteya.com> wrote: > > Hi, Hi Yann, So sorry I missed your email. My email filters were messed up and I fixed them now but missed a bunch of emails :( > > Le vendredi 26 juillet 2019 à 09:22 -0700, Suren Baghdasaryan a écrit : > > This adds testing for polling on pidfd of a process being killed. Test runs > > 10000 iterations by default to stress test pidfd polling functionality. > > It accepts an optional command-line parameter to override the number or > > iterations to run. > > Specifically, it tests for: > > - pidfd_open on a child process succeeds > > - pidfd_send_signal on a child process succeeds > > - polling on pidfd succeeds and returns exactly one event > > - returned event is POLLIN > > - event is received within 3 secs of the process being killed > > > > 10000 iterations was chosen because of the race condition being tested > > which is not consistently reproducible but usually is revealed after less > > than 2000 iterations. > > Reveals race fixed by commit b191d6491be6 ("pidfd: fix a poll race when setting exit_state") > > > > Signed-off-by: Suren Baghdasaryan <surenb@google.com> > > --- > > tools/testing/selftests/pidfd/.gitignore | 1 + > > tools/testing/selftests/pidfd/Makefile | 2 +- > > .../testing/selftests/pidfd/pidfd_poll_test.c | 117 ++++++++++++++++++ > > 3 files changed, 119 insertions(+), 1 deletion(-) > > create mode 100644 tools/testing/selftests/pidfd/pidfd_poll_test.c > > > > diff --git a/tools/testing/selftests/pidfd/.gitignore b/tools/testing/selftests/pidfd/.gitignore > > index 16d84d117bc0..a67896347d34 100644 > > --- a/tools/testing/selftests/pidfd/.gitignore > > +++ b/tools/testing/selftests/pidfd/.gitignore > > @@ -1,2 +1,3 @@ > > pidfd_open_test > > +pidfd_poll_test > > pidfd_test > > diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile > > index 720b2d884b3c..ed58b7108d18 100644 > > --- a/tools/testing/selftests/pidfd/Makefile > > +++ b/tools/testing/selftests/pidfd/Makefile > > @@ -1,7 +1,7 @@ > > # SPDX-License-Identifier: GPL-2.0-only > > CFLAGS += -g -I../../../../usr/include/ -lpthread > > > > -TEST_GEN_PROGS := pidfd_test pidfd_open_test > > +TEST_GEN_PROGS := pidfd_test pidfd_open_test pidfd_poll_test > > > > include ../lib.mk > > > > diff --git a/tools/testing/selftests/pidfd/pidfd_poll_test.c b/tools/testing/selftests/pidfd/pidfd_poll_test.c > > new file mode 100644 > > index 000000000000..f1b62b91e53e > > --- /dev/null > > +++ b/tools/testing/selftests/pidfd/pidfd_poll_test.c > > @@ -0,0 +1,117 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > + > > +#define _GNU_SOURCE > > +#include <errno.h> > > +#include <linux/types.h> > > +#include <linux/wait.h> > > +#include <poll.h> > > +#include <signal.h> > > +#include <stdbool.h> > > +#include <stdio.h> > > +#include <stdlib.h> > > +#include <string.h> > > +#include <syscall.h> > > +#include <sys/wait.h> > > +#include <unistd.h> > > + > > +#include "pidfd.h" > > +#include "../kselftest.h" > > + > > +static bool timeout; > > + > > +static void handle_alarm(int sig) > > +{ > > + timeout = true; > > +} > > Not needed if poll() timeout is used instead. > > > + > > +int main(int argc, char **argv) > > +{ > > + struct pollfd fds; > > + int iter, nevents; > > + int nr_iterations = 10000; > > + > > + fds.events = POLLIN; > > + > > + if (argc > 2) > > + ksft_exit_fail_msg("Unexpected command line argument\n"); > > + > > + if (argc == 2) { > > + nr_iterations = atoi(argv[1]); > > + if (nr_iterations <= 0) > > + ksft_exit_fail_msg("invalid input parameter %s\n", > > + argv[1]); > > + } > > + > > + ksft_print_msg("running pidfd poll test for %d iterations\n", > > + nr_iterations); > > + > > + for (iter = 0; iter < nr_iterations; iter++) { > > + int pidfd; > > + int child_pid = fork(); > > + > > + if (child_pid < 0) { > > + if (errno == EAGAIN) { > > + iter--; > > + continue; > > + } > > + ksft_exit_fail_msg( > > + "%s - failed to fork a child process\n", > > + strerror(errno)); > > + } > > + > > + if (child_pid == 0) { > > + /* Child process just sleeps for a min and exits */ > > + sleep(60); > > Instead of relying on a timer for no reason, I would use the following: > > while (1) > pause(); > Yeah, not much difference I think. If you don't strongly object I'll keep it this way. > > + exit(EXIT_SUCCESS); > > + } > > + > > + /* Parent kills the child and waits for its death */ > > + pidfd = sys_pidfd_open(child_pid, 0); > > + if (pidfd < 0) > > + ksft_exit_fail_msg("%s - pidfd_open failed\n", > > + strerror(errno)); > > + > > + /* Setup 3 sec alarm - plenty of time */ > > + if (signal(SIGALRM, handle_alarm) == SIG_ERR) > > + ksft_exit_fail_msg("%s - signal failed\n", > > + strerror(errno)); > > + alarm(3); > > + > > Would the poll() timeout be more simpler to use than relying on > SIGALRM: no need to setup signal, no need for handler, no need for > timeout variable. Unfortunately that would not work because after timeout is passed poll() checks for the condition one last time, it sees that the process is dead (the condition is satisfied) and it returns nevents==1 (event did happen) instead of nevents==0 (poll timed out). I did implement it this way originally and was surprised that it never timed out even when I saw a 3 sec delay but then figured it out after going through the kernel code. > > + /* Send SIGKILL to the child */ > > + if (sys_pidfd_send_signal(pidfd, SIGKILL, NULL, 0)) > > + ksft_exit_fail_msg("%s - pidfd_send_signal failed\n", > > + strerror(errno)); > > + > > + /* Wait for the death notification */ > > + fds.fd = pidfd; > > + nevents = poll(&fds, 1, -1); > > + > > With > > nevents = poll(&fds, 1, 3000); > > > + /* Check for error conditions */ > > + if (nevents < 0) > > + ksft_exit_fail_msg("%s - poll failed\n", > > + strerror(errno)); > > + > > And > if (nevents == 0) > ksft_exit_fail_msg( > "death notification wait timeout\n"); > > > + if (nevents != 1) > > + ksft_exit_fail_msg("unexpected poll result: %d\n", > > + nevents); > > + > > + if (!(fds.revents & POLLIN)) > > + ksft_exit_fail_msg( > > + "unexpected event type received: 0x%x\n", > > + fds.revents); > > + > > + if (timeout) > > + ksft_exit_fail_msg( > > + "death notification wait timeout\n"); > > + > > + close(pidfd); > > + // Wait for child to prevent zombies > > + if (waitpid(child_pid, NULL, 0) < 0) > > + ksft_exit_fail_msg("%s - waitpid failed\n", > > + strerror(errno)); > > I feel safer now you defeated the zombies army :) Thanks. Thanks for pointing it out! > > > + > > + } > > + > > + ksft_test_result_pass("pidfd poll test: pass\n"); > > + return ksft_exit_pass(); > > +} > > Regards > > -- > Yann Droneaud > OPTEYA > Thanks, Suren. > > -- > To unsubscribe from this group and stop receiving emails from it, send an email to kernel-team+unsubscribe@android.com. >
diff --git a/tools/testing/selftests/pidfd/.gitignore b/tools/testing/selftests/pidfd/.gitignore index 16d84d117bc0..a67896347d34 100644 --- a/tools/testing/selftests/pidfd/.gitignore +++ b/tools/testing/selftests/pidfd/.gitignore @@ -1,2 +1,3 @@ pidfd_open_test +pidfd_poll_test pidfd_test diff --git a/tools/testing/selftests/pidfd/Makefile b/tools/testing/selftests/pidfd/Makefile index 720b2d884b3c..ed58b7108d18 100644 --- a/tools/testing/selftests/pidfd/Makefile +++ b/tools/testing/selftests/pidfd/Makefile @@ -1,7 +1,7 @@ # SPDX-License-Identifier: GPL-2.0-only CFLAGS += -g -I../../../../usr/include/ -lpthread -TEST_GEN_PROGS := pidfd_test pidfd_open_test +TEST_GEN_PROGS := pidfd_test pidfd_open_test pidfd_poll_test include ../lib.mk diff --git a/tools/testing/selftests/pidfd/pidfd_poll_test.c b/tools/testing/selftests/pidfd/pidfd_poll_test.c new file mode 100644 index 000000000000..f1b62b91e53e --- /dev/null +++ b/tools/testing/selftests/pidfd/pidfd_poll_test.c @@ -0,0 +1,117 @@ +// SPDX-License-Identifier: GPL-2.0 + +#define _GNU_SOURCE +#include <errno.h> +#include <linux/types.h> +#include <linux/wait.h> +#include <poll.h> +#include <signal.h> +#include <stdbool.h> +#include <stdio.h> +#include <stdlib.h> +#include <string.h> +#include <syscall.h> +#include <sys/wait.h> +#include <unistd.h> + +#include "pidfd.h" +#include "../kselftest.h" + +static bool timeout; + +static void handle_alarm(int sig) +{ + timeout = true; +} + +int main(int argc, char **argv) +{ + struct pollfd fds; + int iter, nevents; + int nr_iterations = 10000; + + fds.events = POLLIN; + + if (argc > 2) + ksft_exit_fail_msg("Unexpected command line argument\n"); + + if (argc == 2) { + nr_iterations = atoi(argv[1]); + if (nr_iterations <= 0) + ksft_exit_fail_msg("invalid input parameter %s\n", + argv[1]); + } + + ksft_print_msg("running pidfd poll test for %d iterations\n", + nr_iterations); + + for (iter = 0; iter < nr_iterations; iter++) { + int pidfd; + int child_pid = fork(); + + if (child_pid < 0) { + if (errno == EAGAIN) { + iter--; + continue; + } + ksft_exit_fail_msg( + "%s - failed to fork a child process\n", + strerror(errno)); + } + + if (child_pid == 0) { + /* Child process just sleeps for a min and exits */ + sleep(60); + exit(EXIT_SUCCESS); + } + + /* Parent kills the child and waits for its death */ + pidfd = sys_pidfd_open(child_pid, 0); + if (pidfd < 0) + ksft_exit_fail_msg("%s - pidfd_open failed\n", + strerror(errno)); + + /* Setup 3 sec alarm - plenty of time */ + if (signal(SIGALRM, handle_alarm) == SIG_ERR) + ksft_exit_fail_msg("%s - signal failed\n", + strerror(errno)); + alarm(3); + + /* Send SIGKILL to the child */ + if (sys_pidfd_send_signal(pidfd, SIGKILL, NULL, 0)) + ksft_exit_fail_msg("%s - pidfd_send_signal failed\n", + strerror(errno)); + + /* Wait for the death notification */ + fds.fd = pidfd; + nevents = poll(&fds, 1, -1); + + /* Check for error conditions */ + if (nevents < 0) + ksft_exit_fail_msg("%s - poll failed\n", + strerror(errno)); + + if (nevents != 1) + ksft_exit_fail_msg("unexpected poll result: %d\n", + nevents); + + if (!(fds.revents & POLLIN)) + ksft_exit_fail_msg( + "unexpected event type received: 0x%x\n", + fds.revents); + + if (timeout) + ksft_exit_fail_msg( + "death notification wait timeout\n"); + + close(pidfd); + // Wait for child to prevent zombies + if (waitpid(child_pid, NULL, 0) < 0) + ksft_exit_fail_msg("%s - waitpid failed\n", + strerror(errno)); + + } + + ksft_test_result_pass("pidfd poll test: pass\n"); + return ksft_exit_pass(); +}
This adds testing for polling on pidfd of a process being killed. Test runs 10000 iterations by default to stress test pidfd polling functionality. It accepts an optional command-line parameter to override the number or iterations to run. Specifically, it tests for: - pidfd_open on a child process succeeds - pidfd_send_signal on a child process succeeds - polling on pidfd succeeds and returns exactly one event - returned event is POLLIN - event is received within 3 secs of the process being killed 10000 iterations was chosen because of the race condition being tested which is not consistently reproducible but usually is revealed after less than 2000 iterations. Reveals race fixed by commit b191d6491be6 ("pidfd: fix a poll race when setting exit_state") Signed-off-by: Suren Baghdasaryan <surenb@google.com> --- tools/testing/selftests/pidfd/.gitignore | 1 + tools/testing/selftests/pidfd/Makefile | 2 +- .../testing/selftests/pidfd/pidfd_poll_test.c | 117 ++++++++++++++++++ 3 files changed, 119 insertions(+), 1 deletion(-) create mode 100644 tools/testing/selftests/pidfd/pidfd_poll_test.c