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
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 >
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.
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. >
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. >>
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. > >>
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. >>>>
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. >>> >
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 >>
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
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.
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
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.
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
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.
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
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>
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>
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
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?
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