[LTP] [PATCH] pty04: Limit the number of packets sent to avoid timeout

[LTP] [PATCH] pty04: Limit the number of packets sent to avoid timeout

[Richard Palethorpe]

At the end of the test we transmit many packets while closing the PTY
to check for races in the kernel. However if the process which closes
the PTY is delayed this can result a very large number of packets
being transmitted. The kernel appears to handle this quite slowly
which can cause the test to timeout or even a softlockup.

This is not supposed to be a performance test, so this commit puts an
upper bound on the number of packets transmitted.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---

Hopefully will solve: https://github.com/linux-test-project/ltp/issues/674

 testcases/kernel/pty/pty04.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/testcases/kernel/pty/pty04.c b/testcases/kernel/pty/pty04.c
index 4adf2cbb7..a59de7830 100644
--- a/testcases/kernel/pty/pty04.c
+++ b/testcases/kernel/pty/pty04.c
@@ -136,7 +136,8 @@ static int open_pty(const struct ldisc_info *ldisc)
 static ssize_t try_write(int fd, const char *data,
 			 ssize_t size, ssize_t *written)
 {
-	ssize_t ret = write(fd, data, size);
+	ssize_t off = written ? *written : 0;
+	ssize_t ret = write(fd, data + off, size);
 
 	if (ret < 0)
 		return -(errno != EAGAIN);
@@ -149,6 +150,7 @@ static void write_pty(const struct ldisc_info *ldisc)
 	char *data;
 	ssize_t written, ret;
 	size_t len = 0;
+	int max_writes = 1000;
 
 	switch (ldisc->n) {
 	case N_SLIP:
@@ -190,7 +192,8 @@ static void write_pty(const struct ldisc_info *ldisc)
 
 	tst_res(TPASS, "Wrote PTY %s %d (2)", ldisc->name, ptmx);
 
-	while (try_write(ptmx, data, len, NULL) >= 0)
+	TST_CHECKPOINT_WAIT2(0, 100000);
+	while (max_writes-- && try_write(ptmx, data, len, NULL) >= 0)
 		;
 
 	tst_res(TPASS, "Writing to PTY interrupted by hangup");
@@ -331,7 +334,7 @@ static void read_netdev(const struct ldisc_info *ldisc)
 	check_data(ldisc, data, plen);
 	tst_res(TPASS, "Read netdev %s %d (2)", ldisc->name, sk);
 
-	TST_CHECKPOINT_WAKE(0);
+	TST_CHECKPOINT_WAKE2(0, 2);
 	while ((rlen = read(sk, data, plen)) > 0)
 		check_data(ldisc, data, rlen);
 
-- 
2.28.0

[LTP] [PATCH] pty04: Limit the number of packets sent to avoid timeout

[Cyril Hrubis]

Hi!
> At the end of the test we transmit many packets while closing the PTY
> to check for races in the kernel. However if the process which closes
> the PTY is delayed this can result a very large number of packets
> being transmitted. The kernel appears to handle this quite slowly
> which can cause the test to timeout or even a softlockup.
> 
> This is not supposed to be a performance test, so this commit puts an
> upper bound on the number of packets transmitted.
> 
> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
> ---
> 
> Hopefully will solve: https://github.com/linux-test-project/ltp/issues/674
> 
>  testcases/kernel/pty/pty04.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/testcases/kernel/pty/pty04.c b/testcases/kernel/pty/pty04.c
> index 4adf2cbb7..a59de7830 100644
> --- a/testcases/kernel/pty/pty04.c
> +++ b/testcases/kernel/pty/pty04.c
> @@ -136,7 +136,8 @@ static int open_pty(const struct ldisc_info *ldisc)
>  static ssize_t try_write(int fd, const char *data,
>  			 ssize_t size, ssize_t *written)
>  {
> -	ssize_t ret = write(fd, data, size);
> +	ssize_t off = written ? *written : 0;
> +	ssize_t ret = write(fd, data + off, size);

This seems to be correct, but should be send in a seprate patch.

>  	if (ret < 0)
>  		return -(errno != EAGAIN);
> @@ -149,6 +150,7 @@ static void write_pty(const struct ldisc_info *ldisc)
>  	char *data;
>  	ssize_t written, ret;
>  	size_t len = 0;
> +	int max_writes = 1000;
>  
>  	switch (ldisc->n) {
>  	case N_SLIP:
> @@ -190,7 +192,8 @@ static void write_pty(const struct ldisc_info *ldisc)
>  
>  	tst_res(TPASS, "Wrote PTY %s %d (2)", ldisc->name, ptmx);
>  
> -	while (try_write(ptmx, data, len, NULL) >= 0)
> +	TST_CHECKPOINT_WAIT2(0, 100000);
> +	while (max_writes-- && try_write(ptmx, data, len, NULL) >= 0)
>  		;

I wonder if we should change this to be time based instead. I.e. try to
write packets for 10 seconds or so, since hardcoding number of
iterations usually does not scale from embedded to supercomputers.

>  	tst_res(TPASS, "Writing to PTY interrupted by hangup");

And this should be true only if we do not run out of tries meanwhile,
right?

> @@ -331,7 +334,7 @@ static void read_netdev(const struct ldisc_info *ldisc)
>  	check_data(ldisc, data, plen);
>  	tst_res(TPASS, "Read netdev %s %d (2)", ldisc->name, sk);
>  
> -	TST_CHECKPOINT_WAKE(0);
> +	TST_CHECKPOINT_WAKE2(0, 2);
>  	while ((rlen = read(sk, data, plen)) > 0)
>  		check_data(ldisc, data, rlen);
>  
> -- 
> 2.28.0
> 
> 
> -- 
> Mailing list info: https://lists.linux.it/listinfo/ltp

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH] pty04: Limit the number of packets sent to avoid timeout

[Richard Palethorpe]

Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
>> At the end of the test we transmit many packets while closing the PTY
>> to check for races in the kernel. However if the process which closes
>> the PTY is delayed this can result a very large number of packets
>> being transmitted. The kernel appears to handle this quite slowly
>> which can cause the test to timeout or even a softlockup.
>> 
>> This is not supposed to be a performance test, so this commit puts an
>> upper bound on the number of packets transmitted.
>> 
>> Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
>> ---
>> 
>> Hopefully will solve: https://github.com/linux-test-project/ltp/issues/674
>> 
>>  testcases/kernel/pty/pty04.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>> 
>> diff --git a/testcases/kernel/pty/pty04.c b/testcases/kernel/pty/pty04.c
>> index 4adf2cbb7..a59de7830 100644
>> --- a/testcases/kernel/pty/pty04.c
>> +++ b/testcases/kernel/pty/pty04.c
>> @@ -136,7 +136,8 @@ static int open_pty(const struct ldisc_info *ldisc)
>>  static ssize_t try_write(int fd, const char *data,
>>  			 ssize_t size, ssize_t *written)
>>  {
>> -	ssize_t ret = write(fd, data, size);
>> +	ssize_t off = written ? *written : 0;
>> +	ssize_t ret = write(fd, data + off, size);
>
> This seems to be correct, but should be send in a seprate patch.

I suppose I can, but this is actually part of limiting the number of
packets otherwise we don't care that we try to resend the whole packet
each time.

>
>>  	if (ret < 0)
>>  		return -(errno != EAGAIN);
>> @@ -149,6 +150,7 @@ static void write_pty(const struct ldisc_info *ldisc)
>>  	char *data;
>>  	ssize_t written, ret;
>>  	size_t len = 0;
>> +	int max_writes = 1000;
>>  
>>  	switch (ldisc->n) {
>>  	case N_SLIP:
>> @@ -190,7 +192,8 @@ static void write_pty(const struct ldisc_info *ldisc)
>>  
>>  	tst_res(TPASS, "Wrote PTY %s %d (2)", ldisc->name, ptmx);
>>  
>> -	while (try_write(ptmx, data, len, NULL) >= 0)
>> +	TST_CHECKPOINT_WAIT2(0, 100000);
>> +	while (max_writes-- && try_write(ptmx, data, len, NULL) >= 0)
>>  		;
>
> I wonder if we should change this to be time based instead. I.e. try to
> write packets for 10 seconds or so, since hardcoding number of
> iterations usually does not scale from embedded to supercomputers.

Writing is most likely far quicker than reading on any computer, so
limiting it by time worries me more. On a system that is very fast, but
highly contended (e.g. OpenQA guests), one CPU may be able to fill the
TTY buffer before another gets chance to start converting that data to
CAN packets. This will most likely result in a softlockup/timeout which
is what we are trying to avoid. With an iteration limit the fast system
will simply exit the loop before we get chance to read (if contended).

On a slow system which is also highly contended we just have to hope the
iteration limit is low enough. If we set a time limit instead, we still
have the issue of how many seconds to set before the TTY buffers are
full enough to queue up too much work.

I guess we could do two-way communication instead of just writing to the
PTY...

>
>>  	tst_res(TPASS, "Writing to PTY interrupted by hangup");
>
> And this should be true only if we do not run out of tries meanwhile,
> right?

Yes, I suppose we should not print that if the while loop finishes

>
>> @@ -331,7 +334,7 @@ static void read_netdev(const struct ldisc_info *ldisc)
>>  	check_data(ldisc, data, plen);
>>  	tst_res(TPASS, "Read netdev %s %d (2)", ldisc->name, sk);
>>  
>> -	TST_CHECKPOINT_WAKE(0);
>> +	TST_CHECKPOINT_WAKE2(0, 2);
>>  	while ((rlen = read(sk, data, plen)) > 0)
>>  		check_data(ldisc, data, rlen);
>>  
>> -- 
>> 2.28.0
>> 
>> 
>> -- 
>> Mailing list info: https://lists.linux.it/listinfo/ltp


-- 
Thank you,
Richard.

[LTP] [PATCH v2] pty04: Limit the number of packets sent to avoid timeout

[Richard Palethorpe]

At the end of the test we continuously write data to the PTY while
closing the PTY to check for races in the kernel. However if the
process which closes the PTY is delayed this can result in a very
large number of packets being created from the data written to the
PTY. It is easy to fill the PTY buffer with a large amount of data
which the kernel is slow to then parse into packets. This can result
in spurious softlockup warnings and test timeouts.

Theoretically the performance might be a concern for a fast enough
serial line, but this is not supposed to be a performance test.

So this commit limits the amount of data transmitted on the PTY by
waiting for the netdev to echo the data back. This has the added
benefit of testing data transmission in the opposite direction.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---

V2: Use two way communication to limit the writes.

 testcases/kernel/pty/pty04.c | 114 +++++++++++++++++++++++++++--------
 1 file changed, 89 insertions(+), 25 deletions(-)

diff --git a/testcases/kernel/pty/pty04.c b/testcases/kernel/pty/pty04.c
index 4adf2cbb7..3f3996829 100644
--- a/testcases/kernel/pty/pty04.c
+++ b/testcases/kernel/pty/pty04.c
@@ -133,21 +133,48 @@ static int open_pty(const struct ldisc_info *ldisc)
 	return set_ldisc(pts, ldisc);
 }
 
-static ssize_t try_write(int fd, const char *data,
-			 ssize_t size, ssize_t *written)
+static ssize_t try_async_write(int fd, const char *data, ssize_t size,
+			       ssize_t *done)
 {
-	ssize_t ret = write(fd, data, size);
+	ssize_t off = done ? *done : 0;
+	ssize_t ret = write(fd, data + off, size - off);
 
 	if (ret < 0)
 		return -(errno != EAGAIN);
 
-	return !written || (*written += ret) >= size;
+	if (!done)
+		return 1;
+
+	*done += ret;
+	return *done >= size;
+}
+
+static ssize_t try_async_read(int fd, char *data, ssize_t size,
+			      ssize_t *done)
+{
+	ssize_t off = done ? *done : 0;
+	ssize_t ret = read(fd, data + off, size - off);
+
+	if (ret < 0)
+		return -(errno != EAGAIN);
+
+	if (!done)
+		return 1;
+
+	*done += ret;
+	return *done >= size;
 }
 
-static void write_pty(const struct ldisc_info *ldisc)
+#define RETRY_ASYNC(fn) ({				      \
+	ssize_t done = 0;				      \
+	TST_RETRY_FUNC(try_async_##fn(ptmx, data, len, &done),\
+		       TST_RETVAL_NOTNULL);		      \
+})
+
+static void do_pty(const struct ldisc_info *ldisc)
 {
 	char *data;
-	ssize_t written, ret;
+	ssize_t ret;
 	size_t len = 0;
 
 	switch (ldisc->n) {
@@ -171,17 +198,12 @@ static void write_pty(const struct ldisc_info *ldisc)
 		break;
 	}
 
-
-	written = 0;
-	ret = TST_RETRY_FUNC(try_write(ptmx, data, len, &written),
-			     TST_RETVAL_NOTNULL);
+	ret = RETRY_ASYNC(write);
 	if (ret < 0)
 		tst_brk(TBROK | TERRNO, "Failed 1st write to PTY");
 	tst_res(TPASS, "Wrote PTY %s %d (1)", ldisc->name, ptmx);
 
-	written = 0;
-	ret = TST_RETRY_FUNC(try_write(ptmx, data, len, &written),
-			     TST_RETVAL_NOTNULL);
+	ret = RETRY_ASYNC(write);
 	if (ret < 0)
 		tst_brk(TBROK | TERRNO, "Failed 2nd write to PTY");
 
@@ -190,14 +212,23 @@ static void write_pty(const struct ldisc_info *ldisc)
 
 	tst_res(TPASS, "Wrote PTY %s %d (2)", ldisc->name, ptmx);
 
-	while (try_write(ptmx, data, len, NULL) >= 0)
+	ret = RETRY_ASYNC(read);
+	if (ret < 0)
+		tst_brk(TBROK | TERRNO, "Failed read of PTY");
+
+	tst_res(TPASS, "Read PTY %s %d", ldisc->name, ptmx);
+	TST_CHECKPOINT_WAKE(0);
+
+	while (RETRY_ASYNC(read) > -1 && RETRY_ASYNC(write) > -1)
 		;
 
-	tst_res(TPASS, "Writing to PTY interrupted by hangup");
+	tst_res(TPASS, "Transmission on PTY interrupted by hangup");
 
 	tst_free_all();
 }
 
+#undef RETRY_ASYNC
+
 static void open_netdev(const struct ldisc_info *ldisc)
 {
 	struct ifreq ifreq = { 0 };
@@ -288,7 +319,7 @@ static void check_data(const struct ldisc_info *ldisc,
 		tst_res(TINFO, "Will continue test without data checking");
 }
 
-static void try_read(int fd, char *data, ssize_t size)
+static ssize_t try_sync_read(int fd, char *data, ssize_t size)
 {
 	ssize_t ret, n = 0;
 	int retry = mtu;
@@ -297,13 +328,31 @@ static void try_read(int fd, char *data, ssize_t size)
 		ret = read(fd, data + n, size - n);
 
 		if (ret < 0)
-			break;
+			return ret;
 
 		if ((n += ret) >= size)
-			return;
+			return ret;
+	}
+
+	tst_brk(TBROK | TERRNO, "Only read %zd of %zd bytes", n, size);
+}
+
+static ssize_t try_sync_write(int fd, const char *data, ssize_t size)
+{
+	ssize_t ret, n = 0;
+	int retry = mtu;
+
+	while (retry--) {
+		ret = write(fd, data + n, size - n);
+
+		if (ret < 0)
+			return ret;
+
+		if ((n += ret) >= size)
+			return ret;
 	}
 
-	tst_brk(TBROK | TERRNO, "Read %zd of %zd bytes", n, size);
+	tst_brk(TBROK | TERRNO, "Only wrote %zd of %zd bytes", n, size);
 }
 
 static void read_netdev(const struct ldisc_info *ldisc)
@@ -323,19 +372,34 @@ static void read_netdev(const struct ldisc_info *ldisc)
 
 	tst_res(TINFO, "Reading from socket %d", sk);
 
-	try_read(sk, data, plen);
+	TEST(try_sync_read(sk, data, plen));
+	if (TST_RET < 0)
+		tst_brk(TBROK | TTERRNO, "Read netdev %s %d (1)", ldisc->name, sk);
 	check_data(ldisc, data, plen);
 	tst_res(TPASS, "Read netdev %s %d (1)", ldisc->name, sk);
 
-	try_read(sk, data, plen);
+	TEST(try_sync_read(sk, data, plen));
+	if (TST_RET < 0)
+		tst_brk(TBROK | TTERRNO, "Read netdev %s %d (2)", ldisc->name, sk);
 	check_data(ldisc, data, plen);
 	tst_res(TPASS, "Read netdev %s %d (2)", ldisc->name, sk);
 
-	TST_CHECKPOINT_WAKE(0);
-	while ((rlen = read(sk, data, plen)) > 0)
+	TEST(try_sync_write(sk, data, plen));
+	if (TST_RET < 0)
+		tst_brk(TBROK | TTERRNO, "Write netdev %s %d", ldisc->name, sk);
+
+	tst_res(TPASS, "Write netdev %s %d", ldisc->name, sk);
+
+	while (1) {
+		if (try_sync_write(sk, data, plen) < 0)
+			break;
+
+		if ((rlen = try_sync_read(sk, data, plen)) < 0)
+			break;
 		check_data(ldisc, data, rlen);
+	}
 
-	tst_res(TPASS, "Reading data from netdev interrupted by hangup");
+	tst_res(TPASS, "Data transmission on netdev interrupted by hangup");
 
 	close(sk);
 	tst_free_all();
@@ -356,7 +420,7 @@ static void do_test(unsigned int n)
 	}
 
 	if (!SAFE_FORK()) {
-		write_pty(ldisc);
+		do_pty(ldisc);
 		return;
 	}
 
-- 
2.29.1

[LTP] [PATCH v2] pty04: Limit the number of packets sent to avoid timeout

[Cyril Hrubis]

Hi!
> +static ssize_t try_async_write(int fd, const char *data, ssize_t size,
> +			       ssize_t *done)
>  {
> -	ssize_t ret = write(fd, data, size);
> +	ssize_t off = done ? *done : 0;
> +	ssize_t ret = write(fd, data + off, size - off);
>  
>  	if (ret < 0)
>  		return -(errno != EAGAIN);
>  
> -	return !written || (*written += ret) >= size;
> +	if (!done)
> +		return 1;
> +
> +	*done += ret;
> +	return *done >= size;
> +}
> +
> +static ssize_t try_async_read(int fd, char *data, ssize_t size,
> +			      ssize_t *done)
> +{
> +	ssize_t off = done ? *done : 0;
> +	ssize_t ret = read(fd, data + off, size - off);
> +
> +	if (ret < 0)
> +		return -(errno != EAGAIN);
> +
> +	if (!done)
> +		return 1;
> +
> +	*done += ret;
> +	return *done >= size;
>  }
>  
> -static void write_pty(const struct ldisc_info *ldisc)
> +#define RETRY_ASYNC(fn) ({				      \
> +	ssize_t done = 0;				      \
> +	TST_RETRY_FUNC(try_async_##fn(ptmx, data, len, &done),\
> +		       TST_RETVAL_NOTNULL);		      \
> +})

I do not like this macro that much. Maybe we can have two inline
functions here one for read and one for write.

> +static void do_pty(const struct ldisc_info *ldisc)
>  {
>  	char *data;
> -	ssize_t written, ret;
> +	ssize_t ret;
>  	size_t len = 0;
>  
>  	switch (ldisc->n) {
> @@ -171,17 +198,12 @@ static void write_pty(const struct ldisc_info *ldisc)
>  		break;
>  	}
>  
> -
> -	written = 0;
> -	ret = TST_RETRY_FUNC(try_write(ptmx, data, len, &written),
> -			     TST_RETVAL_NOTNULL);
> +	ret = RETRY_ASYNC(write);
>  	if (ret < 0)
>  		tst_brk(TBROK | TERRNO, "Failed 1st write to PTY");
>  	tst_res(TPASS, "Wrote PTY %s %d (1)", ldisc->name, ptmx);
>  
> -	written = 0;
> -	ret = TST_RETRY_FUNC(try_write(ptmx, data, len, &written),
> -			     TST_RETVAL_NOTNULL);
> +	ret = RETRY_ASYNC(write);
>  	if (ret < 0)
>  		tst_brk(TBROK | TERRNO, "Failed 2nd write to PTY");
>  
> @@ -190,14 +212,23 @@ static void write_pty(const struct ldisc_info *ldisc)
>  
>  	tst_res(TPASS, "Wrote PTY %s %d (2)", ldisc->name, ptmx);
>  
> -	while (try_write(ptmx, data, len, NULL) >= 0)
> +	ret = RETRY_ASYNC(read);
> +	if (ret < 0)
> +		tst_brk(TBROK | TERRNO, "Failed read of PTY");
> +
> +	tst_res(TPASS, "Read PTY %s %d", ldisc->name, ptmx);
> +	TST_CHECKPOINT_WAKE(0);
> +
> +	while (RETRY_ASYNC(read) > -1 && RETRY_ASYNC(write) > -1)
>  		;
>  
> -	tst_res(TPASS, "Writing to PTY interrupted by hangup");
> +	tst_res(TPASS, "Transmission on PTY interrupted by hangup");
>  
>  	tst_free_all();
>  }
>  
> +#undef RETRY_ASYNC
> +
>  static void open_netdev(const struct ldisc_info *ldisc)
>  {
>  	struct ifreq ifreq = { 0 };
> @@ -288,7 +319,7 @@ static void check_data(const struct ldisc_info *ldisc,
>  		tst_res(TINFO, "Will continue test without data checking");
>  }
>  
> -static void try_read(int fd, char *data, ssize_t size)
> +static ssize_t try_sync_read(int fd, char *data, ssize_t size)
>  {
>  	ssize_t ret, n = 0;
>  	int retry = mtu;
> @@ -297,13 +328,31 @@ static void try_read(int fd, char *data, ssize_t size)
>  		ret = read(fd, data + n, size - n);
>  
>  		if (ret < 0)
> -			break;
> +			return ret;
>  
>  		if ((n += ret) >= size)
> -			return;
> +			return ret;
> +	}
> +
> +	tst_brk(TBROK | TERRNO, "Only read %zd of %zd bytes", n, size);
> +}
> +
> +static ssize_t try_sync_write(int fd, const char *data, ssize_t size)
> +{
> +	ssize_t ret, n = 0;
> +	int retry = mtu;
> +
> +	while (retry--) {
> +		ret = write(fd, data + n, size - n);
> +
> +		if (ret < 0)
> +			return ret;
> +
> +		if ((n += ret) >= size)
> +			return ret;
>  	}
>  
> -	tst_brk(TBROK | TERRNO, "Read %zd of %zd bytes", n, size);
> +	tst_brk(TBROK | TERRNO, "Only wrote %zd of %zd bytes", n, size);
>  }
>  
>  static void read_netdev(const struct ldisc_info *ldisc)
> @@ -323,19 +372,34 @@ static void read_netdev(const struct ldisc_info *ldisc)
>  
>  	tst_res(TINFO, "Reading from socket %d", sk);
>  
> -	try_read(sk, data, plen);
> +	TEST(try_sync_read(sk, data, plen));
> +	if (TST_RET < 0)
> +		tst_brk(TBROK | TTERRNO, "Read netdev %s %d (1)", ldisc->name, sk);
>  	check_data(ldisc, data, plen);
>  	tst_res(TPASS, "Read netdev %s %d (1)", ldisc->name, sk);
>  
> -	try_read(sk, data, plen);
> +	TEST(try_sync_read(sk, data, plen));
> +	if (TST_RET < 0)
> +		tst_brk(TBROK | TTERRNO, "Read netdev %s %d (2)", ldisc->name, sk);
>  	check_data(ldisc, data, plen);
>  	tst_res(TPASS, "Read netdev %s %d (2)", ldisc->name, sk);
>  
> -	TST_CHECKPOINT_WAKE(0);
> -	while ((rlen = read(sk, data, plen)) > 0)
> +	TEST(try_sync_write(sk, data, plen));
> +	if (TST_RET < 0)
> +		tst_brk(TBROK | TTERRNO, "Write netdev %s %d", ldisc->name, sk);
> +
> +	tst_res(TPASS, "Write netdev %s %d", ldisc->name, sk);
> +
> +	while (1) {
> +		if (try_sync_write(sk, data, plen) < 0)
> +			break;
> +
> +		if ((rlen = try_sync_read(sk, data, plen)) < 0)
> +			break;
>  		check_data(ldisc, data, rlen);
> +	}
>  
> -	tst_res(TPASS, "Reading data from netdev interrupted by hangup");
> +	tst_res(TPASS, "Data transmission on netdev interrupted by hangup");
>  
>  	close(sk);
>  	tst_free_all();
> @@ -356,7 +420,7 @@ static void do_test(unsigned int n)
>  	}
>  
>  	if (!SAFE_FORK()) {
> -		write_pty(ldisc);
> +		do_pty(ldisc);
>  		return;
>  	}

So we do have one process that just reads and one that reads and writes
right? I wonder if that is okay, maybe we should write twice as much as
we read in the do_pty()?

Other than that it looks fine.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2] pty04: Limit the number of packets sent to avoid timeout

[Cyril Hrubis]

Hi!
Ping.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v3] pty04: Limit the number of packets sent to avoid timeout

[Richard Palethorpe]

At the end of the test we continuously write data to the PTY while
closing the PTY to check for races in the kernel. However if the
process which closes the PTY is delayed this can result in a very
large number of packets being created from the data written to the
PTY. It is easy to fill the PTY buffer with a large amount of data
which the kernel is slow to then parse into packets. This can result
in spurious softlockup warnings and test timeouts.

Theoretically the performance might be a concern for a fast enough
serial line, but this is not supposed to be a performance test.

So this commit limits the amount of data transmitted on the PTY by
waiting for the netdev to echo the data back. This has the added
benefit of testing data transmission in the opposite direction.

Signed-off-by: Richard Palethorpe <rpalethorpe@suse.com>
---

V3:
* Return after tst_brk
* Replace retry macro with inline functions

 testcases/kernel/pty/pty04.c | 135 ++++++++++++++++++++++++++++-------
 1 file changed, 109 insertions(+), 26 deletions(-)

diff --git a/testcases/kernel/pty/pty04.c b/testcases/kernel/pty/pty04.c
index 4adf2cbb7..e8f21f1d4 100644
--- a/testcases/kernel/pty/pty04.c
+++ b/testcases/kernel/pty/pty04.c
@@ -133,21 +133,58 @@ static int open_pty(const struct ldisc_info *ldisc)
 	return set_ldisc(pts, ldisc);
 }
 
-static ssize_t try_write(int fd, const char *data,
-			 ssize_t size, ssize_t *written)
+static ssize_t try_async_write(int fd, const char *data, ssize_t size,
+			       ssize_t *done)
 {
-	ssize_t ret = write(fd, data, size);
+	ssize_t off = done ? *done : 0;
+	ssize_t ret = write(fd, data + off, size - off);
 
 	if (ret < 0)
 		return -(errno != EAGAIN);
 
-	return !written || (*written += ret) >= size;
+	if (!done)
+		return 1;
+
+	*done += ret;
+	return *done >= size;
+}
+
+static ssize_t try_async_read(int fd, char *data, ssize_t size,
+			      ssize_t *done)
+{
+	ssize_t off = done ? *done : 0;
+	ssize_t ret = read(fd, data + off, size - off);
+
+	if (ret < 0)
+		return -(errno != EAGAIN);
+
+	if (!done)
+		return 1;
+
+	*done += ret;
+	return *done >= size;
+}
+
+static ssize_t retry_async_write(int fd, const char *data, ssize_t size)
+{
+	ssize_t done = 0;
+
+	return TST_RETRY_FUNC(try_async_write(fd, data, size, &done),
+			      TST_RETVAL_NOTNULL);
 }
 
-static void write_pty(const struct ldisc_info *ldisc)
+static ssize_t retry_async_read(int fd, char *data, ssize_t size)
+{
+	ssize_t done = 0;
+
+	return TST_RETRY_FUNC(try_async_read(fd, data, size, &done),
+			      TST_RETVAL_NOTNULL);
+}
+
+static void do_pty(const struct ldisc_info *ldisc)
 {
 	char *data;
-	ssize_t written, ret;
+	ssize_t ret;
 	size_t len = 0;
 
 	switch (ldisc->n) {
@@ -171,17 +208,12 @@ static void write_pty(const struct ldisc_info *ldisc)
 		break;
 	}
 
-
-	written = 0;
-	ret = TST_RETRY_FUNC(try_write(ptmx, data, len, &written),
-			     TST_RETVAL_NOTNULL);
+	ret = retry_async_write(ptmx, data, len);
 	if (ret < 0)
 		tst_brk(TBROK | TERRNO, "Failed 1st write to PTY");
 	tst_res(TPASS, "Wrote PTY %s %d (1)", ldisc->name, ptmx);
 
-	written = 0;
-	ret = TST_RETRY_FUNC(try_write(ptmx, data, len, &written),
-			     TST_RETVAL_NOTNULL);
+	ret = retry_async_write(ptmx, data, len);
 	if (ret < 0)
 		tst_brk(TBROK | TERRNO, "Failed 2nd write to PTY");
 
@@ -190,14 +222,28 @@ static void write_pty(const struct ldisc_info *ldisc)
 
 	tst_res(TPASS, "Wrote PTY %s %d (2)", ldisc->name, ptmx);
 
-	while (try_write(ptmx, data, len, NULL) >= 0)
-		;
+	ret = retry_async_read(ptmx, data, len);
+	if (ret < 0)
+		tst_brk(TBROK | TERRNO, "Failed read of PTY");
+
+	tst_res(TPASS, "Read PTY %s %d", ldisc->name, ptmx);
+	TST_CHECKPOINT_WAKE(0);
+
+	while (1) {
+		if (retry_async_read(ptmx, data, len) < 0)
+			break;
+
+		if (retry_async_write(ptmx, data, len) < 0)
+			break;
+	}
 
-	tst_res(TPASS, "Writing to PTY interrupted by hangup");
+	tst_res(TPASS, "Transmission on PTY interrupted by hangup");
 
 	tst_free_all();
 }
 
+#undef RETRY_ASYNC
+
 static void open_netdev(const struct ldisc_info *ldisc)
 {
 	struct ifreq ifreq = { 0 };
@@ -288,7 +334,7 @@ static void check_data(const struct ldisc_info *ldisc,
 		tst_res(TINFO, "Will continue test without data checking");
 }
 
-static void try_read(int fd, char *data, ssize_t size)
+static ssize_t try_sync_read(int fd, char *data, ssize_t size)
 {
 	ssize_t ret, n = 0;
 	int retry = mtu;
@@ -297,13 +343,35 @@ static void try_read(int fd, char *data, ssize_t size)
 		ret = read(fd, data + n, size - n);
 
 		if (ret < 0)
-			break;
+			return ret;
 
 		if ((n += ret) >= size)
-			return;
+			return ret;
+	}
+
+	tst_brk(TBROK | TERRNO, "Only read %zd of %zd bytes", n, size);
+
+	return n;
+}
+
+static ssize_t try_sync_write(int fd, const char *data, ssize_t size)
+{
+	ssize_t ret, n = 0;
+	int retry = mtu;
+
+	while (retry--) {
+		ret = write(fd, data + n, size - n);
+
+		if (ret < 0)
+			return ret;
+
+		if ((n += ret) >= size)
+			return ret;
 	}
 
-	tst_brk(TBROK | TERRNO, "Read %zd of %zd bytes", n, size);
+	tst_brk(TBROK | TERRNO, "Only wrote %zd of %zd bytes", n, size);
+
+	return n;
 }
 
 static void read_netdev(const struct ldisc_info *ldisc)
@@ -323,19 +391,34 @@ static void read_netdev(const struct ldisc_info *ldisc)
 
 	tst_res(TINFO, "Reading from socket %d", sk);
 
-	try_read(sk, data, plen);
+	TEST(try_sync_read(sk, data, plen));
+	if (TST_RET < 0)
+		tst_brk(TBROK | TTERRNO, "Read netdev %s %d (1)", ldisc->name, sk);
 	check_data(ldisc, data, plen);
 	tst_res(TPASS, "Read netdev %s %d (1)", ldisc->name, sk);
 
-	try_read(sk, data, plen);
+	TEST(try_sync_read(sk, data, plen));
+	if (TST_RET < 0)
+		tst_brk(TBROK | TTERRNO, "Read netdev %s %d (2)", ldisc->name, sk);
 	check_data(ldisc, data, plen);
 	tst_res(TPASS, "Read netdev %s %d (2)", ldisc->name, sk);
 
-	TST_CHECKPOINT_WAKE(0);
-	while ((rlen = read(sk, data, plen)) > 0)
+	TEST(try_sync_write(sk, data, plen));
+	if (TST_RET < 0)
+		tst_brk(TBROK | TTERRNO, "Write netdev %s %d", ldisc->name, sk);
+
+	tst_res(TPASS, "Write netdev %s %d", ldisc->name, sk);
+
+	while (1) {
+		if (try_sync_write(sk, data, plen) < 0)
+			break;
+
+		if ((rlen = try_sync_read(sk, data, plen)) < 0)
+			break;
 		check_data(ldisc, data, rlen);
+	}
 
-	tst_res(TPASS, "Reading data from netdev interrupted by hangup");
+	tst_res(TPASS, "Data transmission on netdev interrupted by hangup");
 
 	close(sk);
 	tst_free_all();
@@ -356,7 +439,7 @@ static void do_test(unsigned int n)
 	}
 
 	if (!SAFE_FORK()) {
-		write_pty(ldisc);
+		do_pty(ldisc);
 		return;
 	}
 
-- 
2.29.2

[LTP] [PATCH v3] pty04: Limit the number of packets sent to avoid timeout

[Cyril Hrubis]

Hi!
> -	tst_res(TPASS, "Writing to PTY interrupted by hangup");
> +	tst_res(TPASS, "Transmission on PTY interrupted by hangup");
>  
>  	tst_free_all();
>  }
>  
> +#undef RETRY_ASYNC

I've removed this now unused undef and pushed, thanks.

-- 
Cyril Hrubis
chrubis@suse.cz

[LTP] [PATCH v2] pty04: Limit the number of packets sent to avoid timeout

[Richard Palethorpe]

Hello,

Cyril Hrubis <chrubis@suse.cz> writes:

> Hi!
>> +static ssize_t try_async_write(int fd, const char *data, ssize_t size,
>> +			       ssize_t *done)
>>  {
>> -	ssize_t ret = write(fd, data, size);
>> +	ssize_t off = done ? *done : 0;
>> +	ssize_t ret = write(fd, data + off, size - off);
>>  
>>  	if (ret < 0)
>>  		return -(errno != EAGAIN);
>>  
>> -	return !written || (*written += ret) >= size;
>> +	if (!done)
>> +		return 1;
>> +
>> +	*done += ret;
>> +	return *done >= size;
>> +}
>> +
>> +static ssize_t try_async_read(int fd, char *data, ssize_t size,
>> +			      ssize_t *done)
>> +{
>> +	ssize_t off = done ? *done : 0;
>> +	ssize_t ret = read(fd, data + off, size - off);
>> +
>> +	if (ret < 0)
>> +		return -(errno != EAGAIN);
>> +
>> +	if (!done)
>> +		return 1;
>> +
>> +	*done += ret;
>> +	return *done >= size;
>>  }
>>  
>> -static void write_pty(const struct ldisc_info *ldisc)
>> +#define RETRY_ASYNC(fn) ({				      \
>> +	ssize_t done = 0;				      \
>> +	TST_RETRY_FUNC(try_async_##fn(ptmx, data, len, &done),\
>> +		       TST_RETVAL_NOTNULL);		      \
>> +})
>
> I do not like this macro that much. Maybe we can have two inline
> functions here one for read and one for write.

OK.

>
> So we do have one process that just reads and one that reads and writes
> right? I wonder if that is okay, maybe we should write twice as much as
> we read in the do_pty()?
>
> Other than that it looks fine.

They both read and write in the final loop. I will make this clearer in
the final while loop.

-- 
Thank you,
Richard.