Re: [PATCH 1/4] tap: Extend tap_send_frames() to allow multi-buffer frames

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

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

On Thu, Mar 14, 2024 at 08:02:17AM +0100, Stefano Brivio wrote:
> On Fri,  8 Mar 2024 17:53:22 +1100
> David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> > tap_send_frames() takes a vector of buffers and requires exactly one frame
> > per buffer.  We have future plans where we want to have multiple buffers
> > per frame in some circumstances, so extend tap_send_frames() to take the
> > number of buffers per frame as a parameter.
> > 
> > Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> > ---
> >  tap.c | 83 +++++++++++++++++++++++++++++++++++++----------------------
> >  tap.h |  3 ++-
> >  tcp.c |  8 +++---
> >  udp.c |  2 +-
> >  4 files changed, 59 insertions(+), 37 deletions(-)
> > 
> > diff --git a/tap.c b/tap.c
> > index f4051cec..f9e2a8d9 100644
> > --- a/tap.c
> > +++ b/tap.c
> > @@ -309,21 +309,28 @@ void tap_icmp6_send(const struct ctx *c,
> >  
> >  /**
> >   * tap_send_frames_pasta() - Send multiple frames to the pasta tap
> > - * @c:		Execution context
> > - * @iov:	Array of buffers, each containing one frame
> > - * @n:		Number of buffers/frames in @iov
> > + * @c:			Execution context
> > + * @iov:		Array of buffers
> > + * @bufs_per_frame:	Number of buffers (iovec entries) per frame
> > + * @nframes:		Number of frames to send
> >   *
> > + * @iov must have total length @bufs_per_frame * @nframes, with each set of
> > + * @bufs_per_frame contiguous buffers representing a single frame.
> 
> Oh, this does pretty much what I was suggesting as a comment to
> Laurent's "tcp: Replace TCP buffer structure by an iovec array" -- I
> should have reviewed this first.

Right.

> > + * 
> >   * Return: number of frames successfully sent
> >   *
> >   * #syscalls:pasta write
> >   */
> >  static size_t tap_send_frames_pasta(const struct ctx *c,
> > -				    const struct iovec *iov, size_t n)
> > +				    const struct iovec *iov,
> > +				    size_t bufs_per_frame, size_t nframes)
> >  {
> > +	size_t nbufs = bufs_per_frame * nframes;
> >  	size_t i;
> >  
> > -	for (i = 0; i < n; i++) {
> > -		ssize_t rc = write(c->fd_tap, iov[i].iov_base, iov[i].iov_len);
> > +	for (i = 0; i < nbufs; i += bufs_per_frame) {
> > +		ssize_t rc = writev(c->fd_tap, iov + i, bufs_per_frame);
> > +		size_t framelen = iov_size(iov + i, bufs_per_frame);
> >  
> >  		if (rc < 0) {
> >  			debug("tap write: %s", strerror(errno));
> > @@ -340,32 +347,37 @@ static size_t tap_send_frames_pasta(const struct ctx *c,
> >  			default:
> >  				die("Write error on tap device, exiting");
> >  			}
> > -		} else if ((size_t)rc < iov[i].iov_len) {
> > -			debug("short write on tuntap: %zd/%zu",
> > -			      rc, iov[i].iov_len);
> > +		} else if ((size_t)rc < framelen) {
> > +			debug("short write on tuntap: %zd/%zu", rc, framelen);
> >  			break;
> >  		}
> >  	}
> >  
> > -	return i;
> > +	return i / bufs_per_frame;
> >  }
> >  
> >  /**
> >   * tap_send_frames_passt() - Send multiple frames to the passt tap
> > - * @c:		Execution context
> > - * @iov:	Array of buffers, each containing one frame
> > - * @n:		Number of buffers/frames in @iov
> > + * @c:			Execution context
> > + * @iov:		Array of buffers, each containing one frame
> > + * @bufs_per_frame:	Number of buffers (iovec entries) per frame
> > + * @nframes:		Number of frames to send
> >   *
> > + * @iov must have total length @bufs_per_frame * @nframes, with each set of
> > + * @bufs_per_frame contiguous buffers representing a single frame.
> > + * 
> >   * Return: number of frames successfully sent
> >   *
> >   * #syscalls:passt sendmsg
> >   */
> >  static size_t tap_send_frames_passt(const struct ctx *c,
> > -				    const struct iovec *iov, size_t n)
> > +				    const struct iovec *iov,
> > +				    size_t bufs_per_frame, size_t nframes)
> >  {
> > +	size_t nbufs = bufs_per_frame * nframes;
> >  	struct msghdr mh = {
> >  		.msg_iov = (void *)iov,
> > -		.msg_iovlen = n,
> > +		.msg_iovlen = nbufs,
> >  	};
> >  	size_t buf_offset;
> >  	unsigned int i;
> > @@ -376,44 +388,53 @@ static size_t tap_send_frames_passt(const struct ctx *c,
> >  		return 0;
> >  
> >  	/* Check for any partial frames due to short send */
> > -	i = iov_skip_bytes(iov, n, sent, &buf_offset);
> > +	i = iov_skip_bytes(iov, nbufs, sent, &buf_offset);
> > +
> > +	if (i < nbufs && (buf_offset || (i % bufs_per_frame))) {
> > +		/* Number of not-fully-sent buffers in the frame */
> 
> Strictly speaking, this comment is correct, but "not-fully-sent" seems
> to imply that rembufs only counts partially sent buffers. It also
> counts the ones that weren't sent at all. What about:
> 
> 		/* Number of partially sent or not sent buffers for the frame */
> 
> ?

Yeah, the original is technically correct, but easy to misread.  Maybe
	Number of unsent or partially sent buffers for the frame
for slightly more brevity than your suggestion?
	

> The rest of the series looks good to me, I can change this text on
> merge if you like the proposal, or even apply it as it is (well, it's
> correct, after all).

Yes, please adjust and go ahead.

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