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

["Ammar Yasser" <aerosound161@gmail.com>]

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