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

[Maxime Coquelin <maxime.coquelin@redhat.com>]

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?

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