Re: [PATCH v3 06/18] tcp: Combine two parts of pasta tap send path together

[Stefano Brivio <sbrivio@redhat.com>]

On Fri,  6 Jan 2023 11:43:10 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

> tcp_l2_buf_flush() open codes the loop across each frame in a group, but
> but calls tcp_l2_buf_write_one() to send each frame to the pasta tuntap
> device.  Combine these two pasta-specific operations into
> tcp_l2_buf_flush_pasta() which is a little cleaner and will enable further
> cleanups.
> 
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  tcp.c | 40 ++++++++++++++++++----------------------
>  1 file changed, 18 insertions(+), 22 deletions(-)
> 
> diff --git a/tcp.c b/tcp.c
> index d96122d..9960a35 100644
> --- a/tcp.c
> +++ b/tcp.c
> @@ -1391,23 +1391,25 @@ static void tcp_rst_do(struct ctx *c, struct tcp_tap_conn *conn);
>  	} while (0)
>  
>  /**
> - * tcp_l2_buf_write_one() - Write a single buffer to tap file descriptor
> + * tcp_l2_buf_flush_pasta() - Send frames on the pasta tap interface
>   * @c:		Execution context
> - * @iov:	struct iovec item pointing to buffer
> - * @ts:		Current timestamp
> - *
> - * Return: 0 on success, negative error code on failure (tap reset possible)
> + * @iov:	Pointer to array of buffers, one per frame
> + * @n:		Number of buffers/frames to flush
>   */
> -static int tcp_l2_buf_write_one(struct ctx *c, const struct iovec *iov)
> +static void tcp_l2_buf_flush_pasta(struct ctx *c,
> +				  const struct iovec *iov, size_t n)
>  {
> -	if (write(c->fd_tap, (char *)iov->iov_base + 4, iov->iov_len - 4) < 0) {
> -		debug("tap write: %s", strerror(errno));
> -		if (errno != EAGAIN && errno != EWOULDBLOCK)
> -			tap_handler(c, c->fd_tap, EPOLLERR, NULL);
> -		return -errno;
> -	}
> +	size_t i;
>  
> -	return 0;
> +	for (i = 0; i < n; i++) {
> +		if (write(c->fd_tap, (char *)iov->iov_base + 4,
> +			  iov->iov_len - 4) < 0) {

It took me a moment to miss this during review, but a very long time to
figure out later. :( This always sends the first frame in 'iov'.

Surprisingly, pasta_tcp performance tests work just fine most of the
times: for data connections we usually end up moving a single frame at
a time, and retransmissions hide the issue for control messages.

I just posted a patch on top of this, you don't have to respin, and
it's actually more convenient for me to apply this with a fix at this
point.

-- 
Stefano