diff mbox series

syscalls/pipe13: Add regression test for pipe to wake up all readers

Message ID 1582537946-22098-1-git-send-email-xuyang2018.jy@cn.fujitsu.com
State Changes Requested
Headers show
Series syscalls/pipe13: Add regression test for pipe to wake up all readers | expand

Commit Message

Yang Xu Feb. 24, 2020, 9:52 a.m. UTC 
Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
---
 runtest/syscalls                          |  1 +
 testcases/kernel/syscalls/pipe/.gitignore |  1 +
 testcases/kernel/syscalls/pipe/pipe13.c   | 64 +++++++++++++++++++++++
 3 files changed, 66 insertions(+)
 create mode 100644 testcases/kernel/syscalls/pipe/pipe13.c

Comments

Cyril Hrubis Feb. 24, 2020, 12:58 p.m. UTC | #1 
Hi!
> +static void verify_pipe(void)
> +{
> +	int fds[2];
> +	int status1, status2;
> +	pid_t p1, p2;
> +	int ret;
> +
> +	SAFE_PIPE(fds);
> +
> +	p1 = SAFE_FORK();
> +	if (p1 == 0) {
> +		SAFE_CLOSE(fds[1]);
> +		SAFE_READ(0, fds[0], &status1, sizeof(status1));
                                        ^
					Why status1 here?

					can't we just pass a dummy
					char buf; and size 1 here?

					It's not being written to
					anyways.
> +		exit(0);
> +	}
> +	p2 = SAFE_FORK();
> +	if (p2 == 0) {
> +		SAFE_CLOSE(fds[1]);
> +		SAFE_READ(0, fds[0], &status2, sizeof(status2));
                                      ^
				      Here as well.
> +		exit(0);
> +	}
> +
> +	sleep(1);

This sleep here has to be replaced by a proper synchronization given
that it's here to make sure both of the readers sleep on the pipe we
should:

* Use checkpoints to make sure both of the children have managed
  to get before the SAFE_READ().

* The the parent should use the TST_PROCESS_STATE_WAIT() to make sure
  both of the chidlren are sleeping

> +	SAFE_CLOSE(fds[1]);
> +
> +	SAFE_WAITPID(p1, &status1, 0);
> +	ret = waitpid(p2, &status2, WNOHANG);

We should just use waitpid with -1 as a pid here and WNOHANG twice,
because if one of the children hangs it's not guaranteed in any way
which one would that be.

> +	if (ret == p2) {
> +		tst_res(TPASS, "pipe wakes up everybody when last write closes");
> +	} else {
> +		tst_res(TFAIL, "pipe doesn't wake up every body when last write closes");
> +		SAFE_KILL(p2, SIGKILL);
> +		SAFE_WAIT(&status2);
> +	}
> +}
> +
> +static struct tst_test test = {
> +	.test_all = verify_pipe,
> +	.forks_child = 1,
> +	.tags = (const struct tst_tag[]) {
> +		{"linux-git", "6551d5c56eb"},
> +		{}
> +	}
> +};
> -- 
> 2.18.0
> 
> 
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp
Yang Xu Feb. 25, 2020, 9:07 a.m. UTC | #2 
Hi!

> Hi!
>> +static void verify_pipe(void)
>> +{
>> +	int fds[2];
>> +	int status1, status2;
>> +	pid_t p1, p2;
>> +	int ret;
>> +
>> +	SAFE_PIPE(fds);
>> +
>> +	p1 = SAFE_FORK();
>> +	if (p1 == 0) {
>> +		SAFE_CLOSE(fds[1]);
>> +		SAFE_READ(0, fds[0], &status1, sizeof(status1));
>                                          ^
> 					Why status1 here?
> 
> 					can't we just pass a dummy
> 					char buf; and size 1 here?
> 
> 					It's not being written to
> 					anyways.
>> +		exit(0);
>> +	}
>> +	p2 = SAFE_FORK();
>> +	if (p2 == 0) {
>> +		SAFE_CLOSE(fds[1]);
>> +		SAFE_READ(0, fds[0], &status2, sizeof(status2));
>                                        ^
> 				      Here as well.
>> +		exit(0);
>> +	}
>> +
>> +	sleep(1);
> 
> This sleep here has to be replaced by a proper synchronization given
> that it's here to make sure both of the readers sleep on the pipe we
> should:
> 
> * Use checkpoints to make sure both of the children have managed
>    to get before the SAFE_READ().
> 
> * The the parent should use the TST_PROCESS_STATE_WAIT() to make sure
>    both of the chidlren are sleeping
OK, I will use wait and wakeup api as below:


static void do_child(int i)
{
         char buf;
         SAFE_CLOSE(fds[1]);
         TST_CHECKPOINT_WAKE(i);
         SAFE_READ(0, fds[0], &buf, 1);
         exit(0);
}

staic void verify_pipe(void)
....
for (i = 0; i < 2; i++) {
                 pid[i] = SAFE_FORK();
                 if (pid[i] == 0)
                         do_child(i);
                 TST_CHECKPOINT_WAIT(i);
                 TST_PROCESS_STATE_WAIT(pid[i], 'S', 0);
         }
....
> 
>> +	SAFE_CLOSE(fds[1]);
>> +
>> +	SAFE_WAITPID(p1, &status1, 0);
>> +	ret = waitpid(p2, &status2, WNOHANG);
> 
> We should just use waitpid with -1 as a pid here and WNOHANG twice,
> because if one of the children hangs it's not guaranteed in any way
> which one would that be.
> 
On my environment, kernel wakes up the first read and the remaining read 
doesn't be waked up. (I add three childs, 2,3 doesn't wake up)
>> +	if (ret == p2) {
>> +		tst_res(TPASS, "pipe wakes up everybody when last write closes");
>> +	} else {
>> +		tst_res(TFAIL, "pipe doesn't wake up every body when last write closes");
>> +		SAFE_KILL(p2, SIGKILL);
>> +		SAFE_WAIT(&status2);
>> +	}
>> +}
>> +
>> +static struct tst_test test = {
>> +	.test_all = verify_pipe,
>> +	.forks_child = 1,
>> +	.tags = (const struct tst_tag[]) {
>> +		{"linux-git", "6551d5c56eb"},
>> +		{}
>> +	}
>> +};
>> -- 
>> 2.18.0
>>
>>
>>
>>
>> -- 
>> Mailing list info: https://lists.linux.it/listinfo/ltp
>
Cyril Hrubis Feb. 25, 2020, 10:28 a.m. UTC | #3 
Hi!
> > We should just use waitpid with -1 as a pid here and WNOHANG twice,
> > because if one of the children hangs it's not guaranteed in any way
> > which one would that be.
> > 
> On my environment, kernel wakes up the first read and the remaining read 
> doesn't be waked up. (I add three childs, 2,3 doesn't wake up)

But that behavior is not written down in any standard, that's just how
the kernel internals are working at the moment, we should not assume it
will work like this in the future.

What I would do here would be:

	int ret, cnt = 0, sleep_us = 1, fail = 0;

	while (cnt < 2 && sleep_us < 100000) {
		ret = waitpid(-1, NULL, WNOHANG);

		if (ret < 0)
			tst_brk(TBROK | TERRNO, "waitpid()");

		if (ret > 0) {
			cnt++;
			for (i = 0; i < 2; i++) {
				if (pid[i] == ret)
					pid[i] = 0;
			}
			continue;
		}

		usleep(sleep_time);
		sleep_time *= 2;
	}

	for (i = 0; i < 2; i++) {
		if (pid[i]) {
			tst_res(TINFO, "pid %i still sleeps", pid[i]);
			fail = 1;
			SAFE_KILL(pid[i], SIGKILL);
			SAFE_WAIT(NULL);
		}
	}

	if (fail)
		tst_res(TFAIL, "Closed pipe didn't wake everyone");



This has also advantage that we can easily run the test even for 100
children as well as two if we change the upper bound of the for loops to
a variable.
Yang Xu Feb. 26, 2020, 1:27 a.m. UTC | #4 
Hi!

> Hi!
>>> We should just use waitpid with -1 as a pid here and WNOHANG twice,
>>> because if one of the children hangs it's not guaranteed in any way
>>> which one would that be.
>>>
>> On my environment, kernel wakes up the first read and the remaining read
>> doesn't be waked up. (I add three childs, 2,3 doesn't wake up)
> 
> But that behavior is not written down in any standard, that's just how
> the kernel internals are working at the moment, we should not assume it
> will work like this in the future.
Sound reasonable to me, I will follow your advise.
> 
> What I would do here would be:
> 
> 	int ret, cnt = 0, sleep_us = 1, fail = 0;
> 
> 	while (cnt < 2 && sleep_us < 100000) {
> 		ret = waitpid(-1, NULL, WNOHANG);
> 
> 		if (ret < 0)
> 			tst_brk(TBROK | TERRNO, "waitpid()");
> 
> 		if (ret > 0) {
> 			cnt++;
> 			for (i = 0; i < 2; i++) {
> 				if (pid[i] == ret)
> 					pid[i] = 0;
> 			}
> 			continue;
> 		}
> 
> 		usleep(sleep_time);
> 		sleep_time *= 2;
> 	}
> 
> 	for (i = 0; i < 2; i++) {
> 		if (pid[i]) {
> 			tst_res(TINFO, "pid %i still sleeps", pid[i]);
> 			fail = 1;
> 			SAFE_KILL(pid[i], SIGKILL);
> 			SAFE_WAIT(NULL);
> 		}
> 	}
> 
> 	if (fail)
> 		tst_res(TFAIL, "Closed pipe didn't wake everyone");
> 
> 
> 
> This has also advantage that we can easily run the test even for 100
> children as well as two if we change the upper bound of the for loops to
> a variable.
Yes. this way is more wise.

Best Regards
Yang Xu
>
diff mbox series

Patch

diff --git a/runtest/syscalls b/runtest/syscalls
index e42db9910..39d04a3a8 100644
--- a/runtest/syscalls
+++ b/runtest/syscalls
@@ -874,6 +874,7 @@  pipe09 pipe09
 pipe10 pipe10
 pipe11 pipe11
 pipe12 pipe12
+pipe13 pipe13
 
 pipe2_01 pipe2_01
 pipe2_02 pipe2_02
diff --git a/testcases/kernel/syscalls/pipe/.gitignore b/testcases/kernel/syscalls/pipe/.gitignore
index 90b502547..23e7186a6 100644
--- a/testcases/kernel/syscalls/pipe/.gitignore
+++ b/testcases/kernel/syscalls/pipe/.gitignore
@@ -10,3 +10,4 @@ 
 /pipe10
 /pipe11
 /pipe12
+/pipe13
diff --git a/testcases/kernel/syscalls/pipe/pipe13.c b/testcases/kernel/syscalls/pipe/pipe13.c
new file mode 100644
index 000000000..c2a89ba02
--- /dev/null
+++ b/testcases/kernel/syscalls/pipe/pipe13.c
@@ -0,0 +1,64 @@ 
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ * Copyright (c) 2020 FUJITSU LIMITED. All rights reserved.
+ * Author: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
+ *
+ * Test Description:
+ * This is a case to test whether pipe can wakeup all readers
+ * when last writer closes.
+ *
+ * This bug was introduced by commit 0ddad21d3e99 ("pipe: use exclusive
+ * waits when reading or writing").
+ * This bug has been fixed by commit 6551d5c56eb0 ("pipe: make sure to
+ * wake up everybody when the last reader/writer closes").
+ */
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <stdlib.h>
+#include "tst_test.h"
+
+static void verify_pipe(void)
+{
+	int fds[2];
+	int status1, status2;
+	pid_t p1, p2;
+	int ret;
+
+	SAFE_PIPE(fds);
+
+	p1 = SAFE_FORK();
+	if (p1 == 0) {
+		SAFE_CLOSE(fds[1]);
+		SAFE_READ(0, fds[0], &status1, sizeof(status1));
+		exit(0);
+	}
+	p2 = SAFE_FORK();
+	if (p2 == 0) {
+		SAFE_CLOSE(fds[1]);
+		SAFE_READ(0, fds[0], &status2, sizeof(status2));
+		exit(0);
+	}
+
+	sleep(1);
+	SAFE_CLOSE(fds[1]);
+
+	SAFE_WAITPID(p1, &status1, 0);
+	ret = waitpid(p2, &status2, WNOHANG);
+	if (ret == p2) {
+		tst_res(TPASS, "pipe wakes up everybody when last write closes");
+	} else {
+		tst_res(TFAIL, "pipe doesn't wake up every body when last write closes");
+		SAFE_KILL(p2, SIGKILL);
+		SAFE_WAIT(&status2);
+	}
+}
+
+static struct tst_test test = {
+	.test_all = verify_pipe,
+	.forks_child = 1,
+	.tags = (const struct tst_tag[]) {
+		{"linux-git", "6551d5c56eb"},
+		{}
+	}
+};