Re: [EXT] [PATCH] vhost: add device op to offload the interrupt kick

[Eelco Chaudron <echaudro@redhat.com>]

On 27 Mar 2023, at 18:35, Maxime Coquelin wrote:

> On 3/27/23 18:04, Eelco Chaudron wrote:
>>
>>
>> On 27 Mar 2023, at 17:16, Gowrishankar Muthukrishnan wrote:
>>
>>> Hi Eelco,
>>>
>>>> +void
>>>> +rte_vhost_notify_guest(int vid, uint16_t queue_id) {
>>>> +	struct virtio_net *dev = get_device(vid);
>>>> +	struct vhost_virtqueue *vq;
>>>> +
>>>> +	if (!dev ||  queue_id >= VHOST_MAX_VRING)
>>>> +		return;
>>>> +
>>>> +	vq = dev->virtqueue[queue_id];
>>>> +	if (!vq)
>>>> +		return;
>>>> +
>>>> +	rte_spinlock_lock(&vq->access_lock);
>>>> +
>>>
>>> Is spin lock needed here before system call ?
>>
>> I assumed access_lock is protecting all the following fields in this structure, so I need the lock to read the vq->callfd, however, I can/should move the eventfd_write outside of the lock.
>
> The FD might be closed between the check and the call to eventfd_write
> though, but I agree this is not optimal to call the eventfd_write under
> the spinlock in your case, as you will block the pmd thread if it tries
> to enqueue/dequeue packets on this queue, defeating the purpose of this
> patch.
>
> Maybe the solution is to change to read-write locks for the access_lock
> spinlock. The datapath (rte_vhost_enqueue_burst/rte_vhost_dequeue_burst)
> and this API would use the read version, meaning they won't lock each
> other, and the control path (lib/vhost/vhost_user.c) will use the write
> version.
>
> Does that make sense?

Yes, this makes sense, let me investigate this and get back.

>>>> +	if (vq->callfd >= 0)
>>>> +		eventfd_write(vq->callfd, (eventfd_t)1);
>>>> +
>>>> +	rte_spinlock_unlock(&vq->access_lock);
>>>> +}
>>>> +
>>>
>>> Thanks.
>>