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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 2775 bytes --]

On Mon, Feb 13, 2023 at 11:46:09AM +0100, Stefano Brivio wrote:
> On Mon, 13 Feb 2023 13:24:58 +1100
> David Gibson <david@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@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.
> 
> 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.

Right.. I mean it's nice when you can use the pointer/object itself as
the iterator.  But in C, its pretty common for that to get awkward, so
I was conciously switching these from the iterator being 'iov' to the
iterator being 'i'.

> > 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.
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]