diff mbox series

[v6,3/9] vhost: terminate when access lock is not taken

Message ID	20230207104532.2370869-4-david.marchand@redhat.com (mailing list archive)
State	Accepted, archived
Delegated to:	Thomas Monjalon
Headers	From: David Marchand <david.marchand@redhat.com> To: dev@dpdk.org Cc: maxime.coquelin@redhat.com, stephen@networkplumber.org, chenbo.xia@intel.com, jiayu.hu@intel.com, yuanx.wang@intel.com, xuan.ding@intel.com, mb@smartsharesystems.com Subject: [PATCH v6 3/9] vhost: terminate when access lock is not taken Date: Tue, 7 Feb 2023 11:45:26 +0100 Message-Id: <20230207104532.2370869-4-david.marchand@redhat.com> In-Reply-To: <20230207104532.2370869-1-david.marchand@redhat.com> References: <20220328121758.26632-1-david.marchand@redhat.com> <20230207104532.2370869-1-david.marchand@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Precedence: list Errors-To: dev-bounces@dpdk.org
Series	Lock annotations \| [v6,0/9] Lock annotations [v6,1/9] eal: annotate spinlock, rwlock and seqlock [v6,2/9] vhost: simplify need reply handling [v6,3/9] vhost: terminate when access lock is not taken [v6,4/9] vhost: annotate virtqueue access lock [v6,5/9] vhost: annotate async accesses [v6,6/9] vhost: always take IOTLB lock [v6,7/9] vhost: annotate IOTLB lock [v6,8/9] vhost: annotate vDPA device list accesses [v6,9/9] vhost: enable lock check

Checks

Context	Check	Description
ci/checkpatch	warning	coding style issues

Commit Message

David Marchand Feb. 7, 2023, 10:45 a.m. UTC

  Be a bit more strict when a programmatic error is detected with regards to
the access_lock not being taken.
Mark the new helper with __rte_assert_exclusive_lock so that clang
understands where locks are expected to be taken.

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
---
 lib/vhost/vhost.c      | 18 +++---------------
 lib/vhost/vhost.h      | 10 ++++++++++
 lib/vhost/virtio_net.c |  6 +-----
 3 files changed, 14 insertions(+), 20 deletions(-)

Comments

Chenbo Xia Feb. 9, 2023, 8:01 a.m. UTC | #1

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Tuesday, February 7, 2023 6:45 PM
> To: dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; stephen@networkplumber.org; Xia, Chenbo
> <chenbo.xia@intel.com>; Hu, Jiayu <jiayu.hu@intel.com>; Wang, YuanX
> <yuanx.wang@intel.com>; Ding, Xuan <xuan.ding@intel.com>;
> mb@smartsharesystems.com
> Subject: [PATCH v6 3/9] vhost: terminate when access lock is not taken
> 
> Be a bit more strict when a programmatic error is detected with regards to
> the access_lock not being taken.
> Mark the new helper with __rte_assert_exclusive_lock so that clang
> understands where locks are expected to be taken.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
> ---
>  lib/vhost/vhost.c      | 18 +++---------------
>  lib/vhost/vhost.h      | 10 ++++++++++
>  lib/vhost/virtio_net.c |  6 +-----
>  3 files changed, 14 insertions(+), 20 deletions(-)
> 
> diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
> index 19c7b92c32..8cd727ca2f 100644
> --- a/lib/vhost/vhost.c
> +++ b/lib/vhost/vhost.c
> @@ -1781,11 +1781,7 @@ rte_vhost_async_channel_register_thread_unsafe(int
> vid, uint16_t queue_id)
>  	if (unlikely(vq == NULL || !dev->async_copy))
>  		return -1;
> 
> -	if (unlikely(!rte_spinlock_is_locked(&vq->access_lock))) {
> -		VHOST_LOG_CONFIG(dev->ifname, ERR, "%s() called without access
> lock taken.\n",
> -			__func__);
> -		return -1;
> -	}
> +	vq_assert_lock(dev, vq);
> 
>  	return async_channel_register(dev, vq);
>  }
> @@ -1847,11 +1843,7 @@
> rte_vhost_async_channel_unregister_thread_unsafe(int vid, uint16_t
> queue_id)
>  	if (vq == NULL)
>  		return -1;
> 
> -	if (unlikely(!rte_spinlock_is_locked(&vq->access_lock))) {
> -		VHOST_LOG_CONFIG(dev->ifname, ERR, "%s() called without access
> lock taken.\n",
> -			__func__);
> -		return -1;
> -	}
> +	vq_assert_lock(dev, vq);
> 
>  	if (!vq->async)
>  		return 0;
> @@ -1994,11 +1986,7 @@ rte_vhost_async_get_inflight_thread_unsafe(int vid,
> uint16_t queue_id)
>  	if (vq == NULL)
>  		return ret;
> 
> -	if (unlikely(!rte_spinlock_is_locked(&vq->access_lock))) {
> -		VHOST_LOG_CONFIG(dev->ifname, ERR, "%s() called without access
> lock taken.\n",
> -			__func__);
> -		return -1;
> -	}
> +	vq_assert_lock(dev, vq);
> 
>  	if (!vq->async)
>  		return ret;
> diff --git a/lib/vhost/vhost.h b/lib/vhost/vhost.h
> index 1f913803f6..82fe9b5fda 100644
> --- a/lib/vhost/vhost.h
> +++ b/lib/vhost/vhost.h
> @@ -513,6 +513,16 @@ struct virtio_net {
>  	struct rte_vhost_user_extern_ops extern_ops;
>  } __rte_cache_aligned;
> 
> +static inline void
> +vq_assert_lock__(struct virtio_net *dev, struct vhost_virtqueue *vq,
> const char *func)
> +	__rte_assert_exclusive_lock(&vq->access_lock)
> +{
> +	if (unlikely(!rte_spinlock_is_locked(&vq->access_lock)))
> +		rte_panic("VHOST_CONFIG: (%s) %s() called without access lock
> taken.\n",
> +			dev->ifname, func);
> +}
> +#define vq_assert_lock(dev, vq) vq_assert_lock__(dev, vq, __func__)
> +
>  static __rte_always_inline bool
>  vq_is_packed(struct virtio_net *dev)
>  {
> diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
> index aac7aa9d01..cc9675ebe5 100644
> --- a/lib/vhost/virtio_net.c
> +++ b/lib/vhost/virtio_net.c
> @@ -2367,11 +2367,7 @@ rte_vhost_clear_queue_thread_unsafe(int vid,
> uint16_t queue_id,
> 
>  	vq = dev->virtqueue[queue_id];
> 
> -	if (unlikely(!rte_spinlock_is_locked(&vq->access_lock))) {
> -		VHOST_LOG_DATA(dev->ifname, ERR, "%s() called without access
> lock taken.\n",
> -			__func__);
> -		return -1;
> -	}
> +	vq_assert_lock(dev, vq);
> 
>  	if (unlikely(!vq->async)) {
>  		VHOST_LOG_DATA(dev->ifname, ERR,
> --
> 2.39.1

Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>

diff mbox series

Patch

diff --git a/lib/vhost/vhost.c b/lib/vhost/vhost.c
index 19c7b92c32..8cd727ca2f 100644
--- a/lib/vhost/vhost.c
+++ b/lib/vhost/vhost.c
@@ -1781,11 +1781,7 @@  rte_vhost_async_channel_register_thread_unsafe(int vid, uint16_t queue_id)
 	if (unlikely(vq == NULL || !dev->async_copy))
 		return -1;
 
-	if (unlikely(!rte_spinlock_is_locked(&vq->access_lock))) {
-		VHOST_LOG_CONFIG(dev->ifname, ERR, "%s() called without access lock taken.\n",
-			__func__);
-		return -1;
-	}
+	vq_assert_lock(dev, vq);
 
 	return async_channel_register(dev, vq);
 }
@@ -1847,11 +1843,7 @@  rte_vhost_async_channel_unregister_thread_unsafe(int vid, uint16_t queue_id)
 	if (vq == NULL)
 		return -1;
 
-	if (unlikely(!rte_spinlock_is_locked(&vq->access_lock))) {
-		VHOST_LOG_CONFIG(dev->ifname, ERR, "%s() called without access lock taken.\n",
-			__func__);
-		return -1;
-	}
+	vq_assert_lock(dev, vq);
 
 	if (!vq->async)
 		return 0;
@@ -1994,11 +1986,7 @@  rte_vhost_async_get_inflight_thread_unsafe(int vid, uint16_t queue_id)
 	if (vq == NULL)
 		return ret;
 
-	if (unlikely(!rte_spinlock_is_locked(&vq->access_lock))) {
-		VHOST_LOG_CONFIG(dev->ifname, ERR, "%s() called without access lock taken.\n",
-			__func__);
-		return -1;
-	}
+	vq_assert_lock(dev, vq);
 
 	if (!vq->async)
 		return ret;
diff --git a/lib/vhost/vhost.h b/lib/vhost/vhost.h
index 1f913803f6..82fe9b5fda 100644
--- a/lib/vhost/vhost.h
+++ b/lib/vhost/vhost.h
@@ -513,6 +513,16 @@  struct virtio_net {
 	struct rte_vhost_user_extern_ops extern_ops;
 } __rte_cache_aligned;
 
+static inline void
+vq_assert_lock__(struct virtio_net *dev, struct vhost_virtqueue *vq, const char *func)
+	__rte_assert_exclusive_lock(&vq->access_lock)
+{
+	if (unlikely(!rte_spinlock_is_locked(&vq->access_lock)))
+		rte_panic("VHOST_CONFIG: (%s) %s() called without access lock taken.\n",
+			dev->ifname, func);
+}
+#define vq_assert_lock(dev, vq) vq_assert_lock__(dev, vq, __func__)
+
 static __rte_always_inline bool
 vq_is_packed(struct virtio_net *dev)
 {
diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c
index aac7aa9d01..cc9675ebe5 100644
--- a/lib/vhost/virtio_net.c
+++ b/lib/vhost/virtio_net.c
@@ -2367,11 +2367,7 @@  rte_vhost_clear_queue_thread_unsafe(int vid, uint16_t queue_id,
 
 	vq = dev->virtqueue[queue_id];
 
-	if (unlikely(!rte_spinlock_is_locked(&vq->access_lock))) {
-		VHOST_LOG_DATA(dev->ifname, ERR, "%s() called without access lock taken.\n",
-			__func__);
-		return -1;
-	}
+	vq_assert_lock(dev, vq);
 
 	if (unlikely(!vq->async)) {
 		VHOST_LOG_DATA(dev->ifname, ERR,