diff mbox series

vhost: add vDPA resource cleanup callback

Message ID 20211019113956.2254537-1-xuemingl@nvidia.com (mailing list archive)
State New
Delegated to: Maxime Coquelin
Headers show
Series vhost: add vDPA resource cleanup callback | expand

Checks

Context Check Description
ci/intel-Testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-intel-Functional fail Functional Testing issues
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-x86_64-compile-testing success Testing PASS
ci/iol-x86_64-unit-testing success Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-aarch64-compile-testing success Testing PASS
ci/github-robot: build success github build: passed
ci/checkpatch success coding style OK

Commit Message

Xueming(Steven) Li Oct. 19, 2021, 11:39 a.m. UTC
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(+)

Comments

Maxime Coquelin Oct. 21, 2021, noon UTC | #1
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()?
Having the mlx5 implementation of this callback alongside this patch may
help to understand.

Thanks,
Maxime
Xueming(Steven) Li Oct. 21, 2021, 12:35 p.m. UTC | #2
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.
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 :)

> 
> Thanks,
> Maxime
>
Xia, Chenbo Nov. 3, 2021, 8:41 a.m. UTC | #3
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 :)

Thanks,
Chenbo 

> 
> >
> > Thanks,
> > Maxime
> >
Maxime Coquelin Nov. 3, 2021, 8:46 a.m. UTC | #4
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.

Thanks,
Maxime

> Thanks,
> Chenbo
> 
>>
>>>
>>> Thanks,
>>> Maxime
>>>
>
Xueming(Steven) Li Nov. 3, 2021, 1:42 p.m. UTC | #5
On Wed, 2021-11-03 at 08:41 +0000, 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?

Hi Chenbo,

Basically irrelevant IIUC, VM could be paused for more reasons like
disk upgrade or something similar. To speed up device close, resources
that could be reused in resume is not released. That's why we need a
callback to free resources completely.

> 
> 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 :)

Will post once ready, thanks!

> 
> Thanks,
> Chenbo 
> 
> > 
> > > 
> > > Thanks,
> > > Maxime
> > > 
>
Xueming(Steven) Li Nov. 3, 2021, 1:45 p.m. UTC | #6
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 :)

> 
> Thanks,
> Maxime
> 
> > Thanks,
> > Chenbo
> > 
> > > 
> > > > 
> > > > Thanks,
> > > > Maxime
> > > > 
> > 
>
Maxime Coquelin Nov. 3, 2021, 1:49 p.m. UTC | #7
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
>>>>>
>>>
>>
>
diff mbox series

Patch

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);