DPDK patches and discussions
 help / color / mirror / Atom feed
From: Maxime Coquelin <maxime.coquelin@redhat.com>
To: "Xueming(Steven) Li" <xuemingl@mellanox.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, asafp@mellanox.com
Subject: Re: [dpdk-dev] [v2] vdpa/mlx5: fix queue update synchronization
Date: Mon, 3 Aug 2020 16:34:06 +0200	[thread overview]
Message-ID: <4776a516-1928-8e24-bace-52c71a8185ee@redhat.com> (raw)
In-Reply-To: <AM0PR05MB5204FADA70BD32EC6EB5B1D4AC4D0@AM0PR05MB5204.eurprd05.prod.outlook.com>



On 8/3/20 1:12 PM, Xueming(Steven) Li wrote:
> Hi Maxime,
> 
>> -----Original Message-----
>> From: Maxime Coquelin <maxime.coquelin@redhat.com>
>> Sent: Monday, August 3, 2020 5:51 PM
>> To: Xueming(Steven) 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
>>
>>
>>
>> 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.
> 
> Yes, could you please replace lines in integration? We don't care according the lock usage in cq handling.

OK, will do the change while applying:
Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime


  reply	other threads:[~2020-08-03 14:34 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
2020-08-03 11:12     ` Xueming(Steven) Li
2020-08-03 14:34       ` Maxime Coquelin [this message]
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=4776a516-1928-8e24-bace-52c71a8185ee@redhat.com \
    --to=maxime.coquelin@redhat.com \
    --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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).