Re: [RFC v3 8/8] tap: Implement pasta vhost TX

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

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

On Wed, Aug 12, 2026 at 09:45:12PM +0300, Ammar Yasser wrote:
> On Mon Aug 10, 2026 at 12:18 PM EEST, David Gibson wrote:
> > On Sun, Aug 02, 2026 at 01:21:55PM +0000, Ammar Yasser wrote:
> >> Callers will specify whether vhost should be used or no by passing the
> >> vhost argument to tap_send_frames_pasta. If specified, sending will go
> >> through a function called tap_send_frames_vhost, which pops a descriptor
> >> from the queue shared with the kernel and sets the address of that
> >> descriptor to be the base address of a single iov from the group of
> >>  buffers_per_frame * nframes iovs we pass to the function
> >> 
> >> Signed-off-by: Ammar Yasser <aerosound161@gmail.com>
> >
> > Sorry, didn't spot this earlier in the series: if these are based on
> > Eugenio's earlier patches, they should have his Signed-off-by in
> > addition to your own.
> 
> They have significantly diverged in both order and structure so there's
> no clear mapping between mine and his anymore.

That's ok.  S-o-b isn't really about tracing the origin of any single
patch, but about the developer's certificate of origin (see
CONTRIBUTING.md).  By adding your S-o-b you're asserting the things it
says, but if the code is baed on Eugenio's work, even significantly
rearranged, then to assert that you're relying on _his_ assertion of
the DCO.  That's what having both S-o-b lines is documenting.

> if there doesn't need to
> be a mapping i'm happy to add his Signed-off-by. Otherwise, i did
> mention his original series in the cover letter
> 
> >
> >> ---
> >>  tap.c | 108 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> >>  1 file changed, 107 insertions(+), 1 deletion(-)
> >> 
> >> diff --git a/tap.c b/tap.c
> >> index e177eef..66bb71d 100644
> >> --- a/tap.c
> >> +++ b/tap.c
> >> @@ -361,7 +361,32 @@ void tap_icmp6_send(const struct ctx *c,
> >>  }
> >>  
> >>  /**
> >> - * tap_send_frames_pasta() - Send multiple frames to the pasta tap
> >> + * tx_reap() - Reclaim the descriptors the kernel has already processed
> >> + */
> >> +static void tx_reap(void) {
> >> +	struct vring_used *used = &vring_used_all[1].used;
> >> +	uint16_t used_idx = le16toh(used->idx);
> >> +
> >> +	smp_rmb();
> >> +
> >> +	/* increment last_used_idx until it reaches the kernel's used index */
> >> +	while (vqs[1].last_used_idx != used_idx)  {
> >> +		uint16_t desc_id = le32toh(used->ring[vqs[1].last_used_idx % VHOST_NDESCS].id);
> >> +		
> >> +		for (;;) {
> >> +			/* keep going until we find a descriptor without the next flag */
> >> +			vqs[1].num_free++;
> >> +			if (!(le16toh(vring_desc[1][desc_id].flags) & VRING_DESC_F_NEXT))
> >> +				break;
> >> +			/* this descriptor wasn't the last, set desc_id to the next one and keep going */
> >> +			desc_id = le16toh(vring_desc[1][desc_id].next);
> >> +		}
> >> +		vqs[1].last_used_idx++;
> >> +	}
> >> +}
> >> +
> >> +/**
> >> + * tap_send_frames_vhost() - Send multiple frames to the pasta tap
> >>   * @c:			Execution context
> >>   * @iov:		Array of buffers
> >>   * @bufs_per_frame:	Number of buffers (iovec entries) per frame
> >> @@ -371,6 +396,84 @@ void tap_icmp6_send(const struct ctx *c,
> >>   * @bufs_per_frame contiguous buffers representing a single frame.
> >>   *
> >>   * Return: number of frames successfully sent
> >> + */
> >> +static size_t tap_send_frames_vhost(const struct ctx *c,
> >> +				    const struct iovec *iov,
> >> +				    size_t bufs_per_frame, size_t nframes)
> >> +{
> >> +	size_t i;
> >> +	size_t processed_frames = 0;
> >
> > Reverse christmas tree.
> 
> Noted
> 
> >
> >> +
> >> +	/* update our local counters first */
> >> +	tx_reap();
> >> +
> >> +	#define AVAIL_Q(i)(vring_avail_all[i].avail)
> >
> > # for preprocessor directives should always go in column 0.
> 
> Ok
> 
> >
> >> +
> >> +	for (i = 0; i < nframes; i++) {
> >> +		size_t j;
> >> +
> >> +		if (vqs[1].num_free < bufs_per_frame)
> >> +			break;
> >
> > IIRC, we were discussing during the last call that at least for the
> > first version it's fine if we synchronously wait for buffers to be
> > available.  Since then it occurred to me that another acceptable
> > option would be to simply drop frames if there are no available
> > buffers (this is kind of how IP is designed to work - congestion is
> > reported implicitly as packet loss).  Do whichever is easier for now
> > and it can be refined later.
> 
> Will do
> 
> >
> >> +
> >> +		/* set the index of the avail ring in the tx queue to be our last_used_idx */
> >> +		uint16_t head = vqs[1].next_free % VHOST_NDESCS;
> >
> > No inline decls.
> 
> Ok
> 

-- 
David Gibson (he or they)	| 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 --]