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

["Xueming(Steven) Li" <xuemingl@nvidia.com>]

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