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

[Laurent Vivier <lvivier@redhat.com>]

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

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

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

Thanks,
Laurent