Re: [PATCH] tap: Send frames after the first one in tap_send_frames_pasta()

On Mon, 13 Feb 2023 13:24:58 +1100
David Gibson <david(a)gibson.dropbear.id.au> wrote:

...
  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(a)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(a)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. 
Um, yes. I try, whenever possible, to use just one "iterator", which
would be iov, but the price of doing that "cleanly" here is wasting a
struct iovec just to have a zero iov_len at the end, which makes little
sense.

...
  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].' 
That would also be consistent with tap_send_frames_passt(), so sure,
let's change it. I can submit a patch too.

-- 
Stefano