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

[Laurent Vivier <lvivier@redhat.com>]

On 12/11/25 16:27, Stefano Brivio wrote:
> 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'.

Noted :)

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

Yes, _but_ it can map it to what it wants. For instance, for non vhost-user, there will be 
only qpair #0, and QUEUE_FROM_GUEST(QPAIR_DEFAULT) can be mapped to "read from the tap 
socket" and QUEUE_TO_GUEST(QPAIR_DEFAULT) can be mapped to "write to the tap socket".

In fact, in the threading part, one thread will manage one queue pair, and each action is 
mapped to the expected queue.

> 
> 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.
> 
I totally agree and I'm reviewing my code to avoid that.

Thanks,
Laurent