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

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

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
>