[dpdk-dev] [PATCH] doc: announce transition to vDPA port close function

[dpdk-dev] [PATCH] doc: announce transition to vDPA port close function

[Thomas Monjalon]

There is a layer violation in the vDPA API which encourages to destroy
a full device with rte_dev_remove() instead of just closing the port.
The plan is to introduce a new function in 21.08, promote in 21.11,
and deprecate rte_vdpa_get_rte_device() in 21.11.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
 doc/guides/rel_notes/deprecation.rst | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 9584d6bfd7..30f84403eb 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -126,6 +126,14 @@ Deprecation Notices
   can still be used if users specify the devarg "driver=i40evf". I40evf will
   be deleted in DPDK 21.11.
 
+* vdpa: The vDPA API should not try to manipulate or export
+  any ``rte_device`` object, which belongs to the bus layer.
+  The function ``rte_vdpa_get_rte_device()`` will be deprecated in 21.11,
+  when its usage will be replaced with a function ``rte_vdpa_close()``.
+  The new function should enter in 21.08 and get promoted to stable in 21.11.
+  A port close function will allow to close a single port without destroying
+  the rest of the device.
+
 * eventdev: The structure ``rte_event_eth_rx_adapter_queue_conf`` will be
   extended to include ``rte_event_eth_rx_adapter_event_vector_config`` elements
   and the function ``rte_event_eth_rx_adapter_queue_event_vector_config`` will
-- 
2.31.1

Re: [dpdk-dev] [PATCH] doc: announce transition to vDPA port close function

[Andrew Rybchenko]

On 5/18/21 10:34 AM, Thomas Monjalon wrote:
> There is a layer violation in the vDPA API which encourages to destroy
> a full device with rte_dev_remove() instead of just closing the port.
> The plan is to introduce a new function in 21.08, promote in 21.11,
> and deprecate rte_vdpa_get_rte_device() in 21.11.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>

Acked-by: Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>

Re: [dpdk-dev] [PATCH] doc: announce transition to vDPA port close function

[Thomas Monjalon]

18/05/2021 09:34, Thomas Monjalon:
> There is a layer violation in the vDPA API which encourages to destroy
> a full device with rte_dev_remove() instead of just closing the port.
> The plan is to introduce a new function in 21.08, promote in 21.11,
> and deprecate rte_vdpa_get_rte_device() in 21.11.

I forgot this issue.
Let's hope I (or someone else) will introduce the "close" function
so we can remove this layer violation from DPDK 22.11.

Re: [PATCH] doc: announce transition to vDPA port close function

[Maxime Coquelin]

On 5/18/21 09:34, Thomas Monjalon wrote:
> There is a layer violation in the vDPA API which encourages to destroy
> a full device with rte_dev_remove() instead of just closing the port.
> The plan is to introduce a new function in 21.08, promote in 21.11,
> and deprecate rte_vdpa_get_rte_device() in 21.11.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
>   doc/guides/rel_notes/deprecation.rst | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index 9584d6bfd7..30f84403eb 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -126,6 +126,14 @@ Deprecation Notices
>     can still be used if users specify the devarg "driver=i40evf". I40evf will
>     be deleted in DPDK 21.11.
>   
> +* vdpa: The vDPA API should not try to manipulate or export
> +  any ``rte_device`` object, which belongs to the bus layer.
> +  The function ``rte_vdpa_get_rte_device()`` will be deprecated in 21.11,
> +  when its usage will be replaced with a function ``rte_vdpa_close()``.
> +  The new function should enter in 21.08 and get promoted to stable in 21.11.
> +  A port close function will allow to close a single port without destroying
> +  the rest of the device.
> +
>   * eventdev: The structure ``rte_event_eth_rx_adapter_queue_conf`` will be
>     extended to include ``rte_event_eth_rx_adapter_event_vector_config`` elements
>     and the function ``rte_event_eth_rx_adapter_queue_event_vector_config`` will

Maybe there was some changes since you posted the announce, but I don't
see why rte_vdpa_close() would be needed. It seems the only user of
rte_vdpa_get_rte_device() is the internal vDPA example, and it only use
it to get and print the device name.

Thanks,
Maxime

Re: [PATCH] doc: announce transition to vDPA port close function

[Thomas Monjalon]

12/07/2022 11:16, Maxime Coquelin:
> 
> On 5/18/21 09:34, Thomas Monjalon wrote:
> > There is a layer violation in the vDPA API which encourages to destroy
> > a full device with rte_dev_remove() instead of just closing the port.
> > The plan is to introduce a new function in 21.08, promote in 21.11,
> > and deprecate rte_vdpa_get_rte_device() in 21.11.
[...]
> > +* vdpa: The vDPA API should not try to manipulate or export
> > +  any ``rte_device`` object, which belongs to the bus layer.
> > +  The function ``rte_vdpa_get_rte_device()`` will be deprecated in 21.11,
> > +  when its usage will be replaced with a function ``rte_vdpa_close()``.
> > +  The new function should enter in 21.08 and get promoted to stable in 21.11.
> > +  A port close function will allow to close a single port without destroying
> > +  the rest of the device.
> 
> Maybe there was some changes since you posted the announce, but I don't
> see why rte_vdpa_close() would be needed. It seems the only user of
> rte_vdpa_get_rte_device() is the internal vDPA example, and it only use
> it to get and print the device name.

You're right, it was an oversight.
So we need only to get the rte_device name.

I propose to replace
	struct rte_device *rte_vdpa_get_rte_device(struct rte_vdpa_device *vdpa_dev);
with
	const char *rte_vdpa_get_name(void);

Re: [PATCH] doc: announce transition to vDPA port close function

[Thomas Monjalon]

12/07/2022 14:26, Thomas Monjalon:
> 12/07/2022 11:16, Maxime Coquelin:
> > 
> > On 5/18/21 09:34, Thomas Monjalon wrote:
> > > There is a layer violation in the vDPA API which encourages to destroy
> > > a full device with rte_dev_remove() instead of just closing the port.
> > > The plan is to introduce a new function in 21.08, promote in 21.11,
> > > and deprecate rte_vdpa_get_rte_device() in 21.11.
> [...]
> > > +* vdpa: The vDPA API should not try to manipulate or export
> > > +  any ``rte_device`` object, which belongs to the bus layer.
> > > +  The function ``rte_vdpa_get_rte_device()`` will be deprecated in 21.11,
> > > +  when its usage will be replaced with a function ``rte_vdpa_close()``.
> > > +  The new function should enter in 21.08 and get promoted to stable in 21.11.
> > > +  A port close function will allow to close a single port without destroying
> > > +  the rest of the device.
> > 
> > Maybe there was some changes since you posted the announce, but I don't
> > see why rte_vdpa_close() would be needed. It seems the only user of
> > rte_vdpa_get_rte_device() is the internal vDPA example, and it only use
> > it to get and print the device name.
> 
> You're right, it was an oversight.
> So we need only to get the rte_device name.
> 
> I propose to replace
> 	struct rte_device *rte_vdpa_get_rte_device(struct rte_vdpa_device *vdpa_dev);
> with
> 	const char *rte_vdpa_get_name(void);

sorry, I missed a parameter :)
It would be:
	const char *rte_vdpa_get_name(struct rte_vdpa_device *vdpa_dev);

Or do you prefer "rte_vdpa_get_device_name"?

Re: [PATCH] doc: announce transition to vDPA port close function

[Maxime Coquelin]

On 7/12/22 14:28, Thomas Monjalon wrote:
> 12/07/2022 14:26, Thomas Monjalon:
>> 12/07/2022 11:16, Maxime Coquelin:
>>>
>>> On 5/18/21 09:34, Thomas Monjalon wrote:
>>>> There is a layer violation in the vDPA API which encourages to destroy
>>>> a full device with rte_dev_remove() instead of just closing the port.
>>>> The plan is to introduce a new function in 21.08, promote in 21.11,
>>>> and deprecate rte_vdpa_get_rte_device() in 21.11.
>> [...]
>>>> +* vdpa: The vDPA API should not try to manipulate or export
>>>> +  any ``rte_device`` object, which belongs to the bus layer.
>>>> +  The function ``rte_vdpa_get_rte_device()`` will be deprecated in 21.11,
>>>> +  when its usage will be replaced with a function ``rte_vdpa_close()``.
>>>> +  The new function should enter in 21.08 and get promoted to stable in 21.11.
>>>> +  A port close function will allow to close a single port without destroying
>>>> +  the rest of the device.
>>>
>>> Maybe there was some changes since you posted the announce, but I don't
>>> see why rte_vdpa_close() would be needed. It seems the only user of
>>> rte_vdpa_get_rte_device() is the internal vDPA example, and it only use
>>> it to get and print the device name.
>>
>> You're right, it was an oversight.
>> So we need only to get the rte_device name.
>>
>> I propose to replace
>> 	struct rte_device *rte_vdpa_get_rte_device(struct rte_vdpa_device *vdpa_dev);
>> with
>> 	const char *rte_vdpa_get_name(void);
> 
> sorry, I missed a parameter :)
> It would be:
> 	const char *rte_vdpa_get_name(struct rte_vdpa_device *vdpa_dev);
> 
> Or do you prefer "rte_vdpa_get_device_name"?
> 
> 
> 
rte_vdpa_get_device_name may be prefered to avoid confusion with the
socket name.

Thanks,
Maxime

[PATCH v2] doc: announce transition to vDPA device name function

[Thomas Monjalon]

There is a layer violation in the vDPA API for getting the device name.
Instead of providing the name at vDPA level,
a function returns the low-level device object.
The plan is to introduce a new function in 22.11, promote in 23.07,
and remove rte_vdpa_get_rte_device() in 23.11.

Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
---
v2: one year passed, update with a new plan
    v1 was proposing a close function, only device name is needed
---
 doc/guides/rel_notes/deprecation.rst | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 4e5b23c53d..0f7fefbf6a 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -107,6 +107,13 @@ Deprecation Notices
   alternative is implemented.
   The legacy actions should be removed in DPDK 22.11.
 
+* vdpa: The vDPA API should not try to manipulate or export
+  any ``rte_device`` object, which belongs to the bus layer.
+  The function ``rte_vdpa_get_rte_device()`` will be deprecated in 23.07,
+  when its usage will be replaced with ``rte_vdpa_get_device_name()``.
+  The new function should enter in 22.11 and get promoted to stable in 23.07.
+  The target is to remove ``rte_vdpa_get_rte_device()`` in 23.11.
+
 * cryptodev: Hide structures ``rte_cryptodev_sym_session`` and
   ``rte_cryptodev_asym_session`` to remove unnecessary indirection between
   session and the private data of session. An opaque pointer can be exposed
-- 
2.36.1

Re: [PATCH v2] doc: announce transition to vDPA device name function

[Maxime Coquelin]

On 7/12/22 15:36, Thomas Monjalon wrote:
> There is a layer violation in the vDPA API for getting the device name.
> Instead of providing the name at vDPA level,
> a function returns the low-level device object.
> The plan is to introduce a new function in 22.11, promote in 23.07,
> and remove rte_vdpa_get_rte_device() in 23.11.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
> v2: one year passed, update with a new plan
>      v1 was proposing a close function, only device name is needed
> ---
>   doc/guides/rel_notes/deprecation.rst | 7 +++++++
>   1 file changed, 7 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index 4e5b23c53d..0f7fefbf6a 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -107,6 +107,13 @@ Deprecation Notices
>     alternative is implemented.
>     The legacy actions should be removed in DPDK 22.11.
>   
> +* vdpa: The vDPA API should not try to manipulate or export
> +  any ``rte_device`` object, which belongs to the bus layer.
> +  The function ``rte_vdpa_get_rte_device()`` will be deprecated in 23.07,
> +  when its usage will be replaced with ``rte_vdpa_get_device_name()``.
> +  The new function should enter in 22.11 and get promoted to stable in 23.07.
> +  The target is to remove ``rte_vdpa_get_rte_device()`` in 23.11.
> +
>   * cryptodev: Hide structures ``rte_cryptodev_sym_session`` and
>     ``rte_cryptodev_asym_session`` to remove unnecessary indirection between
>     session and the private data of session. An opaque pointer can be exposed

Acked-by: Maxime Coquelin <maxime.coquelin@redhat.com>

Thanks,
Maxime

RE: [PATCH v2] doc: announce transition to vDPA device name function

[Matan Azrad]

From: Thomas Monjalon 
> There is a layer violation in the vDPA API for getting the device name.
> Instead of providing the name at vDPA level, a function returns the low-level
> device object.
> The plan is to introduce a new function in 22.11, promote in 23.07, and remove
> rte_vdpa_get_rte_device() in 23.11.
> 
> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
> ---
> v2: one year passed, update with a new plan
>     v1 was proposing a close function, only device name is needed
> ---
>  doc/guides/rel_notes/deprecation.rst | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst
> b/doc/guides/rel_notes/deprecation.rst
> index 4e5b23c53d..0f7fefbf6a 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -107,6 +107,13 @@ Deprecation Notices
>    alternative is implemented.
>    The legacy actions should be removed in DPDK 22.11.
> 
> +* vdpa: The vDPA API should not try to manipulate or export
> +  any ``rte_device`` object, which belongs to the bus layer.
> +  The function ``rte_vdpa_get_rte_device()`` will be deprecated in
> +23.07,
> +  when its usage will be replaced with ``rte_vdpa_get_device_name()``.
> +  The new function should enter in 22.11 and get promoted to stable in 23.07.
> +  The target is to remove ``rte_vdpa_get_rte_device()`` in 23.11.
> +
>  * cryptodev: Hide structures ``rte_cryptodev_sym_session`` and
>    ``rte_cryptodev_asym_session`` to remove unnecessary indirection between
>    session and the private data of session. An opaque pointer can be exposed
> --
> 2.36.1

Acked-by: Matan Azrad <matan@nvidia.com>

Re: [PATCH v2] doc: announce transition to vDPA device name function

[David Marchand]

On Tue, Jul 12, 2022 at 3:36 PM Thomas Monjalon <thomas@monjalon.net> wrote:
>
> There is a layer violation in the vDPA API for getting the device name.
> Instead of providing the name at vDPA level,
> a function returns the low-level device object.

Exposing a rte_device (as an opaque pointer) in upper device classes
seems a good thing to me.
With the API rework I proposed, we will have accessors to get this
object characteristics (like here, a name identifying it).
Having the vDPA API returns a pointer to a rte_device object makes it
possible to reuse those accessors, nothing more needed.

If the rte_device object is extended in any (unforeseen at the moment)
way in the future, it would still be a matter of using the relevant
accessor.
No update needed in the vDPA API at this point in the future.


On the other hand, what you propose here seems to go the other way.
With each device classes needing to expose, through its own means /
API, the underlying rte_device characteristics.


-- 
David Marchand

Re: [PATCH v2] doc: announce transition to vDPA device name function

[Thomas Monjalon]

13/07/2022 15:26, David Marchand:
> On Tue, Jul 12, 2022 at 3:36 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> >
> > There is a layer violation in the vDPA API for getting the device name.
> > Instead of providing the name at vDPA level,
> > a function returns the low-level device object.
> 
> Exposing a rte_device (as an opaque pointer) in upper device classes
> seems a good thing to me.
> With the API rework I proposed, we will have accessors to get this
> object characteristics (like here, a name identifying it).
> Having the vDPA API returns a pointer to a rte_device object makes it
> possible to reuse those accessors, nothing more needed.
> 
> If the rte_device object is extended in any (unforeseen at the moment)
> way in the future, it would still be a matter of using the relevant
> accessor.
> No update needed in the vDPA API at this point in the future.
> 
> 
> On the other hand, what you propose here seems to go the other way.
> With each device classes needing to expose, through its own means /
> API, the underlying rte_device characteristics.

I realize we don't have a guideline about this API aspect:
should we expose underlying handles?

I understand your point and I think it is valid.
I suggest cancelling this deprecation for now.
Once we have a guideline agreed in doc/guides/contributing/design.rst
we can revisit some API.

For instance, I've avoided to expose rte_device in gpudev,
and the NUMA socket is directly reported in gpudev infos.
Another approach would be to expose the pointer to rte_device.