[v2] doc: plan splitting the ethdev ops struct
diff mbox series

Message ID 20200225124431.4088444-1-ferruh.yigit@intel.com
State Superseded
Delegated to: Thomas Monjalon
Headers show
Series
  • [v2] doc: plan splitting the ethdev ops struct
Related show

Checks

Context Check Description
ci/travis-robot success Travis build: passed
ci/Intel-compilation success Compilation OK
ci/checkpatch success coding style OK

Commit Message

Ferruh Yigit Feb. 25, 2020, 12:44 p.m. UTC
For the ABI compatibility it is better to hide internal data structures
from the application as much as possible. But because of some inline
functions 'struct eth_dev_ops' can't be hidden completely.

Plan is to split the 'struct eth_dev_ops' into two as ones used by
inline functions and ones not used, and hide the second part that not
used by inline functions completely to the application.

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
Acked-by: Jerin Jacob <jerinj@marvell.com>
---
Cc: David Marchand <david.marchand@redhat.com>
Cc: Thomas Monjalon <thomas@monjalon.net>
Cc: Andrew Rybchenko <arybchenko@solarflare.com>

v2:
* Add target date for the work
* Give more detail on what will be done
---
 doc/guides/rel_notes/deprecation.rst | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Andrew Rybchenko Feb. 25, 2020, 3:51 p.m. UTC | #1
On 2/25/20 3:44 PM, Ferruh Yigit wrote:
> For the ABI compatibility it is better to hide internal data structures
> from the application as much as possible. But because of some inline
> functions 'struct eth_dev_ops' can't be hidden completely.
>
> Plan is to split the 'struct eth_dev_ops' into two as ones used by
> inline functions and ones not used, and hide the second part that not
> used by inline functions completely to the application.
>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> Acked-by: Jerin Jacob <jerinj@marvell.com>
> ---
> Cc: David Marchand <david.marchand@redhat.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>
> Cc: Andrew Rybchenko <arybchenko@solarflare.com>
>
> v2:
> * Add target date for the work
> * Give more detail on what will be done
> ---
>  doc/guides/rel_notes/deprecation.rst | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index 99d81564a..ff612a615 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -86,6 +86,17 @@ Deprecation Notices
>    In 19.11 PMDs will still update the field even when the offload is not
>    enabled.
>  
> +* ethdev: Split the ``struct eth_dev_ops`` struct to hide it as much as possible.
> +  Currently the ``struct eth_dev_ops`` struct is accessible by the application
> +  because some inline functions, like ``rte_eth_tx_descriptor_status()``,
> +  access the struct directly. The struct will be separate in two, the ops used
> +  by inline functions still will be accessible to user but rest will be hidden.
> +  Initial split will be done in 20.05 with adding reserved fields for the struct
> +  used by inline functions, and by putting new struct reference into public one
> +  to not increase the size of ``struct rte_eth_dev``, proper split will be done
> +  in 20.11 by moving inline dev_ops function to next to Rx/Tx burst functions and
> +  hiding rest.
> +
>  * cryptodev: support for using IV with all sizes is added, J0 still can
>    be used but only when IV length in following structs ``rte_crypto_auth_xform``,
>    ``rte_crypto_aead_xform`` is set to zero. When IV length is greater or equal

I'd like to understand why do we need an intermediate solution first.
Also rereading above few times I've failed to fully understand what
will be done and why it does not break API/ABI.
Ferruh Yigit Feb. 25, 2020, 4:13 p.m. UTC | #2
On 2/25/2020 3:51 PM, Andrew Rybchenko wrote:
> On 2/25/20 3:44 PM, Ferruh Yigit wrote:
>> For the ABI compatibility it is better to hide internal data structures
>> from the application as much as possible. But because of some inline
>> functions 'struct eth_dev_ops' can't be hidden completely.
>>
>> Plan is to split the 'struct eth_dev_ops' into two as ones used by
>> inline functions and ones not used, and hide the second part that not
>> used by inline functions completely to the application.
>>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>> Acked-by: Jerin Jacob <jerinj@marvell.com>
>> ---
>> Cc: David Marchand <david.marchand@redhat.com>
>> Cc: Thomas Monjalon <thomas@monjalon.net>
>> Cc: Andrew Rybchenko <arybchenko@solarflare.com>
>>
>> v2:
>> * Add target date for the work
>> * Give more detail on what will be done
>> ---
>>  doc/guides/rel_notes/deprecation.rst | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
>> index 99d81564a..ff612a615 100644
>> --- a/doc/guides/rel_notes/deprecation.rst
>> +++ b/doc/guides/rel_notes/deprecation.rst
>> @@ -86,6 +86,17 @@ Deprecation Notices
>>    In 19.11 PMDs will still update the field even when the offload is not
>>    enabled.
>>  
>> +* ethdev: Split the ``struct eth_dev_ops`` struct to hide it as much as possible.
>> +  Currently the ``struct eth_dev_ops`` struct is accessible by the application
>> +  because some inline functions, like ``rte_eth_tx_descriptor_status()``,
>> +  access the struct directly. The struct will be separate in two, the ops used
>> +  by inline functions still will be accessible to user but rest will be hidden.
>> +  Initial split will be done in 20.05 with adding reserved fields for the struct
>> +  used by inline functions, and by putting new struct reference into public one
>> +  to not increase the size of ``struct rte_eth_dev``, proper split will be done
>> +  in 20.11 by moving inline dev_ops function to next to Rx/Tx burst functions and
>> +  hiding rest.
>> +
>>  * cryptodev: support for using IV with all sizes is added, J0 still can
>>    be used but only when IV length in following structs ``rte_crypto_auth_xform``,
>>    ``rte_crypto_aead_xform`` is set to zero. When IV length is greater or equal
> 
> I'd like to understand why do we need an intermediate solution first.

We don't need it really, we may prefer to postpone the update to 20.11.

> Also rereading above few times I've failed to fully understand what
> will be done and why it does not break API/ABI.
> 

Agree it wasn't very clear, but I mean something like below, I hope it clarifies:

Previously:

struct rte_eth_dev

+--------+
|        |    *dev_ops    struct eth_dev_ops
|        |
|        +-------------->+--------+
|        |               |        |
|        |               |        |
|        |               |        |
|        |               |        |
+--------+               |        |
                         |        |
                         |        |
                         |        |
                         +--------+


Proposed:

struct rte_eth_dev

+--------+
|        |                struct eth_dev_ops
|        |
|*dev_ops+-------------->+--------+
|        |               |        |
|        |               | Reserv |
|        |               |        |
|        |               |        |
+--------+               |        |
                         +--------+
                         | inline |
                         | de^_ops|               struct eth_dev_ops
                         +--------+
                         | *priv  +------------->+--------+
                         +--------+              |        |
                                                 |        |
                                                 |        |
                                                 |        |
                                                 |        |
                                                 |        |
                                                 |        |
                                                 |        |
                                                 +--------+

This is only to keep ABI compatibility [1] while separating the struct.

[1]
- The offset of some functions in the dev_ops struct should be same
- The size of the "struct rte_eth_dev" should be same
Andrew Rybchenko Feb. 25, 2020, 4:41 p.m. UTC | #3
On 2/25/20 7:13 PM, Ferruh Yigit wrote:
> On 2/25/2020 3:51 PM, Andrew Rybchenko wrote:
>> On 2/25/20 3:44 PM, Ferruh Yigit wrote:
>>> For the ABI compatibility it is better to hide internal data structures
>>> from the application as much as possible. But because of some inline
>>> functions 'struct eth_dev_ops' can't be hidden completely.
>>>
>>> Plan is to split the 'struct eth_dev_ops' into two as ones used by
>>> inline functions and ones not used, and hide the second part that not
>>> used by inline functions completely to the application.
>>>
>>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>>> Acked-by: Jerin Jacob <jerinj@marvell.com>
>>> ---
>>> Cc: David Marchand <david.marchand@redhat.com>
>>> Cc: Thomas Monjalon <thomas@monjalon.net>
>>> Cc: Andrew Rybchenko <arybchenko@solarflare.com>
>>>
>>> v2:
>>> * Add target date for the work
>>> * Give more detail on what will be done
>>> ---
>>>  doc/guides/rel_notes/deprecation.rst | 11 +++++++++++
>>>  1 file changed, 11 insertions(+)
>>>
>>> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
>>> index 99d81564a..ff612a615 100644
>>> --- a/doc/guides/rel_notes/deprecation.rst
>>> +++ b/doc/guides/rel_notes/deprecation.rst
>>> @@ -86,6 +86,17 @@ Deprecation Notices
>>>    In 19.11 PMDs will still update the field even when the offload is not
>>>    enabled.
>>>  
>>> +* ethdev: Split the ``struct eth_dev_ops`` struct to hide it as much as possible.
>>> +  Currently the ``struct eth_dev_ops`` struct is accessible by the application
>>> +  because some inline functions, like ``rte_eth_tx_descriptor_status()``,
>>> +  access the struct directly. The struct will be separate in two, the ops used
>>> +  by inline functions still will be accessible to user but rest will be hidden.
>>> +  Initial split will be done in 20.05 with adding reserved fields for the struct
>>> +  used by inline functions, and by putting new struct reference into public one
>>> +  to not increase the size of ``struct rte_eth_dev``, proper split will be done
>>> +  in 20.11 by moving inline dev_ops function to next to Rx/Tx burst functions and
>>> +  hiding rest.
>>> +
>>>  * cryptodev: support for using IV with all sizes is added, J0 still can
>>>    be used but only when IV length in following structs ``rte_crypto_auth_xform``,
>>>    ``rte_crypto_aead_xform`` is set to zero. When IV length is greater or equal
>>
>> I'd like to understand why do we need an intermediate solution first.
> 
> We don't need it really, we may prefer to postpone the update to 20.11.
> 
>> Also rereading above few times I've failed to fully understand what
>> will be done and why it does not break API/ABI.
>>
> 
> Agree it wasn't very clear, but I mean something like below, I hope it clarifies:
> 
> Previously:
> 
> struct rte_eth_dev
> 
> +--------+
> |        |    *dev_ops    struct eth_dev_ops
> |        |
> |        +-------------->+--------+
> |        |               |        |
> |        |               |        |
> |        |               |        |
> |        |               |        |
> +--------+               |        |
>                          |        |
>                          |        |
>                          |        |
>                          +--------+
> 
> 
> Proposed:
> 
> struct rte_eth_dev
> 
> +--------+
> |        |                struct eth_dev_ops
> |        |
> |*dev_ops+-------------->+--------+
> |        |               |        |
> |        |               | Reserv |
> |        |               |        |
> |        |               |        |
> +--------+               |        |
>                          +--------+
>                          | inline |
>                          | de^_ops|               struct eth_dev_ops
>                          +--------+
>                          | *priv  +------------->+--------+
>                          +--------+              |        |
>                                                  |        |
>                                                  |        |
>                                                  |        |
>                                                  |        |
>                                                  |        |
>                                                  |        |
>                                                  |        |
>                                                  +--------+
> 
> This is only to keep ABI compatibility [1] while separating the struct.
> 
> [1]
> - The offset of some functions in the dev_ops struct should be same
> - The size of the "struct rte_eth_dev" should be same
> 

OK, got it. Many thanks for explanations.
David Marchand Feb. 25, 2020, 6:13 p.m. UTC | #4
On Tue, Feb 25, 2020 at 1:44 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>
> For the ABI compatibility it is better to hide internal data structures
> from the application as much as possible. But because of some inline
> functions 'struct eth_dev_ops' can't be hidden completely.
>
> Plan is to split the 'struct eth_dev_ops' into two as ones used by
> inline functions and ones not used, and hide the second part that not
> used by inline functions completely to the application.
>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> Acked-by: Jerin Jacob <jerinj@marvell.com>

Sorry, this patch is missing the necessary acks for merging.
This change can still be announced for 20.11 in the next releases and
the details can be discussed again since a conclusion was not reached.


Thanks.
Ferruh Yigit Feb. 25, 2020, 6:18 p.m. UTC | #5
On 2/25/2020 6:13 PM, David Marchand wrote:
> On Tue, Feb 25, 2020 at 1:44 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>
>> For the ABI compatibility it is better to hide internal data structures
>> from the application as much as possible. But because of some inline
>> functions 'struct eth_dev_ops' can't be hidden completely.
>>
>> Plan is to split the 'struct eth_dev_ops' into two as ones used by
>> inline functions and ones not used, and hide the second part that not
>> used by inline functions completely to the application.
>>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>> Acked-by: Jerin Jacob <jerinj@marvell.com>
> 
> Sorry, this patch is missing the necessary acks for merging.
> This change can still be announced for 20.11 in the next releases and
> the details can be discussed again since a conclusion was not reached.
> 
> 

That is OK, we can drop for 20.02, I can send a new version to target the 20.11
change only removing interim change.

Patch
diff mbox series

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 99d81564a..ff612a615 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -86,6 +86,17 @@  Deprecation Notices
   In 19.11 PMDs will still update the field even when the offload is not
   enabled.
 
+* ethdev: Split the ``struct eth_dev_ops`` struct to hide it as much as possible.
+  Currently the ``struct eth_dev_ops`` struct is accessible by the application
+  because some inline functions, like ``rte_eth_tx_descriptor_status()``,
+  access the struct directly. The struct will be separate in two, the ops used
+  by inline functions still will be accessible to user but rest will be hidden.
+  Initial split will be done in 20.05 with adding reserved fields for the struct
+  used by inline functions, and by putting new struct reference into public one
+  to not increase the size of ``struct rte_eth_dev``, proper split will be done
+  in 20.11 by moving inline dev_ops function to next to Rx/Tx burst functions and
+  hiding rest.
+
 * cryptodev: support for using IV with all sizes is added, J0 still can
   be used but only when IV length in following structs ``rte_crypto_auth_xform``,
   ``rte_crypto_aead_xform`` is set to zero. When IV length is greater or equal