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

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

Hi Eelco,

On 4/3/23 16:51, Eelco Chaudron wrote:
> 
> 
> 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?
> 
> Hi Maxime, I prepped a patch, but not the read/write part yet, https://github.com/chaudron/dpdk/commit/d51c93b4ff08b43daed33e3c0fee193a6d039c25#.
> 
> I was thinking that even a read/write lock does not work (or maybe we need a combination of taking the read and write lock). As we need to update statistics, which need protection.
> For example here you see the split (with just two locks, but the sys call could be called in the read lock):
> 
> https://github.com/chaudron/dpdk/blob/d51c93b4ff08b43daed33e3c0fee193a6d039c25/lib/vhost/vhost.c#L1493
> 
> The best would be to not have a lock when calling the system call, but that does not seem safe. I do not see a clear solution, guess also as I have some trouble understanding the lock rules around vq’s.

The lock is indeed required. Maybe we can use read-lock, and use atomic
operations for counters that could be accessed by several threads?

> 
> Do you have some more insights to share? I can ping you offline and discuss this.

Sure, I'll be happy to discuss more about this.

Thanks,
Maxime

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