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 >
next prev parent 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