[v1,1/4] vhost: support host notifier queue configuration
Checks
Commit Message
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(-)
Comments
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.
> * @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);
>
> /**
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);
> >
> > /**
On 6/19/20 3:28 PM, Matan Azrad wrote:
>
>
> 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.
Ok, I didn't looked at this code.
> 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....
I personally don't like the style, but I can live with it if you prefer
doing it like that.
If you do it that way, you will have to rename VHOST_QUEUE_ALL to
RTE_VHOST_QUEUE_ALL, VHOST_MAX_VRING to RTE_VHOST_MAX_VRING and
VHOST_MAX_QUEUE_PAIRS to RTE_VHOST_MAX_QUEUE_PAIRS as it will become
part of the ABI.
Not that it also means that we won't be able to increase the maximum
number of rings without breaking the ABI.
>>> * @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);
>>>
>>> /**
>
Hi Maxime
From: Maxime Coquelin:
> On 6/19/20 3:28 PM, Matan Azrad wrote:
> >
> >
> > 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.
>
> Ok, I didn't looked at this code.
>
> > 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....
>
> I personally don't like the style, but I can live with it if you prefer doing it like
> that.
>
> If you do it that way, you will have to rename VHOST_QUEUE_ALL to
> RTE_VHOST_QUEUE_ALL, VHOST_MAX_VRING to RTE_VHOST_MAX_VRING
> and VHOST_MAX_QUEUE_PAIRS to RTE_VHOST_MAX_QUEUE_PAIRS as it
> will become part of the ABI.
>
> Not that it also means that we won't be able to increase the maximum
> number of rings without breaking the ABI.
What's about defining RTE_VHOST_QUEUE_ALL as UINT16_MAX?
> >>> * @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);
> >>>
> >>> /**
> >
On 6/21/20 8:26 AM, Matan Azrad wrote:
> Hi Maxime
>
> From: Maxime Coquelin:
>> On 6/19/20 3:28 PM, Matan Azrad wrote:
>>>
>>>
>>> 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.
>>
>> Ok, I didn't looked at this code.
>>
>>> 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....
>>
>> I personally don't like the style, but I can live with it if you prefer doing it like
>> that.
>>
>> If you do it that way, you will have to rename VHOST_QUEUE_ALL to
>> RTE_VHOST_QUEUE_ALL, VHOST_MAX_VRING to RTE_VHOST_MAX_VRING
>> and VHOST_MAX_QUEUE_PAIRS to RTE_VHOST_MAX_QUEUE_PAIRS as it
>> will become part of the ABI.
>>
>> Not that it also means that we won't be able to increase the maximum
>> number of rings without breaking the ABI.
>
> What's about defining RTE_VHOST_QUEUE_ALL as UINT16_MAX?
I am not fan, but it is better than basing it on VHOST_MAX_QUEUE_PAIRS.
>>>>> * @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);
>>>>>
>>>>> /**
>>>
>
@@ -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
-----------
@@ -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;
@@ -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);
@@ -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
* @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);
/**
* @warning
@@ -107,6 +107,8 @@
#define VHOST_USER_F_PROTOCOL_FEATURES 30
#endif
+#define VHOST_MAX_VRING 0x100
+#define VHOST_MAX_QUEUE_PAIRS 0x80
/**
* Information relating to memory regions including offsets to
@@ -202,9 +202,6 @@ struct vhost_virtqueue {
TAILQ_HEAD(, vhost_iotlb_entry) iotlb_pending_list;
} __rte_cache_aligned;
-#define VHOST_MAX_VRING 0x100
-#define VHOST_MAX_QUEUE_PAIRS 0x80
-
/* Declare IOMMU related bits for older kernels */
#ifndef VIRTIO_F_IOMMU_PLATFORM
@@ -2960,13 +2960,13 @@ static int vhost_user_slave_set_vring_host_notifier(struct virtio_net *dev,
return process_slave_message_reply(dev, &msg);
}
-int rte_vhost_host_notifier_ctrl(int vid, bool enable)
+int rte_vhost_host_notifier_ctrl(int vid, uint16_t qid, bool enable)
{
struct virtio_net *dev;
struct rte_vdpa_device *vdpa_dev;
int vfio_device_fd, did, ret = 0;
uint64_t offset, size;
- unsigned int i;
+ unsigned int i, q_start, q_last;
dev = get_device(vid);
if (!dev)
@@ -2990,6 +2990,16 @@ int rte_vhost_host_notifier_ctrl(int vid, bool enable)
if (!vdpa_dev)
return -ENODEV;
+ if (qid == VHOST_QUEUE_ALL) {
+ q_start = 0;
+ q_last = dev->nr_vring - 1;
+ } else {
+ if (qid >= dev->nr_vring)
+ return -EINVAL;
+ q_start = qid;
+ q_last = qid;
+ }
+
RTE_FUNC_PTR_OR_ERR_RET(vdpa_dev->ops->get_vfio_device_fd, -ENOTSUP);
RTE_FUNC_PTR_OR_ERR_RET(vdpa_dev->ops->get_notify_area, -ENOTSUP);
@@ -2998,7 +3008,7 @@ int rte_vhost_host_notifier_ctrl(int vid, bool enable)
return -ENOTSUP;
if (enable) {
- for (i = 0; i < dev->nr_vring; i++) {
+ for (i = q_start; i <= q_last; i++) {
if (vdpa_dev->ops->get_notify_area(vid, i, &offset,
&size) < 0) {
ret = -ENOTSUP;
@@ -3013,7 +3023,7 @@ int rte_vhost_host_notifier_ctrl(int vid, bool enable)
}
} else {
disable:
- for (i = 0; i < dev->nr_vring; i++) {
+ for (i = q_start; i <= q_last; i++) {
vhost_user_slave_set_vring_host_notifier(dev, i, -1,
0, 0);
}