DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] doc: plan splitting the ethdev ops struct
@ 2020-02-17 15:38 Ferruh Yigit
  2020-02-18  5:07 ` Jerin Jacob
                   ` (2 more replies)
  0 siblings, 3 replies; 21+ messages in thread
From: Ferruh Yigit @ 2020-02-17 15:38 UTC (permalink / raw)
  To: Neil Horman, John McNamara, Marko Kovacevic
  Cc: dev, Ferruh Yigit, David Marchand, Thomas Monjalon, Andrew Rybchenko

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>
---
Cc: David Marchand <david.marchand@redhat.com>
Cc: Thomas Monjalon <thomas@monjalon.net>
Cc: Andrew Rybchenko <arybchenko@solarflare.com>
---
 doc/guides/rel_notes/deprecation.rst | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index dfcca87ab..2aa431028 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -72,6 +72,12 @@ 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.
+
 * 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
-- 
2.24.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH] doc: plan splitting the ethdev ops struct
  2020-02-17 15:38 [dpdk-dev] [PATCH] doc: plan splitting the ethdev ops struct Ferruh Yigit
@ 2020-02-18  5:07 ` Jerin Jacob
  2020-02-25 12:42   ` Ferruh Yigit
  2020-05-26 13:01   ` Thomas Monjalon
  2020-02-18  6:01 ` Stephen Hemminger
  2020-02-25 12:44 ` [dpdk-dev] [PATCH v2] " Ferruh Yigit
  2 siblings, 2 replies; 21+ messages in thread
From: Jerin Jacob @ 2020-02-18  5:07 UTC (permalink / raw)
  To: Ferruh Yigit
  Cc: Neil Horman, John McNamara, Marko Kovacevic, dpdk-dev,
	David Marchand, Thomas Monjalon, Andrew Rybchenko

On Mon, Feb 17, 2020 at 9:08 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.

It is a good improvement.  IMO, If anything used in fast-path it
should be in ``struct rte_eth_dev``
and rest can completely be moved to internal. In this case, if
`rte_eth_tx_descriptor_status`
not used on fastpath, Maybe we don't need to maintain the inline
status and move completely
to .c file.

Those may be specifics of the work. In general, this change looks good to me.

Acked-by: Jerin Jacob <jerinj@marvell.com>





>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
> Cc: David Marchand <david.marchand@redhat.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>
> Cc: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
>  doc/guides/rel_notes/deprecation.rst | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index dfcca87ab..2aa431028 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -72,6 +72,12 @@ 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.
> +
>  * 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
> --
> 2.24.1
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH] doc: plan splitting the ethdev ops struct
  2020-02-17 15:38 [dpdk-dev] [PATCH] doc: plan splitting the ethdev ops struct Ferruh Yigit
  2020-02-18  5:07 ` Jerin Jacob
@ 2020-02-18  6:01 ` Stephen Hemminger
  2020-02-21 10:40   ` Ferruh Yigit
  2020-02-25 12:44 ` [dpdk-dev] [PATCH v2] " Ferruh Yigit
  2 siblings, 1 reply; 21+ messages in thread
From: Stephen Hemminger @ 2020-02-18  6:01 UTC (permalink / raw)
  To: Ferruh Yigit
  Cc: Neil Horman, John McNamara, Marko Kovacevic, dev, David Marchand,
	Thomas Monjalon, Andrew Rybchenko

On Mon, 17 Feb 2020 15:38:05 +0000
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>
> ---
> Cc: David Marchand <david.marchand@redhat.com>
> Cc: Thomas Monjalon <thomas@monjalon.net>
> Cc: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
>  doc/guides/rel_notes/deprecation.rst | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> index dfcca87ab..2aa431028 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -72,6 +72,12 @@ 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.
> +
>  * 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

Good luck, truely hiding internals is hard. The mbuf structure is already split but not really
hidden at all (just look at dwarf output). It doesn't make sense to do it unless
you can really hide it.

I would attack the rte_device stuff first. Make rte_device opaque to the application
that would help for future versions. Then work backwards to rte_tehtdev.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH] doc: plan splitting the ethdev ops struct
  2020-02-18  6:01 ` Stephen Hemminger
@ 2020-02-21 10:40   ` Ferruh Yigit
  2020-02-25 10:35     ` Andrew Rybchenko
  0 siblings, 1 reply; 21+ messages in thread
From: Ferruh Yigit @ 2020-02-21 10:40 UTC (permalink / raw)
  To: Stephen Hemminger
  Cc: Neil Horman, John McNamara, Marko Kovacevic, dev, David Marchand,
	Thomas Monjalon, Andrew Rybchenko

On 2/18/2020 6:01 AM, Stephen Hemminger wrote:
> On Mon, 17 Feb 2020 15:38:05 +0000
> 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>
>> ---
>> Cc: David Marchand <david.marchand@redhat.com>
>> Cc: Thomas Monjalon <thomas@monjalon.net>
>> Cc: Andrew Rybchenko <arybchenko@solarflare.com>
>> ---
>>  doc/guides/rel_notes/deprecation.rst | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
>> index dfcca87ab..2aa431028 100644
>> --- a/doc/guides/rel_notes/deprecation.rst
>> +++ b/doc/guides/rel_notes/deprecation.rst
>> @@ -72,6 +72,12 @@ 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.
>> +
>>  * 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
> 
> Good luck, truely hiding internals is hard. The mbuf structure is already split but not really
> hidden at all (just look at dwarf output). It doesn't make sense to do it unless
> you can really hide it.

I believe this can be done, only following [1] dev_ops are used by inline
functions, rest can be moved into separate struct and moved into ethdev driver
looking header.

[1]
rx_queue_count
rx_descriptor_done
rx_descriptor_status
tx_descriptor_status


> 
> I would attack the rte_device stuff first. Make rte_device opaque to the application
> that would help for future versions. Then work backwards to rte_tehtdev.
>

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH] doc: plan splitting the ethdev ops struct
  2020-02-21 10:40   ` Ferruh Yigit
@ 2020-02-25 10:35     ` Andrew Rybchenko
  2020-02-25 11:07       ` Ananyev, Konstantin
  2020-02-25 12:28       ` Ferruh Yigit
  0 siblings, 2 replies; 21+ messages in thread
From: Andrew Rybchenko @ 2020-02-25 10:35 UTC (permalink / raw)
  To: Ferruh Yigit, Stephen Hemminger
  Cc: Neil Horman, John McNamara, Marko Kovacevic, dev, David Marchand,
	Thomas Monjalon

On 2/21/20 1:40 PM, Ferruh Yigit wrote:
> On 2/18/2020 6:01 AM, Stephen Hemminger wrote:
>> On Mon, 17 Feb 2020 15:38:05 +0000
>> 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>
>>> ---
>>> Cc: David Marchand <david.marchand@redhat.com>
>>> Cc: Thomas Monjalon <thomas@monjalon.net>
>>> Cc: Andrew Rybchenko <arybchenko@solarflare.com>
>>> ---
>>>  doc/guides/rel_notes/deprecation.rst | 6 ++++++
>>>  1 file changed, 6 insertions(+)
>>>
>>> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
>>> index dfcca87ab..2aa431028 100644
>>> --- a/doc/guides/rel_notes/deprecation.rst
>>> +++ b/doc/guides/rel_notes/deprecation.rst
>>> @@ -72,6 +72,12 @@ 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.
>>> +
>>>  * 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
>>
>> Good luck, truely hiding internals is hard. The mbuf structure is already split but not really
>> hidden at all (just look at dwarf output). It doesn't make sense to do it unless
>> you can really hide it.
> 
> I believe this can be done, only following [1] dev_ops are used by inline
> functions, rest can be moved into separate struct and moved into ethdev driver
> looking header.
> 
> [1]
> rx_queue_count
> rx_descriptor_done
> rx_descriptor_status
> tx_descriptor_status

I think having 3 places (if I understand the intention
correctly) with ethdev callbacks is too much. So, I think
that these ops should be simply moved to nearby Tx/Rx
burst and Tx prepare callbacks (e.g. move into inline_ops
structure which is located at the beginning of rte_eth_dev
in order to preserve 3 existing callback location).

Also I'd consider to deprecate and remove rx_queue_count
and rx_descriptor_done.

>>
>> I would attack the rte_device stuff first. Make rte_device opaque to the application
>> that would help for future versions. Then work backwards to rte_tehtdev.
>>


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH] doc: plan splitting the ethdev ops struct
  2020-02-25 10:35     ` Andrew Rybchenko
@ 2020-02-25 11:07       ` Ananyev, Konstantin
  2020-02-25 11:19         ` Andrew Rybchenko
  2020-02-25 12:28       ` Ferruh Yigit
  1 sibling, 1 reply; 21+ messages in thread
From: Ananyev, Konstantin @ 2020-02-25 11:07 UTC (permalink / raw)
  To: Andrew Rybchenko, Yigit, Ferruh, Stephen Hemminger
  Cc: Neil Horman, Mcnamara, John, Kovacevic, Marko, dev,
	David Marchand, Thomas Monjalon


Hi Andew, 

> On 2/21/20 1:40 PM, Ferruh Yigit wrote:
> > On 2/18/2020 6:01 AM, Stephen Hemminger wrote:
> >> On Mon, 17 Feb 2020 15:38:05 +0000
> >> 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>
> >>> ---
> >>> Cc: David Marchand <david.marchand@redhat.com>
> >>> Cc: Thomas Monjalon <thomas@monjalon.net>
> >>> Cc: Andrew Rybchenko <arybchenko@solarflare.com>
> >>> ---
> >>>  doc/guides/rel_notes/deprecation.rst | 6 ++++++
> >>>  1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
> >>> index dfcca87ab..2aa431028 100644
> >>> --- a/doc/guides/rel_notes/deprecation.rst
> >>> +++ b/doc/guides/rel_notes/deprecation.rst
> >>> @@ -72,6 +72,12 @@ 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.
> >>> +
> >>>  * 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
> >>
> >> Good luck, truely hiding internals is hard. The mbuf structure is already split but not really
> >> hidden at all (just look at dwarf output). It doesn't make sense to do it unless
> >> you can really hide it.
> >
> > I believe this can be done, only following [1] dev_ops are used by inline
> > functions, rest can be moved into separate struct and moved into ethdev driver
> > looking header.
> >
> > [1]
> > rx_queue_count
> > rx_descriptor_done
> > rx_descriptor_status
> > tx_descriptor_status
> 
> I think having 3 places (if I understand the intention
> correctly) with ethdev callbacks is too much. So, I think
> that these ops should be simply moved to nearby Tx/Rx
> burst and Tx prepare callbacks (e.g. move into inline_ops
> structure which is located at the beginning of rte_eth_dev
> in order to preserve 3 existing callback location).

If you are going to change ABI anyway,  would it be worth to consider 
moving rx/tx burst/prepare functions to be per queue,
instead of per device? 

> 
> Also I'd consider to deprecate and remove rx_queue_count
> and rx_descriptor_done.
> 
> >>
> >> I would attack the rte_device stuff first. Make rte_device opaque to the application
> >> that would help for future versions. Then work backwards to rte_tehtdev.
> >>


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH] doc: plan splitting the ethdev ops struct
  2020-02-25 11:07       ` Ananyev, Konstantin
@ 2020-02-25 11:19         ` Andrew Rybchenko
  0 siblings, 0 replies; 21+ messages in thread
From: Andrew Rybchenko @ 2020-02-25 11:19 UTC (permalink / raw)
  To: Ananyev, Konstantin, Yigit, Ferruh, Stephen Hemminger
  Cc: Neil Horman, Mcnamara, John, Kovacevic, Marko, dev,
	David Marchand, Thomas Monjalon

Hi Konstantin,

On 2/25/20 2:07 PM, Ananyev, Konstantin wrote:
> Hi Andew, 
>
>> On 2/21/20 1:40 PM, Ferruh Yigit wrote:
>>> On 2/18/2020 6:01 AM, Stephen Hemminger wrote:
>>>> On Mon, 17 Feb 2020 15:38:05 +0000
>>>> 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>
>>>>> ---
>>>>> Cc: David Marchand <david.marchand@redhat.com>
>>>>> Cc: Thomas Monjalon <thomas@monjalon.net>
>>>>> Cc: Andrew Rybchenko <arybchenko@solarflare.com>
>>>>> ---
>>>>>  doc/guides/rel_notes/deprecation.rst | 6 ++++++
>>>>>  1 file changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
>>>>> index dfcca87ab..2aa431028 100644
>>>>> --- a/doc/guides/rel_notes/deprecation.rst
>>>>> +++ b/doc/guides/rel_notes/deprecation.rst
>>>>> @@ -72,6 +72,12 @@ 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.
>>>>> +
>>>>>  * 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
>>>> Good luck, truely hiding internals is hard. The mbuf structure is already split but not really
>>>> hidden at all (just look at dwarf output). It doesn't make sense to do it unless
>>>> you can really hide it.
>>> I believe this can be done, only following [1] dev_ops are used by inline
>>> functions, rest can be moved into separate struct and moved into ethdev driver
>>> looking header.
>>>
>>> [1]
>>> rx_queue_count
>>> rx_descriptor_done
>>> rx_descriptor_status
>>> tx_descriptor_status
>> I think having 3 places (if I understand the intention
>> correctly) with ethdev callbacks is too much. So, I think
>> that these ops should be simply moved to nearby Tx/Rx
>> burst and Tx prepare callbacks (e.g. move into inline_ops
>> structure which is located at the beginning of rte_eth_dev
>> in order to preserve 3 existing callback location).
> If you are going to change ABI anyway,  would it be worth to consider 
> moving rx/tx burst/prepare functions to be per queue,
> instead of per device? 

I'm thinking about it from time to time. In general I like the idea.
The only question I have if there is a demand for queues with
different offloads which could result in different Rx/Tx callbacks.
Also I'm not sure that it is doable without performance degradation
at all, but hopefully it will be tiny.

>> Also I'd consider to deprecate and remove rx_queue_count
>> and rx_descriptor_done.
>>
>>>> I would attack the rte_device stuff first. Make rte_device opaque to the application
>>>> that would help for future versions. Then work backwards to rte_tehtdev.
>>>>


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH] doc: plan splitting the ethdev ops struct
  2020-02-25 10:35     ` Andrew Rybchenko
  2020-02-25 11:07       ` Ananyev, Konstantin
@ 2020-02-25 12:28       ` Ferruh Yigit
  1 sibling, 0 replies; 21+ messages in thread
From: Ferruh Yigit @ 2020-02-25 12:28 UTC (permalink / raw)
  To: Andrew Rybchenko, Stephen Hemminger
  Cc: Neil Horman, John McNamara, Marko Kovacevic, dev, David Marchand,
	Thomas Monjalon, Kinsella, Ray, Olivier MATZ

On 2/25/2020 10:35 AM, Andrew Rybchenko wrote:
> On 2/21/20 1:40 PM, Ferruh Yigit wrote:
>> On 2/18/2020 6:01 AM, Stephen Hemminger wrote:
>>> On Mon, 17 Feb 2020 15:38:05 +0000
>>> 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>
>>>> ---
>>>> Cc: David Marchand <david.marchand@redhat.com>
>>>> Cc: Thomas Monjalon <thomas@monjalon.net>
>>>> Cc: Andrew Rybchenko <arybchenko@solarflare.com>
>>>> ---
>>>>  doc/guides/rel_notes/deprecation.rst | 6 ++++++
>>>>  1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
>>>> index dfcca87ab..2aa431028 100644
>>>> --- a/doc/guides/rel_notes/deprecation.rst
>>>> +++ b/doc/guides/rel_notes/deprecation.rst
>>>> @@ -72,6 +72,12 @@ 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.
>>>> +
>>>>  * 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
>>>
>>> Good luck, truely hiding internals is hard. The mbuf structure is already split but not really
>>> hidden at all (just look at dwarf output). It doesn't make sense to do it unless
>>> you can really hide it.
>>
>> I believe this can be done, only following [1] dev_ops are used by inline
>> functions, rest can be moved into separate struct and moved into ethdev driver
>> looking header.
>>
>> [1]
>> rx_queue_count
>> rx_descriptor_done
>> rx_descriptor_status
>> tx_descriptor_status
> 
> I think having 3 places (if I understand the intention
> correctly) with ethdev callbacks is too much. 

Yes, this is the intention.

> So, I think
> that these ops should be simply moved to nearby Tx/Rx
> burst and Tx prepare callbacks (e.g. move into inline_ops
> structure which is located at the beginning of rte_eth_dev
> in order to preserve 3 existing callback location).

I think this is reasonable, but this can be done only in 20.11 with ABI break.

What do you think doing the initial hide in 20.05 with three places and do
proper splitting in 20.11 as suggested above.

Or it is possible to drop the interim work and do the all changes in 20.11, not
sure.


> 
> Also I'd consider to deprecate and remove rx_queue_count
> and rx_descriptor_done.

+1 to deprecate the 'rx_descriptor_done', we have already 'rx_descriptor_status'
& 'tx_descriptor_status' to replace the functionality.

@Thomas, @Ray, can you please remind how deprecation was done, is it marking
API, 'rte_eth_rx_descriptor_done', as deprecated in 20.11 and removing it in
21.11? If so I guess deprecation notice should be send before 20.11? (And make
sure all PMDs did the switch before 20.11)


Not sure about 'rx_queue_count', if any application may need it or not.

> 
>>>
>>> I would attack the rte_device stuff first. Make rte_device opaque to the application
>>> that would help for future versions. Then work backwards to rte_tehtdev.
>>>
> 


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH] doc: plan splitting the ethdev ops struct
  2020-02-18  5:07 ` Jerin Jacob
@ 2020-02-25 12:42   ` Ferruh Yigit
  2020-05-26 13:01   ` Thomas Monjalon
  1 sibling, 0 replies; 21+ messages in thread
From: Ferruh Yigit @ 2020-02-25 12:42 UTC (permalink / raw)
  To: Jerin Jacob
  Cc: Neil Horman, John McNamara, Marko Kovacevic, dpdk-dev,
	David Marchand, Thomas Monjalon, Andrew Rybchenko

On 2/18/2020 5:07 AM, Jerin Jacob wrote:
> On Mon, Feb 17, 2020 at 9:08 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.
> 
> It is a good improvement.  IMO, If anything used in fast-path it
> should be in ``struct rte_eth_dev``
> and rest can completely be moved to internal. In this case, if
> `rte_eth_tx_descriptor_status`
> not used on fastpath, Maybe we don't need to maintain the inline
> status and move completely
> to .c file.

Moving fast-past dev_ops to ``struct rte_eth_dev`` makes sense, also suggested
by Andrew, and will hide the rest.

Those inline dev_ops are datapath related so may be better to keep them as
inline, but not really sure about their users and how much they care about minor
performance impact if we chose to convert them into regular functions.

> 
> Those may be specifics of the work. In general, this change looks good to me.
> 
> Acked-by: Jerin Jacob <jerinj@marvell.com>
> 
> 
> 
> 
> 
>>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>> ---
>> Cc: David Marchand <david.marchand@redhat.com>
>> Cc: Thomas Monjalon <thomas@monjalon.net>
>> Cc: Andrew Rybchenko <arybchenko@solarflare.com>
>> ---
>>  doc/guides/rel_notes/deprecation.rst | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
>> index dfcca87ab..2aa431028 100644
>> --- a/doc/guides/rel_notes/deprecation.rst
>> +++ b/doc/guides/rel_notes/deprecation.rst
>> @@ -72,6 +72,12 @@ 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.
>> +
>>  * 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
>> --
>> 2.24.1
>>


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [dpdk-dev] [PATCH v2] doc: plan splitting the ethdev ops struct
  2020-02-17 15:38 [dpdk-dev] [PATCH] doc: plan splitting the ethdev ops struct Ferruh Yigit
  2020-02-18  5:07 ` Jerin Jacob
  2020-02-18  6:01 ` Stephen Hemminger
@ 2020-02-25 12:44 ` Ferruh Yigit
  2020-02-25 15:51   ` Andrew Rybchenko
                     ` (2 more replies)
  2 siblings, 3 replies; 21+ messages in thread
From: Ferruh Yigit @ 2020-02-25 12:44 UTC (permalink / raw)
  To: Neil Horman, John McNamara, Marko Kovacevic
  Cc: dev, Ferruh Yigit, Jerin Jacob, David Marchand, Thomas Monjalon,
	Andrew Rybchenko

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
-- 
2.24.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH v2] doc: plan splitting the ethdev ops struct
  2020-02-25 12:44 ` [dpdk-dev] [PATCH v2] " Ferruh Yigit
@ 2020-02-25 15:51   ` Andrew Rybchenko
  2020-02-25 16:13     ` Ferruh Yigit
  2020-02-25 18:13   ` David Marchand
  2020-03-04  9:57   ` [dpdk-dev] [PATCH v3] " Ferruh Yigit
  2 siblings, 1 reply; 21+ messages in thread
From: Andrew Rybchenko @ 2020-02-25 15:51 UTC (permalink / raw)
  To: Ferruh Yigit, Neil Horman, John McNamara, Marko Kovacevic
  Cc: dev, Jerin Jacob, David Marchand, Thomas Monjalon

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.


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH v2] doc: plan splitting the ethdev ops struct
  2020-02-25 15:51   ` Andrew Rybchenko
@ 2020-02-25 16:13     ` Ferruh Yigit
  2020-02-25 16:41       ` Andrew Rybchenko
  0 siblings, 1 reply; 21+ messages in thread
From: Ferruh Yigit @ 2020-02-25 16:13 UTC (permalink / raw)
  To: Andrew Rybchenko, Neil Horman, John McNamara, Marko Kovacevic
  Cc: dev, Jerin Jacob, David Marchand, Thomas Monjalon

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

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH v2] doc: plan splitting the ethdev ops struct
  2020-02-25 16:13     ` Ferruh Yigit
@ 2020-02-25 16:41       ` Andrew Rybchenko
  0 siblings, 0 replies; 21+ messages in thread
From: Andrew Rybchenko @ 2020-02-25 16:41 UTC (permalink / raw)
  To: Ferruh Yigit, Neil Horman, John McNamara, Marko Kovacevic
  Cc: dev, Jerin Jacob, David Marchand, Thomas Monjalon

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.

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH v2] doc: plan splitting the ethdev ops struct
  2020-02-25 12:44 ` [dpdk-dev] [PATCH v2] " Ferruh Yigit
  2020-02-25 15:51   ` Andrew Rybchenko
@ 2020-02-25 18:13   ` David Marchand
  2020-02-25 18:18     ` Ferruh Yigit
  2020-03-04  9:57   ` [dpdk-dev] [PATCH v3] " Ferruh Yigit
  2 siblings, 1 reply; 21+ messages in thread
From: David Marchand @ 2020-02-25 18:13 UTC (permalink / raw)
  To: Ferruh Yigit
  Cc: Neil Horman, John McNamara, Marko Kovacevic, dev, Jerin Jacob,
	Thomas Monjalon, Andrew Rybchenko

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.

-- 
David Marchand


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH v2] doc: plan splitting the ethdev ops struct
  2020-02-25 18:13   ` David Marchand
@ 2020-02-25 18:18     ` Ferruh Yigit
  0 siblings, 0 replies; 21+ messages in thread
From: Ferruh Yigit @ 2020-02-25 18:18 UTC (permalink / raw)
  To: David Marchand
  Cc: Neil Horman, John McNamara, Marko Kovacevic, dev, Jerin Jacob,
	Thomas Monjalon, Andrew Rybchenko

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.


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [dpdk-dev] [PATCH v3] doc: plan splitting the ethdev ops struct
  2020-02-25 12:44 ` [dpdk-dev] [PATCH v2] " Ferruh Yigit
  2020-02-25 15:51   ` Andrew Rybchenko
  2020-02-25 18:13   ` David Marchand
@ 2020-03-04  9:57   ` Ferruh Yigit
  2020-05-24 23:18     ` Thomas Monjalon
  2020-05-25 10:24     ` David Marchand
  2 siblings, 2 replies; 21+ messages in thread
From: Ferruh Yigit @ 2020-03-04  9:57 UTC (permalink / raw)
  To: Neil Horman, John McNamara, Marko Kovacevic
  Cc: dev, Ferruh Yigit, Jerin Jacob Kollanukkaran, David Marchand,
	Thomas Monjalon, Andrew Rybchenko

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.

Because of ABI break the work will be done in 20.11

Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.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

v3:
* Drop the interim implementation, and target 20.11 with ABI break.
---
 doc/guides/rel_notes/deprecation.rst | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 1339f54f5..0bb252f71 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -93,6 +93,15 @@ 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
+  will be done in 20.11.
+  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 will be moved
+  next to Rx/Tx burst functions, rest of the ``struct eth_dev_ops`` struct will be
+  moved to header file for drivers to hide it from applications.
+
 * 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
-- 
2.24.1


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH v3] doc: plan splitting the ethdev ops struct
  2020-03-04  9:57   ` [dpdk-dev] [PATCH v3] " Ferruh Yigit
@ 2020-05-24 23:18     ` Thomas Monjalon
  2020-05-25  9:11       ` Andrew Rybchenko
  2020-05-25 10:24     ` David Marchand
  1 sibling, 1 reply; 21+ messages in thread
From: Thomas Monjalon @ 2020-05-24 23:18 UTC (permalink / raw)
  To: Ferruh Yigit
  Cc: Neil Horman, John McNamara, Marko Kovacevic, dev,
	Jerin Jacob Kollanukkaran, David Marchand, Andrew Rybchenko, mdr,
	olivier.matz

04/03/2020 10:57, Ferruh Yigit:
> 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.
> 
> Because of ABI break the work will be done in 20.11
> 
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> ---
> +* ethdev: Split the ``struct eth_dev_ops`` struct to hide it as much as possible
> +  will be done in 20.11.
> +  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 will be moved
> +  next to Rx/Tx burst functions, rest of the ``struct eth_dev_ops`` struct will be
> +  moved to header file for drivers to hide it from applications.

Acked-by: Thomas Monjalon <thomas@monjalon.net>



^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH v3] doc: plan splitting the ethdev ops struct
  2020-05-24 23:18     ` Thomas Monjalon
@ 2020-05-25  9:11       ` Andrew Rybchenko
  2020-05-26 13:55         ` Thomas Monjalon
  0 siblings, 1 reply; 21+ messages in thread
From: Andrew Rybchenko @ 2020-05-25  9:11 UTC (permalink / raw)
  To: Thomas Monjalon, Ferruh Yigit
  Cc: Neil Horman, John McNamara, Marko Kovacevic, dev,
	Jerin Jacob Kollanukkaran, David Marchand, mdr, olivier.matz

On 5/25/20 2:18 AM, Thomas Monjalon wrote:
> 04/03/2020 10:57, Ferruh Yigit:
>> 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.
>>
>> Because of ABI break the work will be done in 20.11
>>
>> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
>> ---
>> +* ethdev: Split the ``struct eth_dev_ops`` struct to hide it as much as possible
>> +  will be done in 20.11.
>> +  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 will be moved
>> +  next to Rx/Tx burst functions, rest of the ``struct eth_dev_ops`` struct will be
>> +  moved to header file for drivers to hide it from applications.
> Acked-by: Thomas Monjalon <thomas@monjalon.net>

Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH v3] doc: plan splitting the ethdev ops struct
  2020-03-04  9:57   ` [dpdk-dev] [PATCH v3] " Ferruh Yigit
  2020-05-24 23:18     ` Thomas Monjalon
@ 2020-05-25 10:24     ` David Marchand
  1 sibling, 0 replies; 21+ messages in thread
From: David Marchand @ 2020-05-25 10:24 UTC (permalink / raw)
  To: Ferruh Yigit
  Cc: Neil Horman, John McNamara, Marko Kovacevic, dev,
	Jerin Jacob Kollanukkaran, Thomas Monjalon, Andrew Rybchenko

On Wed, Mar 4, 2020 at 10:57 AM 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.
>
> Because of ABI break the work will be done in 20.11
>
> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>

Acked-by: David Marchand <david.marchand@redhat.com>


-- 
David Marchand


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH] doc: plan splitting the ethdev ops struct
  2020-02-18  5:07 ` Jerin Jacob
  2020-02-25 12:42   ` Ferruh Yigit
@ 2020-05-26 13:01   ` Thomas Monjalon
  1 sibling, 0 replies; 21+ messages in thread
From: Thomas Monjalon @ 2020-05-26 13:01 UTC (permalink / raw)
  To: Jerin Jacob
  Cc: Ferruh Yigit, dev, Neil Horman, John McNamara, Marko Kovacevic,
	dpdk-dev, David Marchand, Andrew Rybchenko

18/02/2020 06:07, Jerin Jacob:
> On Mon, Feb 17, 2020 at 9:08 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.
> 
> It is a good improvement.  IMO, If anything used in fast-path it
> should be in ``struct rte_eth_dev``
> and rest can completely be moved to internal. In this case, if
> `rte_eth_tx_descriptor_status`
> not used on fastpath, Maybe we don't need to maintain the inline
> status and move completely
> to .c file.
> 
> Those may be specifics of the work. In general, this change looks good to me.
> 
> Acked-by: Jerin Jacob <jerinj@marvell.com>

This ack is missing from v3.
Jerin, please could you confirm on v3?




^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [dpdk-dev] [PATCH v3] doc: plan splitting the ethdev ops struct
  2020-05-25  9:11       ` Andrew Rybchenko
@ 2020-05-26 13:55         ` Thomas Monjalon
  0 siblings, 0 replies; 21+ messages in thread
From: Thomas Monjalon @ 2020-05-26 13:55 UTC (permalink / raw)
  To: Ferruh Yigit
  Cc: dev, Neil Horman, John McNamara, Marko Kovacevic, dev,
	Jerin Jacob Kollanukkaran, David Marchand, mdr, olivier.matz,
	Andrew Rybchenko

25/05/2020 11:11, Andrew Rybchenko:
> On 5/25/20 2:18 AM, Thomas Monjalon wrote:
> > 04/03/2020 10:57, Ferruh Yigit:
> >> 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.
> >>
> >> Because of ABI break the work will be done in 20.11
> >>
> >> Signed-off-by: Ferruh Yigit <ferruh.yigit@intel.com>
> >> ---
> >> +* ethdev: Split the ``struct eth_dev_ops`` struct to hide it as much as possible
> >> +  will be done in 20.11.
> >> +  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 will be moved
> >> +  next to Rx/Tx burst functions, rest of the ``struct eth_dev_ops`` struct will be
> >> +  moved to header file for drivers to hide it from applications.
> > Acked-by: Thomas Monjalon <thomas@monjalon.net>
> 
> Acked-by: Andrew Rybchenko <arybchenko@solarflare.com>

Acked-by: David Marchand <david.marchand@redhat.com>

Applied, thanks



^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, other threads:[~2020-05-26 13:55 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-17 15:38 [dpdk-dev] [PATCH] doc: plan splitting the ethdev ops struct Ferruh Yigit
2020-02-18  5:07 ` Jerin Jacob
2020-02-25 12:42   ` Ferruh Yigit
2020-05-26 13:01   ` Thomas Monjalon
2020-02-18  6:01 ` Stephen Hemminger
2020-02-21 10:40   ` Ferruh Yigit
2020-02-25 10:35     ` Andrew Rybchenko
2020-02-25 11:07       ` Ananyev, Konstantin
2020-02-25 11:19         ` Andrew Rybchenko
2020-02-25 12:28       ` Ferruh Yigit
2020-02-25 12:44 ` [dpdk-dev] [PATCH v2] " Ferruh Yigit
2020-02-25 15:51   ` Andrew Rybchenko
2020-02-25 16:13     ` Ferruh Yigit
2020-02-25 16:41       ` Andrew Rybchenko
2020-02-25 18:13   ` David Marchand
2020-02-25 18:18     ` Ferruh Yigit
2020-03-04  9:57   ` [dpdk-dev] [PATCH v3] " Ferruh Yigit
2020-05-24 23:18     ` Thomas Monjalon
2020-05-25  9:11       ` Andrew Rybchenko
2020-05-26 13:55         ` Thomas Monjalon
2020-05-25 10:24     ` David Marchand

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).