[dpdk-dev,1/2] vhost: fix deadlock on IOTLB miss

Message ID 20171012153850.21837-2-maxime.coquelin@redhat.com (mailing list archive)
State Accepted, archived
Delegated to: Yuanhan Liu
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Maxime Coquelin Oct. 12, 2017, 3:38 p.m. UTC
  An optimization was done to only take the iotlb cache lock
once per packet burst instead of once per IOVA translation.

With this, IOTLB miss requests are sent to Qemu with the lock
held, which can cause a deadlock if the socket buffer is full,
and if Qemu is waiting for an IOTLB update to be done.

Holding the lock is not necessary when sending an IOTLB miss
request, as it is not manipulating the IOTLB cache list, which
the lock protects. Let's just release it while sending the
IOTLB miss.

Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/librte_vhost/vhost.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)
  

Comments

Jens Freimann Oct. 13, 2017, 11:32 a.m. UTC | #1
On Thu, Oct 12, 2017 at 03:38:49PM +0000, Maxime Coquelin wrote:
>An optimization was done to only take the iotlb cache lock
>once per packet burst instead of once per IOVA translation.
>
>With this, IOTLB miss requests are sent to Qemu with the lock
>held, which can cause a deadlock if the socket buffer is full,
>and if Qemu is waiting for an IOTLB update to be done.
>
>Holding the lock is not necessary when sending an IOTLB miss
>request, as it is not manipulating the IOTLB cache list, which
>the lock protects. Let's just release it while sending the
>IOTLB miss.
>
>Signed-off-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>---
> lib/librte_vhost/vhost.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>

Seems to be safe, because in case of an IOTLB miss we only take a
different lock. 

Reviewed-by: Jens Freimann <jfreimann@redhat.com>
  

Patch

diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
index 54a1864eb..4f8b73a09 100644
--- a/lib/librte_vhost/vhost.c
+++ b/lib/librte_vhost/vhost.c
@@ -55,6 +55,7 @@ 
 
 struct virtio_net *vhost_devices[MAX_VHOST_DEVICE];
 
+/* Called with iotlb_lock read-locked */
 uint64_t
 __vhost_iova_to_vva(struct virtio_net *dev, struct vhost_virtqueue *vq,
 		    uint64_t iova, uint64_t size, uint8_t perm)
@@ -71,8 +72,19 @@  __vhost_iova_to_vva(struct virtio_net *dev, struct vhost_virtqueue *vq,
 		return vva;
 
 	if (!vhost_user_iotlb_pending_miss(vq, iova + tmp_size, perm)) {
+		/*
+		 * iotlb_lock is read-locked for a full burst,
+		 * but it only protects the iotlb cache.
+		 * In case of IOTLB miss, we might block on the socket,
+		 * which could cause a deadlock with QEMU if an IOTLB update
+		 * is being handled. We can safely unlock here to avoid it.
+		 */
+		vhost_user_iotlb_rd_unlock(vq);
+
 		vhost_user_iotlb_pending_insert(vq, iova + tmp_size, perm);
 		vhost_user_iotlb_miss(dev, iova + tmp_size, perm);
+
+		vhost_user_iotlb_rd_lock(vq);
 	}
 
 	return 0;