DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] vhost: add vDPA resource cleanup callback
@ 2021-10-19 11:39 Xueming Li
  2021-10-21 12:00 ` Maxime Coquelin
  2022-01-27  8:45 ` [PATCH v2] " Xueming Li
  0 siblings, 2 replies; 13+ messages in thread
From: Xueming Li @ 2021-10-19 11:39 UTC (permalink / raw)
  To: dev; +Cc: xuemingl, Maxime Coquelin, Chenbo Xia

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: add vDPA resource cleanup callback
  2021-10-19 11:39 [dpdk-dev] [PATCH] vhost: add vDPA resource cleanup callback Xueming Li
@ 2021-10-21 12:00 ` Maxime Coquelin
  2021-10-21 12:35   ` Xueming(Steven) Li
  2022-01-27  8:45 ` [PATCH v2] " Xueming Li
  1 sibling, 1 reply; 13+ messages in thread
From: Maxime Coquelin @ 2021-10-21 12:00 UTC (permalink / raw)
  To: Xueming Li, dev; +Cc: Chenbo Xia

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: add vDPA resource cleanup callback
  2021-10-21 12:00 ` Maxime Coquelin
@ 2021-10-21 12:35   ` Xueming(Steven) Li
  2021-11-03  8:41     ` Xia, Chenbo
  0 siblings, 1 reply; 13+ messages in thread
From: Xueming(Steven) Li @ 2021-10-21 12:35 UTC (permalink / raw)
  To: maxime.coquelin, dev; +Cc: chenbo.xia

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
> 


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: add vDPA resource cleanup callback
  2021-10-21 12:35   ` Xueming(Steven) Li
@ 2021-11-03  8:41     ` Xia, Chenbo
  2021-11-03  8:46       ` Maxime Coquelin
  2021-11-03 13:42       ` [dpdk-dev] " Xueming(Steven) Li
  0 siblings, 2 replies; 13+ messages in thread
From: Xia, Chenbo @ 2021-11-03  8:41 UTC (permalink / raw)
  To: Xueming(Steven) Li, maxime.coquelin, dev

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: add vDPA resource cleanup callback
  2021-11-03  8:41     ` Xia, Chenbo
@ 2021-11-03  8:46       ` Maxime Coquelin
  2021-11-03 13:45         ` Xueming(Steven) Li
  2021-11-03 13:42       ` [dpdk-dev] " Xueming(Steven) Li
  1 sibling, 1 reply; 13+ messages in thread
From: Maxime Coquelin @ 2021-11-03  8:46 UTC (permalink / raw)
  To: Xia, Chenbo, Xueming(Steven) Li, dev



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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: add vDPA resource cleanup callback
  2021-11-03  8:41     ` Xia, Chenbo
  2021-11-03  8:46       ` Maxime Coquelin
@ 2021-11-03 13:42       ` Xueming(Steven) Li
  1 sibling, 0 replies; 13+ messages in thread
From: Xueming(Steven) Li @ 2021-11-03 13:42 UTC (permalink / raw)
  To: chenbo.xia, maxime.coquelin, dev

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: add vDPA resource cleanup callback
  2021-11-03  8:46       ` Maxime Coquelin
@ 2021-11-03 13:45         ` Xueming(Steven) Li
  2021-11-03 13:49           ` Maxime Coquelin
  0 siblings, 1 reply; 13+ messages in thread
From: Xueming(Steven) Li @ 2021-11-03 13:45 UTC (permalink / raw)
  To: chenbo.xia, maxime.coquelin, dev

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [dpdk-dev] [PATCH] vhost: add vDPA resource cleanup callback
  2021-11-03 13:45         ` Xueming(Steven) Li
@ 2021-11-03 13:49           ` Maxime Coquelin
  2022-01-26 10:03             ` Maxime Coquelin
  0 siblings, 1 reply; 13+ messages in thread
From: Maxime Coquelin @ 2021-11-03 13:49 UTC (permalink / raw)
  To: Xueming(Steven) Li, chenbo.xia, dev



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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] vhost: add vDPA resource cleanup callback
  2021-11-03 13:49           ` Maxime Coquelin
@ 2022-01-26 10:03             ` Maxime Coquelin
  2022-01-27  8:48               ` Xueming(Steven) Li
  0 siblings, 1 reply; 13+ messages in thread
From: Maxime Coquelin @ 2022-01-26 10:03 UTC (permalink / raw)
  To: Xueming(Steven) Li, chenbo.xia, dev

Hi Xueming,

On 11/3/21 14:49, Maxime Coquelin wrote:
> 
> 
> 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

Could you please rebase your patch if you want it in v22.03?

Thanks!
Maxime

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2] vhost: add vDPA resource cleanup callback
  2021-10-19 11:39 [dpdk-dev] [PATCH] vhost: add vDPA resource cleanup callback Xueming Li
  2021-10-21 12:00 ` Maxime Coquelin
@ 2022-01-27  8:45 ` Xueming Li
  2022-02-03 13:47   ` Maxime Coquelin
  2022-02-10 15:13   ` Maxime Coquelin
  1 sibling, 2 replies; 13+ messages in thread
From: Xueming Li @ 2022-01-27  8:45 UTC (permalink / raw)
  To: dev; +Cc: xuemingl, Matan Azrad, Xia Chenbo, Maxime Coquelin

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/vdpa_driver.h | 3 +++
 lib/vhost/vhost_user.c  | 6 ++++++
 2 files changed, 9 insertions(+)

diff --git a/lib/vhost/vdpa_driver.h b/lib/vhost/vdpa_driver.h
index fc2d6acedd1..fddbd506523 100644
--- a/lib/vhost/vdpa_driver.h
+++ b/lib/vhost/vdpa_driver.h
@@ -34,6 +34,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 5eb1dd68123..798b0ca4c0d 100644
--- a/lib/vhost/vhost_user.c
+++ b/lib/vhost/vhost_user.c
@@ -220,6 +220,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);
-- 
2.34.1


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] vhost: add vDPA resource cleanup callback
  2022-01-26 10:03             ` Maxime Coquelin
@ 2022-01-27  8:48               ` Xueming(Steven) Li
  0 siblings, 0 replies; 13+ messages in thread
From: Xueming(Steven) Li @ 2022-01-27  8:48 UTC (permalink / raw)
  To: chenbo.xia, maxime.coquelin, dev

On Wed, 2022-01-26 at 11:03 +0100, Maxime Coquelin wrote:
> Hi Xueming,
> 
> On 11/3/21 14:49, Maxime Coquelin wrote:
> > 
> > 
> > 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
> 
> Could you please rebase your patch if you want it in v22.03?

Thanks for reminding, new version sent.
Mlx5 PMD still WIP.

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


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] vhost: add vDPA resource cleanup callback
  2022-01-27  8:45 ` [PATCH v2] " Xueming Li
@ 2022-02-03 13:47   ` Maxime Coquelin
  2022-02-10 15:13   ` Maxime Coquelin
  1 sibling, 0 replies; 13+ messages in thread
From: Maxime Coquelin @ 2022-02-03 13:47 UTC (permalink / raw)
  To: Xueming Li, dev; +Cc: Matan Azrad, Xia Chenbo



On 1/27/22 09:45, 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/vdpa_driver.h | 3 +++
>   lib/vhost/vhost_user.c  | 6 ++++++
>   2 files changed, 9 insertions(+)
> 
> diff --git a/lib/vhost/vdpa_driver.h b/lib/vhost/vdpa_driver.h
> index fc2d6acedd1..fddbd506523 100644
> --- a/lib/vhost/vdpa_driver.h
> +++ b/lib/vhost/vdpa_driver.h
> @@ -34,6 +34,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 5eb1dd68123..798b0ca4c0d 100644
> --- a/lib/vhost/vhost_user.c
> +++ b/lib/vhost/vhost_user.c
> @@ -220,6 +220,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);

Reviewed-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2] vhost: add vDPA resource cleanup callback
  2022-01-27  8:45 ` [PATCH v2] " Xueming Li
  2022-02-03 13:47   ` Maxime Coquelin
@ 2022-02-10 15:13   ` Maxime Coquelin
  1 sibling, 0 replies; 13+ messages in thread
From: Maxime Coquelin @ 2022-02-10 15:13 UTC (permalink / raw)
  To: Xueming Li, dev; +Cc: Matan Azrad, Xia Chenbo



On 1/27/22 09:45, 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/vdpa_driver.h | 3 +++
>   lib/vhost/vhost_user.c  | 6 ++++++
>   2 files changed, 9 insertions(+)
> 
> diff --git a/lib/vhost/vdpa_driver.h b/lib/vhost/vdpa_driver.h
> index fc2d6acedd1..fddbd506523 100644
> --- a/lib/vhost/vdpa_driver.h
> +++ b/lib/vhost/vdpa_driver.h
> @@ -34,6 +34,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 5eb1dd68123..798b0ca4c0d 100644
> --- a/lib/vhost/vhost_user.c
> +++ b/lib/vhost/vhost_user.c
> @@ -220,6 +220,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);


Applied to dpdk-next-virtio/main.

Thanks,
Maxime


^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2022-02-10 15:13 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-19 11:39 [dpdk-dev] [PATCH] vhost: add vDPA resource cleanup callback Xueming Li
2021-10-21 12:00 ` Maxime Coquelin
2021-10-21 12:35   ` Xueming(Steven) Li
2021-11-03  8:41     ` Xia, Chenbo
2021-11-03  8:46       ` Maxime Coquelin
2021-11-03 13:45         ` Xueming(Steven) Li
2021-11-03 13:49           ` Maxime Coquelin
2022-01-26 10:03             ` Maxime Coquelin
2022-01-27  8:48               ` Xueming(Steven) Li
2021-11-03 13:42       ` [dpdk-dev] " Xueming(Steven) Li
2022-01-27  8:45 ` [PATCH v2] " Xueming Li
2022-02-03 13:47   ` Maxime Coquelin
2022-02-10 15:13   ` Maxime Coquelin

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