ethdev: hide internal structures

ethdev: hide internal structures

[Tyler Retzlaff]

hi folks,

I don't understand the text of this change.  would you mind explaining?

    commit f9bdee267ab84fd12dc288419aba341310b6ae08
    Author: Konstantin Ananyev <konstantin.ananyev@intel.com>
    Date:   Wed Oct 13 14:37:04 2021 +0100
    ethdev: hide internal structures                        

+* ethdev: Made ``rte_eth_dev``, ``rte_eth_dev_data``, ``rte_eth_rxtx_callback``           +  private data structures.  ``rte_eth_devices[]`` can't be accessed directly
+  by user any more. While it is an ABI breakage, this change is intended
+  to be transparent for both users (no changes in user app is required) and               +  PMD developers (no changes in PMD is required).


if it is an ABI break (and it is also an API break) how is it that
this change could be "transparent" to the user application?

* existing binaries will not run. (they need to be recompiled)
* existing code will not compile. (code changes are required)

in order to cope with this change an application will have to have the
code modified and will need to be re-compiled. so i don't understand how
that is transparent?

thanks

Re: ethdev: hide internal structures

[Ferruh Yigit]

On 11/16/2021 12:24 AM, Tyler Retzlaff wrote:
> hi folks,
> 
> I don't understand the text of this change.  would you mind explaining?
> 
>      commit f9bdee267ab84fd12dc288419aba341310b6ae08
>      Author: Konstantin Ananyev <konstantin.ananyev@intel.com>
>      Date:   Wed Oct 13 14:37:04 2021 +0100
>      ethdev: hide internal structures
> 
> +* ethdev: Made ``rte_eth_dev``, ``rte_eth_dev_data``, ``rte_eth_rxtx_callback``           > +  private data structures.  ``rte_eth_devices[]`` can't be accessed directly
> +  by user any more. While it is an ABI breakage, this change is intended
> +  to be transparent for both users (no changes in user app is required) and> +  PMD developers (no changes in PMD is required).
> 
> 
> if it is an ABI break (and it is also an API break) how is it that
> this change could be "transparent" to the user application?
> 
> * existing binaries will not run. (they need to be recompiled)
> * existing code will not compile. (code changes are required)
> 
> in order to cope with this change an application will have to have the
> code modified and will need to be re-compiled. so i don't understand how
> that is transparent?
> 


Hi Tyler,

It shouldn't be an API change, which API is changed?

Existing binaries won't run and needs recompile, but shouldn't need to change
the code.
Unless application is accessing *internal* DPDK structs (which were exposed
to application because of some technical issues that above commit fixes).

What code change do you require, in driver or application?

RE: ethdev: hide internal structures

[Ananyev, Konstantin]

> hi folks,
> 
> I don't understand the text of this change.  would you mind explaining?
> 
>     commit f9bdee267ab84fd12dc288419aba341310b6ae08
>     Author: Konstantin Ananyev <konstantin.ananyev@intel.com>
>     Date:   Wed Oct 13 14:37:04 2021 +0100
>     ethdev: hide internal structures
> 
> +* ethdev: Made ``rte_eth_dev``, ``rte_eth_dev_data``, ``rte_eth_rxtx_callback``           +  private data structures.  ``rte_eth_devices[]`` can't
> be accessed directly
> +  by user any more. While it is an ABI breakage, this change is intended
> +  to be transparent for both users (no changes in user app is required) and               +  PMD developers (no changes in PMD is required).
> 
> 
> if it is an ABI break (and it is also an API break) how is it that
> this change could be "transparent" to the user application?
> 
> * existing binaries will not run. (they need to be recompiled)
> * existing code will not compile. (code changes are required)
> 
> in order to cope with this change an application will have to have the
> code modified and will need to be re-compiled. so i don't understand how
> that is transparent?
 
rte_eth_dev,  rte_eth_dev_data, rte_eth_rxtx_callback are internal
data structures that were used by public inline ethdev functions. 
Well behaving app should not access these data structures directly.
So, for well behaving app there should no changes in the code required.
That what I meant by 'transparent' above.
But it is still an ABI change, so yes, the app has to be re-compiled. 

Konstantin

Re: ethdev: hide internal structures

[Tyler Retzlaff]

On Tue, Nov 16, 2021 at 09:32:15AM +0000, Ferruh Yigit wrote:
> 
> Hi Tyler,
> 
> It shouldn't be an API change, which API is changed?

exported declarations that were consumed by the application were removed
from an installed header. anything making reference to rte_eth_devices[]
will no longer compile.

any change that removes any identifier or macro visible to the application
from an installed header is an api break.

> Existing binaries won't run and needs recompile, but shouldn't need to change
> the code.
> Unless application is accessing *internal* DPDK structs (which were exposed
> to application because of some technical issues that above commit fixes).

the application was, but the access was to a symbol and identifier that
had not been previously marked __rte_internal or __rte_experimental and thus
assumed to be public.

just to be clear i agree with the change making these internal but there
was virtually no warning.

https://doc.dpdk.org/guides-19.11/contributing/abi_policy.html

the exports and declarations need to be marked deprecated to give ample
time before being removed in accordance with the abi policy.

i will ask that work be scheduled to identify the gap in the public api
surface that access to these structures was providing rather than
backing the change out. fortunately it is only schedule rather
than service impacting since the application hadn't been deployed yet.

i thought someone was responsible for reviewing abi/api related changes
on the board to understand the implications of changes like this?

thanks

Re: ethdev: hide internal structures

[Tyler Retzlaff]

On Tue, Nov 16, 2021 at 10:32:55AM +0000, Ananyev, Konstantin wrote:
>  
> rte_eth_dev,  rte_eth_dev_data, rte_eth_rxtx_callback are internal
> data structures that were used by public inline ethdev functions. 
> Well behaving app should not access these data structures directly.
> So, for well behaving app there should no changes in the code required.
> That what I meant by 'transparent' above.
> But it is still an ABI change, so yes, the app has to be re-compiled. 

so it appears the application was establishing a private context /
vendor extension between the application and a pmd. the application
was abusing access to the rte_eth_devices[] to get the private context
from the rte_eth_dev.

is there a proper / supported way of providing this functionality
through the public api?

> 
> Konstantin

Re: ethdev: hide internal structures

[Ferruh Yigit]

On 11/16/2021 5:54 PM, Tyler Retzlaff wrote:
> On Tue, Nov 16, 2021 at 09:32:15AM +0000, Ferruh Yigit wrote:
>>
>> Hi Tyler,
>>
>> It shouldn't be an API change, which API is changed?
> 
> exported declarations that were consumed by the application were removed
> from an installed header. anything making reference to rte_eth_devices[]
> will no longer compile.
> 
> any change that removes any identifier or macro visible to the application
> from an installed header is an api break.
> 
>> Existing binaries won't run and needs recompile, but shouldn't need to change
>> the code.
>> Unless application is accessing *internal* DPDK structs (which were exposed
>> to application because of some technical issues that above commit fixes).
> 
> the application was, but the access was to a symbol and identifier that
> had not been previously marked __rte_internal or __rte_experimental and thus
> assumed to be public.
> 
> just to be clear i agree with the change making these internal but there
> was virtually no warning.
> 
> https://doc.dpdk.org/guides-19.11/contributing/abi_policy.html
> 
> the exports and declarations need to be marked deprecated to give ample
> time before being removed in accordance with the abi policy.
> 
> i will ask that work be scheduled to identify the gap in the public api
> surface that access to these structures was providing rather than
> backing the change out. fortunately it is only schedule rather
> than service impacting since the application hadn't been deployed yet.
> 
> i thought someone was responsible for reviewing abi/api related changes
> on the board to understand the implications of changes like this?
> 

Sorry for the negative impact on your product, I can understand the
frustration.

The 'rte_eth_devices[]' was marked as '@internal' in the header file
since 2012 [1], so it is not new, but it was not marked programmatically,
only as comment in the header file.
Expectation was applications to not directly use it.


For long term ABI stability, this is a good step forward, although
the impact was known, best time for these kind of change is the 21.11
release, otherwise change needs to wait (at least) one more year.

This change has been discussed and accepted in the technical board [2],
and a deprecation notice has been sent to mail list [3] for notification.

Agree the announce was a little late than we normally do (although
only a month late than what defined in process), this is accepted by
the board to not miss the ABI break window (.11 release).
As you will recognize, not only ethdev, but a few more device abstraction
layer libraries had similar changes in this release.


[1]
f831c63cbe86 ("ethdev: minor changes")

[2]
https://mails.dpdk.org/archives/dev/2021-July/214662.html

[3]
https://patches.dpdk.org/project/dpdk/patch/20210826103500.2172550-1-ferruh.yigit@intel.com/

Re: ethdev: hide internal structures

[Tyler Retzlaff]

On Tue, Nov 16, 2021 at 08:07:49PM +0000, Ferruh Yigit wrote:
> On 11/16/2021 5:54 PM, Tyler Retzlaff wrote:
> >
> >i thought someone was responsible for reviewing abi/api related changes
> >on the board to understand the implications of changes like this?
> >
> 
> Sorry for the negative impact on your product, I can understand the
> frustration.
> 
> The 'rte_eth_devices[]' was marked as '@internal' in the header file
> since 2012 [1], so it is not new, but it was not marked programmatically,
> only as comment in the header file.
> Expectation was applications to not directly use it.

unfortunately there are a lot of these expectations in the project code,
rarely do consuming applications get written in the way we would expect
and this is a lesson that if it is not mechanically enforced it isn't
prevented.

> 
> 
> For long term ABI stability, this is a good step forward, although
> the impact was known, best time for these kind of change is the 21.11
> release, otherwise change needs to wait (at least) one more year.

agreed, we appreciate what will be accomplished with the change.

> 
> This change has been discussed and accepted in the technical board [2],
> and a deprecation notice has been sent to mail list [3] for notification.

the notes from [2] aren't that clear, but i think it is fair you point
out that if [3] were read carefully it was implied that it would impact
ethdev. anyway, it is moot now.

> 
> Agree the announce was a little late than we normally do (although
> only a month late than what defined in process), this is accepted by
> the board to not miss the ABI break window (.11 release).
> As you will recognize, not only ethdev, but a few more device abstraction
> layer libraries had similar changes in this release.

yes, i understand. perhaps in the future it may be possible to introduce
some kind of __deprecation notice during compilation earlier than the
removal and it may have been noticed sooner. perhaps a patch that did
this near the time of the original notification [2].

i've left the details of the functional gap in my other reply to the
thread, hopefully you have a suggestion.

thanks Ferruh, appreciate it.

> 
> 
> [1]
> f831c63cbe86 ("ethdev: minor changes")
> 
> [2]
> https://mails.dpdk.org/archives/dev/2021-July/214662.html
> 
> [3]
> https://patches.dpdk.org/project/dpdk/patch/20210826103500.2172550-1-ferruh.yigit@intel.com/

Re: ethdev: hide internal structures

[Stephen Hemminger]

On Tue, 16 Nov 2021 11:10:18 -0800
Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:

> On Tue, Nov 16, 2021 at 10:32:55AM +0000, Ananyev, Konstantin wrote:
> >  
> > rte_eth_dev,  rte_eth_dev_data, rte_eth_rxtx_callback are internal
> > data structures that were used by public inline ethdev functions. 
> > Well behaving app should not access these data structures directly.
> > So, for well behaving app there should no changes in the code required.
> > That what I meant by 'transparent' above.
> > But it is still an ABI change, so yes, the app has to be re-compiled.   
> 
> so it appears the application was establishing a private context /
> vendor extension between the application and a pmd. the application
> was abusing access to the rte_eth_devices[] to get the private context
> from the rte_eth_dev.
> 
> is there a proper / supported way of providing this functionality
> through the public api?
> 
> > 
> > Konstantin  

Keep a array in application?  Portid is universally
available.

struct my_portdata *my_ports[RTE_ETH_MAXPORTS];

Re: ethdev: hide internal structures

[Tyler Retzlaff]

On Tue, Nov 16, 2021 at 01:25:10PM -0800, Stephen Hemminger wrote:
> On Tue, 16 Nov 2021 11:10:18 -0800
> Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> 
> > On Tue, Nov 16, 2021 at 10:32:55AM +0000, Ananyev, Konstantin wrote:
> > >  
> > > rte_eth_dev,  rte_eth_dev_data, rte_eth_rxtx_callback are internal
> > > data structures that were used by public inline ethdev functions. 
> > > Well behaving app should not access these data structures directly.
> > > So, for well behaving app there should no changes in the code required.
> > > That what I meant by 'transparent' above.
> > > But it is still an ABI change, so yes, the app has to be re-compiled.   
> > 
> > so it appears the application was establishing a private context /
> > vendor extension between the application and a pmd. the application
> > was abusing access to the rte_eth_devices[] to get the private context
> > from the rte_eth_dev.
> > 
> > is there a proper / supported way of providing this functionality
> > through the public api?
> > 
> > > 
> > > Konstantin  
> 
> Keep a array in application?  Portid is universally
> available.
> 
> struct my_portdata *my_ports[RTE_ETH_MAXPORTS];

i guess by this you mean maintain the storage in the application and
then export that storage for proprietary use in the pmd. ordinarily i
wouldn't want to have this hard-coded into the modules abi but since
we are talking about vendor extensions it has to be managed somewhere.

anyway, i guess i have my answer.

thanks stephen, appreciate it.

Re: ethdev: hide internal structures

[Stephen Hemminger]

On Tue, 16 Nov 2021 14:58:08 -0800
Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:

> On Tue, Nov 16, 2021 at 01:25:10PM -0800, Stephen Hemminger wrote:
> > On Tue, 16 Nov 2021 11:10:18 -0800
> > Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> >   
> > > On Tue, Nov 16, 2021 at 10:32:55AM +0000, Ananyev, Konstantin wrote:  
> > > >  
> > > > rte_eth_dev,  rte_eth_dev_data, rte_eth_rxtx_callback are internal
> > > > data structures that were used by public inline ethdev functions. 
> > > > Well behaving app should not access these data structures directly.
> > > > So, for well behaving app there should no changes in the code required.
> > > > That what I meant by 'transparent' above.
> > > > But it is still an ABI change, so yes, the app has to be re-compiled.     
> > > 
> > > so it appears the application was establishing a private context /
> > > vendor extension between the application and a pmd. the application
> > > was abusing access to the rte_eth_devices[] to get the private context
> > > from the rte_eth_dev.
> > > 
> > > is there a proper / supported way of providing this functionality
> > > through the public api?
> > >   
> > > > 
> > > > Konstantin    
> > 
> > Keep a array in application?  Portid is universally
> > available.
> > 
> > struct my_portdata *my_ports[RTE_ETH_MAXPORTS];  
> 
> i guess by this you mean maintain the storage in the application and
> then export that storage for proprietary use in the pmd. ordinarily i
> wouldn't want to have this hard-coded into the modules abi but since
> we are talking about vendor extensions it has to be managed somewhere.
> 
> anyway, i guess i have my answer.
> 
> thanks stephen, appreciate it.

Don't understand, how are application and pmd exchanging extra data?
Maybe a non-standard PMD API?

Re: ethdev: hide internal structures

[Tyler Retzlaff]

On Tue, Nov 16, 2021 at 03:22:01PM -0800, Stephen Hemminger wrote:
> On Tue, 16 Nov 2021 14:58:08 -0800
> Tyler Retzlaff <roretzla@linux.microsoft.com> wrote:
> 
> > > 
> > > Keep a array in application?  Portid is universally
> > > available.
> > > 
> > > struct my_portdata *my_ports[RTE_ETH_MAXPORTS];  
> > 
> > i guess by this you mean maintain the storage in the application and
> > then export that storage for proprietary use in the pmd. ordinarily i
> > wouldn't want to have this hard-coded into the modules abi but since
> > we are talking about vendor extensions it has to be managed somewhere.
> > 
> > anyway, i guess i have my answer.
> > 
> > thanks stephen, appreciate it.
> 
> Don't understand, how are application and pmd exchanging extra data?
> Maybe a non-standard PMD API?

yes. consider the case of a "vendor extension" where for a specific pmd
driver it is possible that extra / non-standard operations are
supported.

in this instance we have a pmd that does some whiz-bang thing that isn't
something most hardware/pmds could do (or need to under ordinary 
circumstances) so it doesn't make sense to adapt the generalized pmd api
for some one-off hardware/device.  however, the vendor ships an
application that is extended to understand this extra functionality and
needs a way to hook up with and inter-operate with the non-standard api.

one example that is very common is some kind of advanced statistics that
most hardware aren't capable of producing. as long as the application
knows it is working with this advanced hardware it can present those
statistics.

in the code i'm looking at it isn't statistics but specialized control
operations that can't be expressed via the exported pmd api (and should
not be).