On Mon, Feb 13, 2023 at 02:12:11AM +0100, Stefano Brivio wrote:
> ...instead of repeatedly sending out the first one in iov.
> 
> Fixes: e21ee41ac35a ("tcp: Combine two parts of pasta tap send path together")
> Signed-off-by: Stefano Brivio <sbrivio@redhat.com>
> ---
> I just applied this, to unblock a series by David which was pending
> for way too long. The commit reference in Fixes: refers to a commit
> from said series which I'm pushing out together with this patch.

Huh... how did this ever work even slightly.  From that point of view,

Reviewed-by: David Gibson <david@gibson.dropbear.id.au>

> Posting anyway for reviews.

That said..

> 
>  tap.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tap.c b/tap.c
> index af9bc15..716d887 100644
> --- a/tap.c
> +++ b/tap.c
> @@ -316,12 +316,13 @@ static void tap_send_frames_pasta(struct ctx *c,
>  {
>  	size_t i;
>  
> -	for (i = 0; i < n; i++) {
> +	for (i = 0; i < n; i++, iov++) {

I quite dislike having multiple "counters" that need to be updated for
each loop iteration (manual strength reduction.  It's really easy to
make a mistake in later changes and let the two values get out of sync
- which is exactly what I did with the earlier change that introduced
this bug.

W.r.t. performance, I generally trust the compiler's automatic
strength reduction to have a better idea of whether it will be worth
it or not than my own guess.

>  		if (write(c->fd_tap, (char *)iov->iov_base, iov->iov_len) < 0) {

So, my *intention* on the older patch was to replace 'iov->' above
with 'iov[i].'

>  			debug("tap write: %s", strerror(errno));
>  			if (errno != EAGAIN && errno != EWOULDBLOCK)
>  				tap_handler(c, c->fd_tap, EPOLLERR, NULL);
>  			i--;
> +			iov--;
>  		}
>  	}
>  }

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson