Re: [dpdk-dev] [PATCH v1 1/4] vhost: support host notifier queue configuration

[Matan Azrad <matan@mellanox.com>]

From: Maxime Coquelin:
> On 6/18/20 6:28 PM, Matan Azrad wrote:
> > As an arrangement to per queue operations in the vDPA device it is
> > needed to change the next experimental API:
> >
> > The API ``rte_vhost_host_notifier_ctrl`` was changed to be per queue
> > instead of per device.
> >
> > A `qid` parameter was added to the API arguments list.
> >
> > Setting the parameter to the value VHOST_QUEUE_ALL will configure the
> > host notifier to all the device queues as done before this patch.
> >
> > Signed-off-by: Matan Azrad <matan@mellanox.com>
> > ---
> >  doc/guides/rel_notes/release_20_08.rst |  2 ++
> >  drivers/vdpa/ifc/ifcvf_vdpa.c          |  6 +++---
> >  drivers/vdpa/mlx5/mlx5_vdpa.c          |  5 +++--
> >  lib/librte_vhost/rte_vdpa.h            |  8 ++++++--
> >  lib/librte_vhost/rte_vhost.h           |  2 ++
> >  lib/librte_vhost/vhost.h               |  3 ---
> >  lib/librte_vhost/vhost_user.c          | 18 ++++++++++++++----
> >  7 files changed, 30 insertions(+), 14 deletions(-)
> >
> > diff --git a/doc/guides/rel_notes/release_20_08.rst
> > b/doc/guides/rel_notes/release_20_08.rst
> > index ba16d3b..9732959 100644
> > --- a/doc/guides/rel_notes/release_20_08.rst
> > +++ b/doc/guides/rel_notes/release_20_08.rst
> > @@ -111,6 +111,8 @@ API Changes
> >     Also, make sure to start the actual text at the margin.
> >
> =========================================================
> >
> > +* vhost: The API of ``rte_vhost_host_notifier_ctrl`` was changed to
> > +be per
> > +  queue and not per device, a qid parameter was added to the arguments
> list.
> >
> >  ABI Changes
> >  -----------
> > diff --git a/drivers/vdpa/ifc/ifcvf_vdpa.c
> > b/drivers/vdpa/ifc/ifcvf_vdpa.c index ec97178..336837a 100644
> > --- a/drivers/vdpa/ifc/ifcvf_vdpa.c
> > +++ b/drivers/vdpa/ifc/ifcvf_vdpa.c
> > @@ -839,7 +839,7 @@ struct internal_list {
> >  	vdpa_ifcvf_stop(internal);
> >  	vdpa_disable_vfio_intr(internal);
> >
> > -	ret = rte_vhost_host_notifier_ctrl(vid, false);
> > +	ret = rte_vhost_host_notifier_ctrl(vid, VHOST_QUEUE_ALL, false);
> >  	if (ret && ret != -ENOTSUP)
> >  		goto error;
> >
> > @@ -858,7 +858,7 @@ struct internal_list {
> >  	if (ret)
> >  		goto stop_vf;
> >
> > -	rte_vhost_host_notifier_ctrl(vid, true);
> > +	rte_vhost_host_notifier_ctrl(vid, VHOST_QUEUE_ALL, true);
> >
> >  	internal->sw_fallback_running = true;
> >
> > @@ -893,7 +893,7 @@ struct internal_list {
> >  	rte_atomic32_set(&internal->dev_attached, 1);
> >  	update_datapath(internal);
> >
> > -	if (rte_vhost_host_notifier_ctrl(vid, true) != 0)
> > +	if (rte_vhost_host_notifier_ctrl(vid, VHOST_QUEUE_ALL, true) != 0)
> >  		DRV_LOG(NOTICE, "vDPA (%d): software relay is used.", did);
> >
> >  	return 0;
> > diff --git a/drivers/vdpa/mlx5/mlx5_vdpa.c
> > b/drivers/vdpa/mlx5/mlx5_vdpa.c index 9e758b6..8ea1300 100644
> > --- a/drivers/vdpa/mlx5/mlx5_vdpa.c
> > +++ b/drivers/vdpa/mlx5/mlx5_vdpa.c
> > @@ -147,7 +147,8 @@
> >  	int ret;
> >
> >  	if (priv->direct_notifier) {
> > -		ret = rte_vhost_host_notifier_ctrl(priv->vid, false);
> > +		ret = rte_vhost_host_notifier_ctrl(priv->vid,
> VHOST_QUEUE_ALL,
> > +						   false);
> >  		if (ret != 0) {
> >  			DRV_LOG(INFO, "Direct HW notifier FD cannot be "
> >  				"destroyed for device %d: %d.", priv->vid,
> ret); @@ -155,7 +156,7
> > @@
> >  		}
> >  		priv->direct_notifier = 0;
> >  	}
> > -	ret = rte_vhost_host_notifier_ctrl(priv->vid, true);
> > +	ret = rte_vhost_host_notifier_ctrl(priv->vid, VHOST_QUEUE_ALL,
> > +true);
> >  	if (ret != 0)
> >  		DRV_LOG(INFO, "Direct HW notifier FD cannot be configured
> for"
> >  			" device %d: %d.", priv->vid, ret); diff --git
> > a/lib/librte_vhost/rte_vdpa.h b/lib/librte_vhost/rte_vdpa.h index
> > ecb3d91..2db536c 100644
> > --- a/lib/librte_vhost/rte_vdpa.h
> > +++ b/lib/librte_vhost/rte_vdpa.h
> > @@ -202,22 +202,26 @@ struct rte_vdpa_device *  int
> > rte_vdpa_get_device_num(void);
> >
> > +#define VHOST_QUEUE_ALL VHOST_MAX_VRING
> > +
> >  /**
> >   * @warning
> >   * @b EXPERIMENTAL: this API may change without prior notice
> >   *
> > - * Enable/Disable host notifier mapping for a vdpa port.
> > + * Enable/Disable host notifier mapping for a vdpa queue.
> >   *
> >   * @param vid
> >   *  vhost device id
> >   * @param enable
> >   *  true for host notifier map, false for host notifier unmap
> > + * @param qid
> > + *  vhost queue id, VHOST_QUEUE_ALL to configure all the device
> > + queues
> I would prefer two APIs that passing a special ID that means all queues:
> 
> rte_vhost_host_notifier_ctrl(int vid, uint16_t qid, bool enable);
> rte_vhost_host_notifier_ctrl_all(int vid, bool enable);
> 
> I think it is clearer for the user of the API.
> Or if you think an extra API is overkill, just let the driver loop on all the
> queues.

We have a lot of options here with pros and cons.
I took the rte_eth_dev_callback_register style.

It is less intrusive with minimum code change.  

I'm not sure what is the clearest option but the current suggestion is well defined and 
allows to configure all the queues too.

Let me know what you prefer....

> >   * @return
> >   *  0 on success, -1 on failure
> >   */
> >  __rte_experimental
> >  int
> > -rte_vhost_host_notifier_ctrl(int vid, bool enable);
> > +rte_vhost_host_notifier_ctrl(int vid, uint16_t qid, bool enable);
> >
> >  /**