[RFC,v2,3/9] vhost: annotate virtqueue access lock

  This change simply annotates existing paths of the code leading to
manipulations of the vq->access_lock.

One small change is required: vhost_poll_enqueue_completed was getting
a queue_id to get hold of the vq, while its callers already knew of
the vq. For the annotation sake, vq is now directly passed.

vhost_user_lock_all_queue_pairs and vhost_user_unlock_all_queue_pairs
are skipped since vq->access_lock are conditionally held.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/vhost/vhost.h      |  2 ++
 lib/vhost/vhost_user.c |  2 ++
 lib/vhost/virtio_net.c | 16 ++++++++++++----
 3 files changed, 16 insertions(+), 4 deletions(-)
  

Hi David,

Please see replies inline.

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Wednesday, March 30, 2022 9:50 PM
> To: dev@dpdk.org
> Cc: maxime.coquelin@redhat.com; Xia, Chenbo <chenbo.xia@intel.com>; Hu,
> Jiayu <jiayu.hu@intel.com>; Wang, YuanX <yuanx.wang@intel.com>; Ding,
> Xuan <xuan.ding@intel.com>
> Subject: [RFC PATCH v2 3/9] vhost: annotate virtqueue access lock
> 
> This change simply annotates existing paths of the code leading to
> manipulations of the vq->access_lock.
> 
> One small change is required: vhost_poll_enqueue_completed was getting a
> queue_id to get hold of the vq, while its callers already knew of the vq. For
> the annotation sake, vq is now directly passed.
> 
> vhost_user_lock_all_queue_pairs and vhost_user_unlock_all_queue_pairs
> are skipped since vq->access_lock are conditionally held.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>  lib/vhost/vhost.h      |  2 ++
>  lib/vhost/vhost_user.c |  2 ++
>  lib/vhost/virtio_net.c | 16 ++++++++++++----
>  3 files changed, 16 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/vhost/vhost.h b/lib/vhost/vhost.h index
> a9edc271aa..158460b7d7 100644
> --- a/lib/vhost/vhost.h
> +++ b/lib/vhost/vhost.h
> @@ -834,6 +834,7 @@ vhost_need_event(uint16_t event_idx, uint16_t
> new_idx, uint16_t old)
> 
>  static __rte_always_inline void
>  vhost_vring_call_split(struct virtio_net *dev, struct vhost_virtqueue *vq)
> +	RTE_EXC_LOCK_REQUIRES(vq->access_lock)

vhost_vring_call_split() is called in rte_vhost_vring_call() too, but it doesn't
acquire vq->access_lock before calling vhost_vring_call_split().

>  {
>  	/* Flush used->idx update before we read avail->flags. */
>  	rte_atomic_thread_fence(__ATOMIC_SEQ_CST);
> @@ -872,6 +873,7 @@ vhost_vring_call_split(struct virtio_net *dev, struct
> vhost_virtqueue *vq)
> 
>  static __rte_always_inline void
>  vhost_vring_call_packed(struct virtio_net *dev, struct vhost_virtqueue *vq)
> +	RTE_EXC_LOCK_REQUIRES(vq->access_lock)

Ditto.

>  {
>  	uint16_t old, new, off, off_wrap;
>  	bool signalled_used_valid, kick = false; diff --git
> a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c index
> 1d390677fa..87eaa2ab4a 100644
> --- a/lib/vhost/vhost_user.c
> +++ b/lib/vhost/vhost_user.c
> @@ -2909,6 +2909,7 @@ vhost_user_check_and_alloc_queue_pair(struct
> virtio_net *dev,
> 
>  static void
>  vhost_user_lock_all_queue_pairs(struct virtio_net *dev)
> +	RTE_NO_ANNOTATED_LOCK_CHECK
>  {
>  	unsigned int i = 0;
>  	unsigned int vq_num = 0;
> @@ -2926,6 +2927,7 @@ vhost_user_lock_all_queue_pairs(struct virtio_net
> *dev)
> 
>  static void
>  vhost_user_unlock_all_queue_pairs(struct virtio_net *dev)
> +	RTE_NO_ANNOTATED_LOCK_CHECK
>  {
>  	unsigned int i = 0;
>  	unsigned int vq_num = 0;
> diff --git a/lib/vhost/virtio_net.c b/lib/vhost/virtio_net.c index
> 5f432b0d77..514ee00993 100644
> --- a/lib/vhost/virtio_net.c
> +++ b/lib/vhost/virtio_net.c
> @@ -1246,6 +1246,7 @@ vhost_enqueue_single_packed(struct virtio_net
> *dev,  static __rte_noinline uint32_t  virtio_dev_rx_split(struct virtio_net
> *dev, struct vhost_virtqueue *vq,
>  	struct rte_mbuf **pkts, uint32_t count)
> +	RTE_EXC_LOCK_REQUIRES(vq->access_lock)
>  {
>  	uint32_t pkt_idx = 0;
>  	uint16_t num_buffers;
> @@ -1441,6 +1442,7 @@ virtio_dev_rx_packed(struct virtio_net *dev,
>  		     struct vhost_virtqueue *__rte_restrict vq,
>  		     struct rte_mbuf **__rte_restrict pkts,
>  		     uint32_t count)
> +	RTE_EXC_LOCK_REQUIRES(vq->access_lock)
>  {
>  	uint32_t pkt_idx = 0;
> 
> @@ -1955,11 +1957,11 @@ write_back_completed_descs_packed(struct
> vhost_virtqueue *vq,  }
> 
>  static __rte_always_inline uint16_t
> -vhost_poll_enqueue_completed(struct virtio_net *dev, uint16_t queue_id,
> +vhost_poll_enqueue_completed(struct virtio_net *dev, struct
> +vhost_virtqueue *vq,
>  		struct rte_mbuf **pkts, uint16_t count, int16_t dma_id,
>  		uint16_t vchan_id)
> +	RTE_EXC_LOCK_REQUIRES(vq->access_lock)

rte_vhost_clear_queue_thread_unsafe() doesn't acquire vq->access_lock.
Will it cause a compiler warning?

Thanks,
Jiayu
>  {
> -	struct vhost_virtqueue *vq = dev->virtqueue[queue_id];
>  	struct vhost_async *async = vq->async;
>  	struct async_inflight_info *pkts_info = async->pkts_info;
>  	uint16_t nr_cpl_pkts = 0;
> @@ -2062,7 +2064,7 @@ rte_vhost_poll_enqueue_completed(int vid,
> uint16_t queue_id,
>  		goto out;
>  	}
> 
> -	n_pkts_cpl = vhost_poll_enqueue_completed(dev, queue_id, pkts,
> count, dma_id, vchan_id);
> +	n_pkts_cpl = vhost_poll_enqueue_completed(dev, vq, pkts, count,
> +dma_id, vchan_id);
> 
>  out:
>  	rte_spinlock_unlock(&vq->access_lock);
> @@ -2104,7 +2106,7 @@ rte_vhost_clear_queue_thread_unsafe(int vid,
> uint16_t queue_id,
>  		return 0;
>  	}
> 
> -	n_pkts_cpl = vhost_poll_enqueue_completed(dev, queue_id, pkts,
> count, dma_id, vchan_id);
> +	n_pkts_cpl = vhost_poll_enqueue_completed(dev, vq, pkts, count,
> +dma_id, vchan_id);
> 
>  	return n_pkts_cpl;
>  }
> @@ -2679,6 +2681,7 @@ static uint16_t
>  virtio_dev_tx_split(struct virtio_net *dev, struct vhost_virtqueue *vq,
>  	struct rte_mempool *mbuf_pool, struct rte_mbuf **pkts, uint16_t
> count,
>  	bool legacy_ol_flags)
> +	RTE_EXC_LOCK_REQUIRES(vq->access_lock)
>  {
>  	uint16_t i;
>  	uint16_t free_entries;
> @@ -2774,6 +2777,7 @@ static uint16_t
>  virtio_dev_tx_split_legacy(struct virtio_net *dev,
>  	struct vhost_virtqueue *vq, struct rte_mempool *mbuf_pool,
>  	struct rte_mbuf **pkts, uint16_t count)
> +	RTE_EXC_LOCK_REQUIRES(vq->access_lock)
>  {
>  	return virtio_dev_tx_split(dev, vq, mbuf_pool, pkts, count, true);  }
> @@ -2783,6 +2787,7 @@ static uint16_t
> virtio_dev_tx_split_compliant(struct virtio_net *dev,
>  	struct vhost_virtqueue *vq, struct rte_mempool *mbuf_pool,
>  	struct rte_mbuf **pkts, uint16_t count)
> +	RTE_EXC_LOCK_REQUIRES(vq->access_lock)
>  {
>  	return virtio_dev_tx_split(dev, vq, mbuf_pool, pkts, count, false);  }
> @@ -2982,6 +2987,7 @@ virtio_dev_tx_packed(struct virtio_net *dev,
>  		     struct rte_mbuf **__rte_restrict pkts,
>  		     uint32_t count,
>  		     bool legacy_ol_flags)
> +	RTE_EXC_LOCK_REQUIRES(vq->access_lock)
>  {
>  	uint32_t pkt_idx = 0;
> 
> @@ -3025,6 +3031,7 @@ static uint16_t
>  virtio_dev_tx_packed_legacy(struct virtio_net *dev,
>  	struct vhost_virtqueue *__rte_restrict vq, struct rte_mempool
> *mbuf_pool,
>  	struct rte_mbuf **__rte_restrict pkts, uint32_t count)
> +	RTE_EXC_LOCK_REQUIRES(vq->access_lock)
>  {
>  	return virtio_dev_tx_packed(dev, vq, mbuf_pool, pkts, count, true);  }
> @@ -3034,6 +3041,7 @@ static uint16_t
> virtio_dev_tx_packed_compliant(struct virtio_net *dev,
>  	struct vhost_virtqueue *__rte_restrict vq, struct rte_mempool
> *mbuf_pool,
>  	struct rte_mbuf **__rte_restrict pkts, uint32_t count)
> +	RTE_EXC_LOCK_REQUIRES(vq->access_lock)
>  {
>  	return virtio_dev_tx_packed(dev, vq, mbuf_pool, pkts, count, false);  }
> --
> 2.23.0
  

On Thu, Apr 7, 2022 at 3:40 AM Hu, Jiayu <jiayu.hu@intel.com> wrote:
> > diff --git a/lib/vhost/vhost.h b/lib/vhost/vhost.h index
> > a9edc271aa..158460b7d7 100644
> > --- a/lib/vhost/vhost.h
> > +++ b/lib/vhost/vhost.h
> > @@ -834,6 +834,7 @@ vhost_need_event(uint16_t event_idx, uint16_t
> > new_idx, uint16_t old)
> >
> >  static __rte_always_inline void
> >  vhost_vring_call_split(struct virtio_net *dev, struct vhost_virtqueue *vq)
> > +     RTE_EXC_LOCK_REQUIRES(vq->access_lock)
>
> vhost_vring_call_split() is called in rte_vhost_vring_call() too, but it doesn't
> acquire vq->access_lock before calling vhost_vring_call_split().

I have some issues with sending patches from other people (Mimecast
seems to think I try to impersonate them and strip the content of the
mail?).

You'll notice the series in patchwork starts at patch 2.
https://patchwork.dpdk.org/project/dpdk/list/?series=22292&state=*

My intent was to have Maxime fix (already in next-virtio:
https://git.dpdk.org/next/dpdk-next-virtio/commit/?id=53d8fffcf8e3c89c9785f8ce50db892f2cdfd7c7)
in this series.


[snip]

> > @@ -1955,11 +1957,11 @@ write_back_completed_descs_packed(struct
> > vhost_virtqueue *vq,  }
> >
> >  static __rte_always_inline uint16_t
> > -vhost_poll_enqueue_completed(struct virtio_net *dev, uint16_t queue_id,
> > +vhost_poll_enqueue_completed(struct virtio_net *dev, struct
> > +vhost_virtqueue *vq,
> >               struct rte_mbuf **pkts, uint16_t count, int16_t dma_id,
> >               uint16_t vchan_id)
> > +     RTE_EXC_LOCK_REQUIRES(vq->access_lock)
>
> rte_vhost_clear_queue_thread_unsafe() doesn't acquire vq->access_lock.
> Will it cause a compiler warning?

Mm, probably a rebase/split error on my side when doing rfc v2.
On the other hand I don't think we can enable the check at this point
of the series in any case (there would be other warnings, at least for
rwlocks).
I'll double check before sending next revision, thanks for pointing out.