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

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(-)

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.

Hi!
Ping.

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.