Re: [PATCH v3 4/6] vhost-user: Add queue pair parameter throughout the network stack

[Stefano Brivio <sbrivio@redhat.com>]

On Thu, 11 Dec 2025 14:26:01 +0100
Laurent Vivier <lvivier@redhat.com> wrote:

> On 12/11/25 13:16, Stefano Brivio wrote:
> > On Thu, 11 Dec 2025 09:48:42 +0100
> > Laurent Vivier <lvivier@redhat.com> wrote:
> >   
> >> On 12/11/25 08:01, Stefano Brivio wrote:  
> >>> On Wed,  3 Dec 2025 19:54:32 +0100
> >>> Laurent Vivier <lvivier@redhat.com> wrote:
> >>>      
> >>>> diff --git a/vu_common.c b/vu_common.c
> >>>> index b13b7c308fd8..80d9a30f6f71 100644
> >>>> --- a/vu_common.c
> >>>> +++ b/vu_common.c
> >>>> @@ -196,11 +196,11 @@ static void vu_handle_tx(struct vu_dev *vdev, int index,
> >>>>    
> >>>>    		data = IOV_TAIL(elem[count].out_sg, elem[count].out_num, 0);
> >>>>    		if (IOV_DROP_HEADER(&data, struct virtio_net_hdr_mrg_rxbuf))
> >>>> -			tap_add_packet(vdev->context, &data, now);
> >>>> +			tap_add_packet(vdev->context, 0, &data, now);
> >>>>    
> >>>>    		count++;
> >>>>    	}
> >>>> -	tap_handler(vdev->context, now);
> >>>> +	tap_handler(vdev->context, 0, now);
> >>>>    
> >>>>    	if (count) {
> >>>>    		int i;
> >>>> @@ -235,23 +235,26 @@ void vu_kick_cb(struct vu_dev *vdev, union epoll_ref ref,
> >>>>    }
> >>>>    
> >>>>    /**
> >>>> - * vu_send_single() - Send a buffer to the front-end using the RX virtqueue
> >>>> + * vu_send_single() - Send a buffer to the front-end using a specified virtqueue
> >>>>     * @c:		execution context
> >>>> + * @qpair:	Queue pair on which to send the buffer
> >>>>     * @buf:	address of the buffer
> >>>>     * @size:	size of the buffer
> >>>>     *
> >>>>     * Return: number of bytes sent, -1 if there is an error
> >>>>     */
> >>>> -int vu_send_single(const struct ctx *c, const void *buf, size_t size)
> >>>> +int vu_send_single(const struct ctx *c, unsigned int qpair, const void *buf, size_t size)
> >>>>    {
> >>>>    	struct vu_dev *vdev = c->vdev;
> >>>> -	struct vu_virtq *vq = &vdev->vq[VHOST_USER_RX_QUEUE];
> >>>>    	struct vu_virtq_element elem[VIRTQUEUE_MAX_SIZE];
> >>>>    	struct iovec in_sg[VIRTQUEUE_MAX_SIZE];
> >>>> +	struct vu_virtq *vq;
> >>>>    	size_t total;
> >>>>    	int elem_cnt;
> >>>>    	int i;
> >>>>    
> >>>> +	vq = &vdev->vq[qpair << 1];  
> >>>
> >>> << 1 instead of * 2 is a bit surprising here, for a few seconds I
> >>> thought you swapped qpair and 1.
> >>>
> >>> Then I started thinking that somebody is likely to mix up (probably not
> >>> you) indices of RX and TX queues at some point. So... what about some
> >>> macros, say (let's see if I got it right this time):
> >>>
> >>> #define VHOST_SEND_QUEUE(pair)	((pair) * 2)
> >>> #define VHOST_RECV_QUEUE(pair)	(pair)  
> >>
> >> I will. David had the same comment.  
> > 
> > Uh, wait, I must have missed it. Do you have a Message-ID? I'm afraid I
> > must have missed some emails here but I don't see them in archives
> > either...  
> 
> Message-ID: aRF1_Qj6uxf1ndiA@zatzit

Ah, yes, I read that, but I didn't relate it to this topic as it was
just about the direction / naming. I see now.

> >> TX and RX are from the point of view of guest, it's
> >> not obvious when we read passt code.  
> > 
> > Right, yes, for me neither, I always get confused. That's why I thought
> > we could make the RX vhost-user queue become "SEND" in passt's code,
> > but:
> >   
> >> I would prefer as David proposed to use, i.e. FROMGUEST and TOGUEST:
> >>
> >> #define VHOST_FROM_GUEST(qpair) ((qpair) * 2 + 1)
> >> #define VHOST_TO_GUEST(qpair)   ((qpair) * 2)  
> > 
> > ...this is even clearer. It misses the QUEUE though. Does
> > VHOST_QUEUE_{FROM,TO}_GUEST fit where you use it? Otherwise I guess VQ
> > together with FROM / TO should be clear enough.
> >   
> >>> and:
> >>>
> >>> #define VHOST_QUEUE_PAIR(q)	((q) % 2) ? (q) : (q) / 2)  
> >>
> >> I don't undestand the purpose of this one.  
> > 
> > To get the pair number from a queue number. You're doing something like
> > that (I guess?) in 5/6, vu_handle_tx():
> > 
> > +	tap_flush_pools(index / 2);
> > 
> > +			tap_add_packet(vdev->context, index / 2, &data, now);
> > 
> > +	tap_handler(vdev->context, index / 2, now);
> > 
> > but now that I see your definition for VHOST_FROM_GUEST() above, and
> > that the purpose wasn't clear to you, I guess it should be:
> > 
> > #define VHOST_PAIR_FROM_QUEUE(q) (((q) % 2) ? ((q) - 1 / 2) : ((q) / 2))
> >   
> 
> Why not simply:
> 
> #define VHOST_PAIR_FROM_QUEUE(q) (q / 2)
> 
> QUEUES 0,1 -> QP 0
> QUEUES 2,3 -> QP 1

Ah, right, of course. Don't forget the parentheses around 'q'.

> > ...or maybe it's not needed? I'm not sure.
> >   
> >>>
> >>> ...are they correct? A short description or "Theory of operation"
> >>> section somewhere with a recap of how queue indices are used would be
> >>> nice to have.
> >>>
> >>> And maybe also something explaining that 0 that's now appearing in
> >>> argument lists:
> >>>
> >>> #define VHOST_NO_QUEUE		0  
> >>
> >> It's not really NO_QUEUE, it's default queue pair, the queue pair 0  
> > 
> > Hmm but for non-vhost-user usages then it's not a queue, right? Well,  
> For non vhost usage we can say there is only one queue.
> 
> > whatever, as long as we have a definition for it... or maybe we could
> > have VHOST_QUEUE_DEFAULT and NO_VHOST_QUEUE or VHOST_NO_QUEUE all being
> > 0?
> >   
> 
> Perhaps we could instead use a generic naming:
> 
> QPAIR_DEFAULT
> QUEUE_FROM_GUEST(qpair)
> QUEUE_TO_GUEST(qpair)

...but for non vhost-user we would have QUEUE_FROM_GUEST(QPAIR_DEFAULT)
evaluating to 1 which isn't correct I guess?

In general it looks reasonable to me, I would just like to make sure we
avoid passing around a '0' in the non-vhost-user case which would look
rather obscure.

-- 
Stefano