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.
