Re: [dpdk-dev] [PATCH] vhost: add vDPA resource cleanup callback

[Maxime Coquelin <maxime.coquelin@redhat.com>]

On 11/3/21 14:45, Xueming(Steven) Li wrote:
> On Wed, 2021-11-03 at 09:46 +0100, Maxime Coquelin wrote:
>>
>> On 11/3/21 09:41, Xia, Chenbo wrote:
>>> Hi Xueming,
>>>
>>>> -----Original Message-----
>>>> From: Xueming(Steven) Li <xuemingl@nvidia.com>
>>>> Sent: Thursday, October 21, 2021 8:36 PM
>>>> To: maxime.coquelin@redhat.com; dev@dpdk.org
>>>> Cc: Xia, Chenbo <chenbo.xia@intel.com>
>>>> Subject: Re: [PATCH] vhost: add vDPA resource cleanup callback
>>>>
>>>> On Thu, 2021-10-21 at 14:00 +0200, Maxime Coquelin wrote:
>>>>> Hi Xueming,
>>>>>
>>>>> On 10/19/21 13:39, Xueming Li wrote:
>>>>>> This patch adds vDPA device cleanup callback to release resources on
>>>>>> vhost user connection close.
>>>>>>
>>>>>> Signed-off-by: Xueming Li <xuemingl@nvidia.com>
>>>>>> ---
>>>>>>     lib/vhost/rte_vdpa_dev.h | 3 +++
>>>>>>     lib/vhost/vhost_user.c   | 6 ++++++
>>>>>>     2 files changed, 9 insertions(+)
>>>>>>
>>>>>> diff --git a/lib/vhost/rte_vdpa_dev.h b/lib/vhost/rte_vdpa_dev.h
>>>>>> index b0f494815fa..2711004fe05 100644
>>>>>> --- a/lib/vhost/rte_vdpa_dev.h
>>>>>> +++ b/lib/vhost/rte_vdpa_dev.h
>>>>>> @@ -32,6 +32,9 @@ struct rte_vdpa_dev_ops {
>>>>>>     	/** Driver close the device (Mandatory) */
>>>>>>     	int (*dev_close)(int vid);
>>>>>>
>>>>>> +	/** Connection closed, clean up resources */
>>>>>> +	int (*dev_cleanup)(int vid);
>>>>>> +
>>>>>>     	/** Enable/disable this vring (Mandatory) */
>>>>>>     	int (*set_vring_state)(int vid, int vring, int state);
>>>>>>
>>>>>> diff --git a/lib/vhost/vhost_user.c b/lib/vhost/vhost_user.c
>>>>>> index 5a894ca0cc7..032b621c86c 100644
>>>>>> --- a/lib/vhost/vhost_user.c
>>>>>> +++ b/lib/vhost/vhost_user.c
>>>>>> @@ -162,6 +162,12 @@ free_mem_region(struct virtio_net *dev)
>>>>>>     void
>>>>>>     vhost_backend_cleanup(struct virtio_net *dev)
>>>>>>     {
>>>>>> +	struct rte_vdpa_device *vdpa_dev;
>>>>>> +
>>>>>> +	vdpa_dev = dev->vdpa_dev;
>>>>>> +	if (vdpa_dev && vdpa_dev->ops->dev_cleanup != NULL)
>>>>>> +		vdpa_dev->ops->dev_cleanup(dev->vid);
>>>>>> +
>>>>>>     	if (dev->mem) {
>>>>>>     		free_mem_region(dev);
>>>>>>     		rte_free(dev->mem);
>>>>>>
>>>>>
>>>>> What will be done there that cannot be done in .dev_close()?
>>>>
>>>> .dev_close() mainly handles VM suspend and driver reset. If release
>>>> everything inside dev_close(), the suspend and resume takes longer time
>>>> if number of VQs are huge. Customer want to upgrade VM configuration
>>>> using suspend and resume, pause customer VM too long can't be accepted.
>>>
>>> By saying 'upgrade VM configuration', do you mean VM memory hotplug? Or something
>>> more?
>>>
>>> Is this patch a next-step improvement of this commit?
>>>
>>> commit 127f9c6f7b78a47b73b3e1c39e021cc81a30b4c9
>>> Author: Matan Azrad <matan@mellanox.com>
>>> Date:   Mon Jun 29 14:08:19 2020 +0000
>>>
>>>       vhost: handle memory hotplug with vDPA devices
>>>
>>>       Some vDPA drivers' basic configurations should be updated when the
>>>       guest memory is hotplugged.
>>>
>>>       Close vDPA device before hotplug operation and recreate it after the
>>>       hotplug operation is done.
>>>
>>>       Signed-off-by: Matan Azrad <matan@mellanox.com>
>>>       Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>
>>>       Reviewed-by: Chenbo Xia <chenbo.xia@intel.com>
>>>
>>>> So the idea is to cache and reuse resource between dev_close() and
>>>> dev_conf(). Actually, the two functions looks more like dev_stop() and
>>>> dev_start().
>>>>
>>>> dev_cleanup hooks to vhost backend cleanup which called when socket
>>>> closed for both client and server mode, a safe point to cleanup all
>>>> cached resources.
>>>>
>>>>> Having the mlx5 implementation of this callback alongside this patch may
>>>>> help to understand.
>>>>
>>>> The mlx5 implementation still a prototype, pending on internal review.
>>>> So I just post the vhost part to get suggestion/comment. Let me know if
>>>> the ugly code does help :)
>>>
>>> I would prefer to see the mlx implementation with this patch in the same
>>> patchset to understand the problem. A new callback is fine if the problem
>>> itself makes sense :)
>>
>> FYI, I'm about to apply a patch that marks the vDPA driver API as
>> internal, when you will submit a new version please apply on top of it.
> 
> Haven't check your patch yet, but sounds good form subject :)

The branch is now ready, you can rebase your patch on top of it:
git://dpdk.org/next/dpdk-next-virtio main

Maxime

>>
>> Thanks,
>> Maxime
>>
>>> Thanks,
>>> Chenbo
>>>
>>>>
>>>>>
>>>>> Thanks,
>>>>> Maxime
>>>>>
>>>
>>
>