Re: [dpdk-dev] [PATCH v3 1/9] vhost: provide helper for host notifier ctrl

["Wang, Xiao W" <xiao.w.wang@intel.com>]

Hi,

> -----Original Message-----
> From: Maxime Coquelin [mailto:maxime.coquelin@redhat.com]
> Sent: Friday, December 14, 2018 5:33 AM
> To: Wang, Xiao W <xiao.w.wang@intel.com>;
> alejandro.lucero@netronome.com; Bie, Tiwei <tiwei.bie@intel.com>
> Cc: dev@dpdk.org; Wang, Zhihong <zhihong.wang@intel.com>; Ye, Xiaolong
> <xiaolong.ye@intel.com>
> Subject: Re: [PATCH v3 1/9] vhost: provide helper for host notifier ctrl
> 
> 
> 
> On 12/13/18 11:09 AM, Xiao Wang wrote:
> > VDPA driver can decide if it needs to enable/disable the host notifier
> > mapping, so exposing a API can allow flexibility. A later patch will
> > base on this.
> >
> > Signed-off-by: Xiao Wang <xiao.w.wang@intel.com>
> > ---
> > v2:
> > * Reword the vdpa host notifier control API comment.
> > ---
> >   drivers/net/ifc/ifcvf_vdpa.c           |  3 +++
> >   lib/librte_vhost/rte_vdpa.h            | 18 ++++++++++++++++++
> >   lib/librte_vhost/rte_vhost_version.map |  1 +
> >   lib/librte_vhost/vhost.c               |  3 +--
> >   lib/librte_vhost/vhost_user.c          |  7 +------
> >   5 files changed, 24 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/net/ifc/ifcvf_vdpa.c b/drivers/net/ifc/ifcvf_vdpa.c
> > index 97a57f182..e844109f3 100644
> > --- a/drivers/net/ifc/ifcvf_vdpa.c
> > +++ b/drivers/net/ifc/ifcvf_vdpa.c
> > @@ -556,6 +556,9 @@ ifcvf_dev_config(int vid)
> >   	rte_atomic32_set(&internal->dev_attached, 1);
> >   	update_datapath(internal);
> >
> > +	if (rte_vhost_host_notifier_ctrl(vid, true) != 0)
> > +		DRV_LOG(NOTICE, "vDPA (%d): software relay is used.", did);
> > +
> >   	return 0;
> >   }
> >
> > diff --git a/lib/librte_vhost/rte_vdpa.h b/lib/librte_vhost/rte_vdpa.h
> > index a418da47c..fff657391 100644
> > --- a/lib/librte_vhost/rte_vdpa.h
> > +++ b/lib/librte_vhost/rte_vdpa.h
> > @@ -11,6 +11,8 @@
> >    * Device specific vhost lib
> >    */
> >
> > +#include <stdbool.h>
> > +
> >   #include <rte_pci.h>
> >   #include "rte_vhost.h"
> >
> > @@ -155,4 +157,20 @@ rte_vdpa_get_device(int did);
> >    */
> >   int __rte_experimental
> >   rte_vdpa_get_device_num(void);
> > +
> > +/**
> > + * @warning
> > + * @b EXPERIMENTAL: this API may change without prior notice
> > + *
> > + * Enable/Disable host notifier mapping for a vdpa port.
> > + *
> > + * @param vid
> > + *  vhost device id
> > + * @enable
> > + *  true for host notifier map, false for host notifier unmap
> > + * @return
> > + *  0 on success, -1 on failure
> > + */
> > +int __rte_experimental
> > +rte_vhost_host_notifier_ctrl(int vid, bool enable);
> >   #endif /* _RTE_VDPA_H_ */
> > diff --git a/lib/librte_vhost/rte_vhost_version.map
> b/lib/librte_vhost/rte_vhost_version.map
> > index ae39b6e21..22302e972 100644
> > --- a/lib/librte_vhost/rte_vhost_version.map
> > +++ b/lib/librte_vhost/rte_vhost_version.map
> > @@ -83,4 +83,5 @@ EXPERIMENTAL {
> >   	rte_vhost_crypto_finalize_requests;
> >   	rte_vhost_crypto_set_zero_copy;
> >   	rte_vhost_va_from_guest_pa;
> > +	rte_vhost_host_notifier_ctrl;
> >   };
> > diff --git a/lib/librte_vhost/vhost.c b/lib/librte_vhost/vhost.c
> > index 70ac6bc9c..e7a60e0b4 100644
> > --- a/lib/librte_vhost/vhost.c
> > +++ b/lib/librte_vhost/vhost.c
> > @@ -408,8 +408,7 @@ vhost_detach_vdpa_device(int vid)
> >   	if (dev == NULL)
> >   		return;
> >
> > -	vhost_user_host_notifier_ctrl(vid, false);
> > -
> > +	vhost_destroy_device_notify(dev);
> It seems that is addition is not mentioned in the commit message.
> Why is it needed now?

Compared with the vhost_attach_vdpa_device, I think we should not just disable host notifier, but also destroy the vhost port. Also, this internal API is currently not used.
Yes, we need to mention this point in the commit message. BTW, I prefer to remove this unused internal API, by a separate patch.

BRs,
Xiao

> 
> 
> >   	dev->vdpa_dev_id = -1;
> >   }
> >
> > diff --git a/lib/librte_vhost/vhost_user.c b/lib/librte_vhost/vhost_user.c
> > index 3ea64eba6..5e0da0589 100644
> > --- a/lib/librte_vhost/vhost_user.c
> > +++ b/lib/librte_vhost/vhost_user.c
> > @@ -2045,11 +2045,6 @@ vhost_user_msg_handler(int vid, int fd)
> >   		if (vdpa_dev->ops->dev_conf)
> >   			vdpa_dev->ops->dev_conf(dev->vid);
> >   		dev->flags |= VIRTIO_DEV_VDPA_CONFIGURED;
> > -		if (vhost_user_host_notifier_ctrl(dev->vid, true) != 0) {
> > -			RTE_LOG(INFO, VHOST_CONFIG,
> > -				"(%d) software relay is used for vDPA,
> performance may be low.\n",
> > -				dev->vid);
> > -		}
> >   	}
> >
> >   	return 0;
> > @@ -2144,7 +2139,7 @@ static int
> vhost_user_slave_set_vring_host_notifier(struct virtio_net *dev,
> >   	return process_slave_message_reply(dev, &msg);
> >   }
> >
> > -int vhost_user_host_notifier_ctrl(int vid, bool enable)
> > +int rte_vhost_host_notifier_ctrl(int vid, bool enable)
> >   {
> >   	struct virtio_net *dev;
> >   	struct rte_vdpa_device *vdpa_dev;
> >