[LTP] [PATCH] syscalls/pipe13: Add regression test for pipe to wake up all readers

[LTP] [PATCH] syscalls/pipe13: Add regression test for pipe to wake up all readers

[Yang Xu]

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

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"},
+		{}
+	}
+};
-- 
2.18.0

[LTP] [PATCH] syscalls/pipe13: Add regression test for pipe to wake up all readers

[Cyril Hrubis]

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

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] syscalls/pipe13: Add regression test for pipe to wake up all readers

[Yang Xu]

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
> 

[LTP] [PATCH v2] syscalls/pipe13: Add regression test for pipe to wake up all readers

[Yang Xu]

Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
---
v1->v2:
1.use ltp library wake and wait api
2.remove useless var

 runtest/syscalls                          |  1 +
 testcases/kernel/syscalls/pipe/.gitignore |  1 +
 testcases/kernel/syscalls/pipe/pipe13.c   | 73 +++++++++++++++++++++++
 3 files changed, 75 insertions(+)
 create mode 100644 testcases/kernel/syscalls/pipe/pipe13.c

diff --git a/runtest/syscalls b/runtest/syscalls
index d2551b2ec..f51456b8f 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..cd0d166a5
--- /dev/null
+++ b/testcases/kernel/syscalls/pipe/pipe13.c
@@ -0,0 +1,73 @@
+// 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 int fds[2];
+static int pid[2];
+static char buf;
+
+static void do_child(int i)
+{
+	SAFE_CLOSE(fds[1]);
+	TST_CHECKPOINT_WAKE(i);
+	SAFE_READ(0, fds[0], &buf, 1);
+	exit(0);
+}
+
+static void verify_pipe(void)
+{
+	int status;
+	int i;
+
+	SAFE_PIPE(fds);
+
+	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(pid[0], &status, 0);
+	TEST(waitpid(pid[1], &status, WNOHANG));
+	if (TST_RET != pid[1]) {
+		tst_res(TFAIL, "pipe doesn't waked up everybody when last write closes, "
+				"child %d existed", pid[1]);
+		goto out_kill;
+	}
+	tst_res(TPASS, "pipe has waked up everybody when last write close");
+	return;
+
+out_kill:
+	SAFE_KILL(pid[1], SIGKILL);
+	SAFE_WAIT(&status);
+}
+
+static struct tst_test test = {
+	.test_all = verify_pipe,
+	.forks_child = 1,
+	.needs_checkpoints = 1,
+	.tags = (const struct tst_tag[]) {
+		{"linux-git", "6551d5c56eb"},
+		{}
+	}
+};
-- 
2.18.0

[LTP] [PATCH] syscalls/pipe13: Add regression test for pipe to wake up all readers

[Cyril Hrubis]

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.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] syscalls/pipe13: Add regression test for pipe to wake up all readers

[Yang Xu]

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
> 

[LTP] [PATCH v3] syscalls/pipe13: Add regression test for pipe to wake up all readers

[Yang Xu]

Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
Signed-off-by: Cyril Hrubis <chrubis@suse.cz>
---
v2->v3:
1.use Cyril's waitpid way and add his signed-off-by
2. change child num to 10, IMO, it is more meaningful than 2
 runtest/syscalls                          |  1 +
 testcases/kernel/syscalls/pipe/.gitignore |  1 +
 testcases/kernel/syscalls/pipe/pipe13.c   | 87 +++++++++++++++++++++++
 3 files changed, 89 insertions(+)
 create mode 100644 testcases/kernel/syscalls/pipe/pipe13.c

diff --git a/runtest/syscalls b/runtest/syscalls
index d2551b2ec..f51456b8f 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..cc3f9f55f
--- /dev/null
+++ b/testcases/kernel/syscalls/pipe/pipe13.c
@@ -0,0 +1,87 @@
+// 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 case is designed to test whether pipe can wakeup all readers
+ * when last writer closes.
+ *
+ * This is also a regression test for commit 6551d5c56eb0
+ * ("pipe: make sure to wake up everybody when the last reader/writer closes").
+ * This bug was introduced by commit 0ddad21d3e99 ("pipe: use exclusive
+ * waits when reading or writing").
+ */
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/wait.h>
+#include <stdlib.h>
+#include "tst_test.h"
+
+#define CHILD_NUM 10
+
+static int pid[CHILD_NUM];
+static int fds[2];
+static char buf;
+
+static void do_child(unsigned int i)
+{
+	SAFE_CLOSE(fds[1]);
+	TST_CHECKPOINT_WAKE(i);
+	SAFE_READ(0, fds[0], &buf, 1);
+	exit(0);
+}
+
+static void verify_pipe(void)
+{
+	int ret;
+	unsigned int i, cnt = 0, sleep_us = 1, fail = 0;
+
+	SAFE_PIPE(fds);
+	tst_res(TINFO, "Creating %d child processes", CHILD_NUM);
+	for (i = 0; i < CHILD_NUM; 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]);
+	while (cnt < CHILD_NUM && sleep_us < 100000) {
+		ret = waitpid(-1, NULL, WNOHANG);
+		if (ret < 0)
+			tst_brk(TBROK | TERRNO, "waitpid()");
+		if (ret > 0) {
+			cnt++;
+			for (i = 0; i < CHILD_NUM; i++) {
+				if (pid[i] == ret)
+					pid[i] = 0;
+			}
+			continue;
+		}
+		usleep(sleep_us);
+		sleep_us *= 2;
+	}
+	for (i = 0; i < CHILD_NUM; 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 up everyone");
+	else
+		tst_res(TPASS, "Closed pipe waked up everyone");
+}
+
+static struct tst_test test = {
+	.test_all = verify_pipe,
+	.forks_child = 1,
+	.needs_checkpoints = 1,
+	.tags = (const struct tst_tag[]) {
+		{"linux-git", "6551d5c56eb"},
+		{}
+	}
+};
-- 
2.18.0

[LTP] [PATCH v3] syscalls/pipe13: Add regression test for pipe to wake up all readers

[Cyril Hrubis]

Hi!
I've added a few more fixes/assertions and pushed, thanks.

* Changed the code to run the test for a few different number of
  children

* Added check for return value from the read() in the child

* Added SAFE_CLOSE(fds[0]) into the parent
  - this fixes failures with large enough -i parameter where the test
    would run out of file descriptors

-- 
Cyril Hrubis
chrubis@suse.cz