DPDK patches and discussions
 help / color / mirror / Atom feed
From: Maxime Coquelin <maxime.coquelin@redhat.com>
To: Xueming Li <xuemingl@mellanox.com>
Cc: dev@dpdk.org, Asaf@dpdk.org, "Penso <asafp"@mellanox.com
Subject: Re: [dpdk-dev] [v2] vdpa/mlx5: fix queue update synchronization
Date: Mon, 3 Aug 2020 11:50:41 +0200
Message-ID: <d7f27aff-accb-6b82-7ef9-782d368d9dc7@redhat.com> (raw)
In-Reply-To: <1596360111-5088-1-git-send-email-xuemingl@mellanox.com>



On 8/2/20 11:21 AM, Xueming Li wrote:
> The driver CQ event management is done by non vhost library thread,
> either the dpdk host thread or the internal vDPA driver thread.
> 
> When a queue is updated the CQ may be destroyed and created by the vhost
> library thread via the queue state operation.
> 
> When the queue update feature was added, it didn't synchronize the CQ
> management to the queue update what may cause invalid memory access.
> 
> Add the aforementioned synchronization by a new per device configuration
> mutex.
> 
> Fixes: c47d6e83334e ("vdpa/mlx5: support queue update")
> 
> Signed-off-by: Xueming Li <xuemingl@mellanox.com>
> Acked-by: Matan Azrad <matan@mellanox.com>
> ---
>  drivers/vdpa/mlx5/mlx5_vdpa.c       | 8 +++++++-
>  drivers/vdpa/mlx5/mlx5_vdpa.h       | 1 +
>  drivers/vdpa/mlx5/mlx5_vdpa_event.c | 8 ++++++++
>  3 files changed, 16 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/vdpa/mlx5/mlx5_vdpa.c b/drivers/vdpa/mlx5/mlx5_vdpa.c
> index c0b87bcc01..a8f3e4b1de 100644
> --- a/drivers/vdpa/mlx5/mlx5_vdpa.c
> +++ b/drivers/vdpa/mlx5/mlx5_vdpa.c
> @@ -133,6 +133,7 @@ mlx5_vdpa_set_vring_state(int vid, int vring, int state)
>  	struct rte_vdpa_device *vdev = rte_vhost_get_vdpa_device(vid);
>  	struct mlx5_vdpa_priv *priv =
>  		mlx5_vdpa_find_priv_resource_by_vdev(vdev);
> +	int ret;
>  
>  	if (priv == NULL) {
>  		DRV_LOG(ERR, "Invalid vDPA device: %s.", vdev->device->name);
> @@ -142,7 +143,10 @@ mlx5_vdpa_set_vring_state(int vid, int vring, int state)
>  		DRV_LOG(ERR, "Too big vring id: %d.", vring);
>  		return -E2BIG;
>  	}
> -	return mlx5_vdpa_virtq_enable(priv, vring, state);
> +	pthread_mutex_lock(&priv->vq_config_lock);
> +	ret = mlx5_vdpa_virtq_enable(priv, vring, state);
> +	pthread_mutex_unlock(&priv->vq_config_lock);
> +	return ret;
>  }
>  
>  static int
> @@ -742,6 +746,7 @@ mlx5_vdpa_pci_probe(struct rte_pci_driver *pci_drv __rte_unused,
>  	}
>  	mlx5_vdpa_config_get(pci_dev->device.devargs, priv);
>  	SLIST_INIT(&priv->mr_list);
> +	pthread_mutex_init(&priv->vq_config_lock, NULL);
>  	pthread_mutex_lock(&priv_list_lock);
>  	TAILQ_INSERT_TAIL(&priv_list, priv, next);
>  	pthread_mutex_unlock(&priv_list_lock);
> @@ -793,6 +798,7 @@ mlx5_vdpa_pci_remove(struct rte_pci_device *pci_dev)
>  			priv->var = NULL;
>  		}
>  		mlx5_glue->close_device(priv->ctx);
> +		pthread_mutex_destroy(&priv->vq_config_lock);
>  		rte_free(priv);
>  	}
>  	return 0;
> diff --git a/drivers/vdpa/mlx5/mlx5_vdpa.h b/drivers/vdpa/mlx5/mlx5_vdpa.h
> index 57044d9d33..462805a352 100644
> --- a/drivers/vdpa/mlx5/mlx5_vdpa.h
> +++ b/drivers/vdpa/mlx5/mlx5_vdpa.h
> @@ -120,6 +120,7 @@ enum {
>  struct mlx5_vdpa_priv {
>  	TAILQ_ENTRY(mlx5_vdpa_priv) next;
>  	uint8_t configured;
> +	pthread_mutex_t vq_config_lock;
>  	uint64_t last_traffic_tic;
>  	pthread_t timer_tid;
>  	pthread_mutex_t timer_lock;
> diff --git a/drivers/vdpa/mlx5/mlx5_vdpa_event.c b/drivers/vdpa/mlx5/mlx5_vdpa_event.c
> index 7dc1ac0fa9..4a8b7b0bd9 100644
> --- a/drivers/vdpa/mlx5/mlx5_vdpa_event.c
> +++ b/drivers/vdpa/mlx5/mlx5_vdpa_event.c
> @@ -274,6 +274,7 @@ mlx5_vdpa_poll_handle(void *arg)
>  								 priv->event_us;
>  	while (1) {
>  		max = 0;
> +		pthread_mutex_lock(&priv->vq_config_lock);
>  		for (i = 0; i < priv->nr_virtqs; i++) {
>  			cq = &priv->virtqs[i].eqp.cq;
>  			if (cq->cq && !cq->armed) {
> @@ -298,6 +299,7 @@ mlx5_vdpa_poll_handle(void *arg)
>  					priv->vdev->device->name);
>  				mlx5_vdpa_arm_all_cqs(priv);
>  				pthread_mutex_lock(&priv->timer_lock);
> +				pthread_mutex_unlock(&priv->vq_config_lock);

Is it mandatory to hold timer_lock before releasing vq_config_lock?
If not, swapping would be maybe safer.

>  				priv->timer_on = 0;
>  				while (!priv->timer_on)
>  					pthread_cond_wait(&priv->timer_cond,
> @@ -312,6 +314,7 @@ mlx5_vdpa_poll_handle(void *arg)
>  		} else {
>  			priv->last_traffic_tic = current_tic;
>  		}
> +		pthread_mutex_unlock(&priv->vq_config_lock);
>  		mlx5_vdpa_timer_sleep(priv, max);
>  	}
>  	return NULL;
> @@ -327,6 +330,7 @@ mlx5_vdpa_interrupt_handler(void *cb_arg)
>  		uint8_t buf[sizeof(struct mlx5dv_devx_async_event_hdr) + 128];
>  	} out;
>  
> +	pthread_mutex_lock(&priv->vq_config_lock);
>  	while (mlx5_glue->devx_get_event(priv->eventc, &out.event_resp,
>  					 sizeof(out.buf)) >=
>  				       (ssize_t)sizeof(out.event_resp.cookie)) {
> @@ -337,12 +341,15 @@ mlx5_vdpa_interrupt_handler(void *cb_arg)
>  		struct mlx5_vdpa_virtq *virtq = container_of(eqp,
>  						   struct mlx5_vdpa_virtq, eqp);
>  
> +		if (!virtq->enable)
> +			continue;
>  		mlx5_vdpa_cq_poll(cq);
>  		/* Notify guest for descs consuming. */
>  		if (cq->callfd != -1)
>  			eventfd_write(cq->callfd, (eventfd_t)1);
>  		if (priv->event_mode == MLX5_VDPA_EVENT_MODE_ONLY_INTERRUPT) {
>  			mlx5_vdpa_cq_arm(priv, cq);
> +			pthread_mutex_unlock(&priv->vq_config_lock);
>  			return;
>  		}
>  		/* Don't arm again - timer will take control. */
> @@ -363,6 +370,7 @@ mlx5_vdpa_interrupt_handler(void *cb_arg)
>  		pthread_cond_signal(&priv->timer_cond);
>  	}
>  	pthread_mutex_unlock(&priv->timer_lock);
> +	pthread_mutex_unlock(&priv->vq_config_lock);
>  }
>  
>  int
> 


  reply	other threads:[~2020-08-03  9:50 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-02  8:14 [dpdk-dev] [PATCH] vdpa/mlx5: add device configuration lock Xueming Li
2020-08-02  9:21 ` [dpdk-dev] [v2] vdpa/mlx5: fix queue update synchronization Xueming Li
2020-08-03  9:50   ` Maxime Coquelin [this message]
2020-08-03 11:12     ` Xueming(Steven) Li
2020-08-03 14:34       ` Maxime Coquelin
2020-08-03 15:56   ` Maxime Coquelin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=d7f27aff-accb-6b82-7ef9-782d368d9dc7@redhat.com \
    --to=maxime.coquelin@redhat.com \
    --cc=Asaf@dpdk.org \
    --cc=asafp"@mellanox.com \
    --cc=dev@dpdk.org \
    --cc=xuemingl@mellanox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git