[v2] doc: add reserve fields to eventdev public structures
diff mbox series

Message ID 20200803072903.1209-1-pbhagavatula@marvell.com
State New
Delegated to: Thomas Monjalon
Headers show
Series
  • [v2] doc: add reserve fields to eventdev public structures
Related show

Checks

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

Commit Message

Pavan Nikhilesh Bhagavatula Aug. 3, 2020, 7:29 a.m. UTC
From: Pavan Nikhilesh <pbhagavatula@marvell.com>

Add 64 byte padding at the end of event device public structure to allow
future extensions.

Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
Acked-by: Jerin Jacob <jerinj@marvell.com>
---
 v2 Changes:
 - Modify commit title.
 - Add patch reference to doc.

 doc/guides/rel_notes/deprecation.rst | 11 +++++++++++
 1 file changed, 11 insertions(+)

--
2.17.1

Comments

Bruce Richardson Aug. 4, 2020, 10:41 a.m. UTC | #1
On Mon, Aug 03, 2020 at 12:59:03PM +0530, pbhagavatula@marvell.com wrote:
> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> 
> Add 64 byte padding at the end of event device public structure to allow
> future extensions.
> 
> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> Acked-by: Jerin Jacob <jerinj@marvell.com>
> ---
>  v2 Changes:
>  - Modify commit title.
>  - Add patch reference to doc.
> 
>  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 ea4cfa7a4..ec5db68e9 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -151,3 +151,14 @@ Deprecation Notices
>    Python 2 support will be completely removed in 20.11.
>    In 20.08, explicit deprecation warnings will be displayed when running
>    scripts with Python 2.
> +
> +* eventdev: A 64 byte padding is added at the end of the following structures
> +  in event device library to support future extensions:
> +  ``rte_event_crypto_adapter_conf``, ``rte_event_eth_rx_adapter_conf``,
> +  ``rte_event_eth_rx_adapter_queue_conf``, ``rte_event_eth_tx_adapter_conf``,
> +  ``rte_event_timer_adapter_conf``, ``rte_event_timer_adapter_info``,
> +  ``rte_event_dev_info``, ``rte_event_dev_config``, ``rte_event_queue_conf``,
> +  ``rte_event_port_conf``, ``rte_event_timer_adapter``,
> +  ``rte_event_timer_adapter_data``.
> +  Reference:
> +  http://patches.dpdk.org/project/dpdk/list/?series=10728&archive=both&state=*
> --

I don't like this idea of adding lots of padding to the ends of these
structures. For some structures, such as the public arrays for devices it
may be necessary, but for all the conf structures passed as parameters to
functions I think we can do better. Since these structures are passed by
the user to various functions, function versioning can be used to ensure
that the correct function in eventdev is always called. From there to the
individual PMDs, we can implement ABI compatibility by either:
1. including the length of the struct as a parameter to the driver. (This is
  a bit similar to my proposal for rawdev [1])
2. including the ABI version as a parameter to the driver.

Regards
/Bruce

[1] http://inbox.dpdk.org/dev/?q=enhance+rawdev+APIs
Jerin Jacob Aug. 4, 2020, 11:37 a.m. UTC | #2
On Tue, Aug 4, 2020 at 4:12 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Mon, Aug 03, 2020 at 12:59:03PM +0530, pbhagavatula@marvell.com wrote:
> > From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> >
> > Add 64 byte padding at the end of event device public structure to allow
> > future extensions.
> >
> > Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > Acked-by: Jerin Jacob <jerinj@marvell.com>
> > ---
> >  v2 Changes:
> >  - Modify commit title.
> >  - Add patch reference to doc.
> >
> >  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 ea4cfa7a4..ec5db68e9 100644
> > --- a/doc/guides/rel_notes/deprecation.rst
> > +++ b/doc/guides/rel_notes/deprecation.rst
> > @@ -151,3 +151,14 @@ Deprecation Notices
> >    Python 2 support will be completely removed in 20.11.
> >    In 20.08, explicit deprecation warnings will be displayed when running
> >    scripts with Python 2.
> > +
> > +* eventdev: A 64 byte padding is added at the end of the following structures
> > +  in event device library to support future extensions:
> > +  ``rte_event_crypto_adapter_conf``, ``rte_event_eth_rx_adapter_conf``,
> > +  ``rte_event_eth_rx_adapter_queue_conf``, ``rte_event_eth_tx_adapter_conf``,
> > +  ``rte_event_timer_adapter_conf``, ``rte_event_timer_adapter_info``,
> > +  ``rte_event_dev_info``, ``rte_event_dev_config``, ``rte_event_queue_conf``,
> > +  ``rte_event_port_conf``, ``rte_event_timer_adapter``,
> > +  ``rte_event_timer_adapter_data``.
> > +  Reference:
> > +  http://patches.dpdk.org/project/dpdk/list/?series=10728&archive=both&state=*
> > --
>
> I don't like this idea of adding lots of padding to the ends of these
> structures. For some structures, such as the public arrays for devices it
> may be necessary, but for all the conf structures passed as parameters to
> functions I think we can do better. Since these structures are passed by
> the user to various functions, function versioning can be used to ensure
> that the correct function in eventdev is always called. From there to the
> individual PMDs, we can implement ABI compatibility by either:
> 1. including the length of the struct as a parameter to the driver. (This is
>   a bit similar to my proposal for rawdev [1])
> 2. including the ABI version as a parameter to the driver.

But, Will the above solution work if the application is dependent on
struct size?
i.e change of s1 size will change offset of s3 i.e
app_sepecific_struct_s3. Right?
i.e DPDK version should not change the offset of s3. Right?

example,
struct app_struct {
          struct dpdk_public_struct_s1 s1;
          struct dpdk_public_struct_s2 s2;
          struct app_sepecific_struct_s3 s3;
}


>
> Regards
> /Bruce
>
> [1] http://inbox.dpdk.org/dev/?q=enhance+rawdev+APIs
Bruce Richardson Aug. 4, 2020, 2:24 p.m. UTC | #3
On Tue, Aug 04, 2020 at 05:07:12PM +0530, Jerin Jacob wrote:
> On Tue, Aug 4, 2020 at 4:12 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Mon, Aug 03, 2020 at 12:59:03PM +0530, pbhagavatula@marvell.com wrote:
> > > From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > >
> > > Add 64 byte padding at the end of event device public structure to allow
> > > future extensions.
> > >
> > > Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > > Acked-by: Jerin Jacob <jerinj@marvell.com>
> > > ---
> > >  v2 Changes:
> > >  - Modify commit title.
> > >  - Add patch reference to doc.
> > >
> > >  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 ea4cfa7a4..ec5db68e9 100644
> > > --- a/doc/guides/rel_notes/deprecation.rst
> > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > @@ -151,3 +151,14 @@ Deprecation Notices
> > >    Python 2 support will be completely removed in 20.11.
> > >    In 20.08, explicit deprecation warnings will be displayed when running
> > >    scripts with Python 2.
> > > +
> > > +* eventdev: A 64 byte padding is added at the end of the following structures
> > > +  in event device library to support future extensions:
> > > +  ``rte_event_crypto_adapter_conf``, ``rte_event_eth_rx_adapter_conf``,
> > > +  ``rte_event_eth_rx_adapter_queue_conf``, ``rte_event_eth_tx_adapter_conf``,
> > > +  ``rte_event_timer_adapter_conf``, ``rte_event_timer_adapter_info``,
> > > +  ``rte_event_dev_info``, ``rte_event_dev_config``, ``rte_event_queue_conf``,
> > > +  ``rte_event_port_conf``, ``rte_event_timer_adapter``,
> > > +  ``rte_event_timer_adapter_data``.
> > > +  Reference:
> > > +  http://patches.dpdk.org/project/dpdk/list/?series=10728&archive=both&state=*
> > > --
> >
> > I don't like this idea of adding lots of padding to the ends of these
> > structures. For some structures, such as the public arrays for devices it
> > may be necessary, but for all the conf structures passed as parameters to
> > functions I think we can do better. Since these structures are passed by
> > the user to various functions, function versioning can be used to ensure
> > that the correct function in eventdev is always called. From there to the
> > individual PMDs, we can implement ABI compatibility by either:
> > 1. including the length of the struct as a parameter to the driver. (This is
> >   a bit similar to my proposal for rawdev [1])
> > 2. including the ABI version as a parameter to the driver.
> 
> But, Will the above solution work if the application is dependent on
> struct size?
> i.e change of s1 size will change offset of s3 i.e
> app_sepecific_struct_s3. Right?
> i.e DPDK version should not change the offset of s3. Right?
> 
> example,
> struct app_struct {
>           struct dpdk_public_struct_s1 s1;
>           struct dpdk_public_struct_s2 s2;
>           struct app_sepecific_struct_s3 s3;
> }
> 
Not sure what exactly you mean here. The actual offsets and sizes of the
structs will obviously change as you change the struct, but the end
compiled app has no idea of structs, all it knows of is offsets, which is
why you provide ABI compatible versions of the functions which use "legacy"
copies of the structs to ensure correct offsets. It's pretty much standard
practice for ABI versioning.

The real complication arises because the actual eventdev driver functions
are not called directly with the linker resolving symbol versioning.
Instead they are called using function pointers from the code. This is why
one needs to add in the additional parameter to the driver APIs so that the
ABI info - be it struct size or version - can be passed from the versioned
eventdev library function through to the driver.

Regards,
/Bruce
Jerin Jacob Aug. 4, 2020, 4:03 p.m. UTC | #4
On Tue, Aug 4, 2020 at 7:55 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Tue, Aug 04, 2020 at 05:07:12PM +0530, Jerin Jacob wrote:
> > On Tue, Aug 4, 2020 at 4:12 PM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > >
> > > On Mon, Aug 03, 2020 at 12:59:03PM +0530, pbhagavatula@marvell.com wrote:
> > > > From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > > >
> > > > Add 64 byte padding at the end of event device public structure to allow
> > > > future extensions.
> > > >
> > > > Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > > > Acked-by: Jerin Jacob <jerinj@marvell.com>
> > > > ---
> > > >  v2 Changes:
> > > >  - Modify commit title.
> > > >  - Add patch reference to doc.
> > > >
> > > >  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 ea4cfa7a4..ec5db68e9 100644
> > > > --- a/doc/guides/rel_notes/deprecation.rst
> > > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > > @@ -151,3 +151,14 @@ Deprecation Notices
> > > >    Python 2 support will be completely removed in 20.11.
> > > >    In 20.08, explicit deprecation warnings will be displayed when running
> > > >    scripts with Python 2.
> > > > +
> > > > +* eventdev: A 64 byte padding is added at the end of the following structures
> > > > +  in event device library to support future extensions:
> > > > +  ``rte_event_crypto_adapter_conf``, ``rte_event_eth_rx_adapter_conf``,
> > > > +  ``rte_event_eth_rx_adapter_queue_conf``, ``rte_event_eth_tx_adapter_conf``,
> > > > +  ``rte_event_timer_adapter_conf``, ``rte_event_timer_adapter_info``,
> > > > +  ``rte_event_dev_info``, ``rte_event_dev_config``, ``rte_event_queue_conf``,
> > > > +  ``rte_event_port_conf``, ``rte_event_timer_adapter``,
> > > > +  ``rte_event_timer_adapter_data``.
> > > > +  Reference:
> > > > +  http://patches.dpdk.org/project/dpdk/list/?series=10728&archive=both&state=*
> > > > --
> > >
> > > I don't like this idea of adding lots of padding to the ends of these
> > > structures. For some structures, such as the public arrays for devices it
> > > may be necessary, but for all the conf structures passed as parameters to
> > > functions I think we can do better. Since these structures are passed by
> > > the user to various functions, function versioning can be used to ensure
> > > that the correct function in eventdev is always called. From there to the
> > > individual PMDs, we can implement ABI compatibility by either:
> > > 1. including the length of the struct as a parameter to the driver. (This is
> > >   a bit similar to my proposal for rawdev [1])
> > > 2. including the ABI version as a parameter to the driver.
> >
> > But, Will the above solution work if the application is dependent on
> > struct size?
> > i.e change of s1 size will change offset of s3 i.e
> > app_sepecific_struct_s3. Right?
> > i.e DPDK version should not change the offset of s3. Right?
> >
> > example,
> > struct app_struct {
> >           struct dpdk_public_struct_s1 s1;
> >           struct dpdk_public_struct_s2 s2;
> >           struct app_sepecific_struct_s3 s3;
> > }
> >
> Not sure what exactly you mean here. The actual offsets and sizes of the
> structs will obviously change as you change the struct, but the end
> compiled app has no idea of structs, all it knows of is offsets, which is
> why you provide ABI compatible versions of the functions which use "legacy"
> copies of the structs to ensure correct offsets. It's pretty much standard
> practice for ABI versioning.

Currently, We have only function versioning(not structure versioning).
Are you suggesting having structure versioning?
Will it complicate the code in terms of readability and supporting multiple
structure versions aginst its support functions.

>
> The real complication arises because the actual eventdev driver functions
> are not called directly with the linker resolving symbol versioning.
> Instead they are called using function pointers from the code. This is why
> one needs to add in the additional parameter to the driver APIs so that the
> ABI info - be it struct size or version - can be passed from the versioned
> eventdev library function through to the driver.

If I understand it correctly, it is easy for rawdev subsystem as
config structure as _opaque_.
But in the case for ethdev, cryptodev, eventdev, config structure are
not opaque.

If we take structure versioning, Is n't call for adding structure
version change to
functions that's been associated with(Kind of M x N case to make a
structure change. Here M means
offending structure to make change and N means function associated
with structure)




>
> Regards,
> /Bruce
Stephen Hemminger Aug. 4, 2020, 4:20 p.m. UTC | #5
On Tue, 4 Aug 2020 11:41:53 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

> On Mon, Aug 03, 2020 at 12:59:03PM +0530, pbhagavatula@marvell.com wrote:
> > From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > 
> > Add 64 byte padding at the end of event device public structure to allow
> > future extensions.
> > 
> > Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > Acked-by: Jerin Jacob <jerinj@marvell.com>
> > ---
> >  v2 Changes:
> >  - Modify commit title.
> >  - Add patch reference to doc.
> > 
> >  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 ea4cfa7a4..ec5db68e9 100644
> > --- a/doc/guides/rel_notes/deprecation.rst
> > +++ b/doc/guides/rel_notes/deprecation.rst
> > @@ -151,3 +151,14 @@ Deprecation Notices
> >    Python 2 support will be completely removed in 20.11.
> >    In 20.08, explicit deprecation warnings will be displayed when running
> >    scripts with Python 2.
> > +
> > +* eventdev: A 64 byte padding is added at the end of the following structures
> > +  in event device library to support future extensions:
> > +  ``rte_event_crypto_adapter_conf``, ``rte_event_eth_rx_adapter_conf``,
> > +  ``rte_event_eth_rx_adapter_queue_conf``, ``rte_event_eth_tx_adapter_conf``,
> > +  ``rte_event_timer_adapter_conf``, ``rte_event_timer_adapter_info``,
> > +  ``rte_event_dev_info``, ``rte_event_dev_config``, ``rte_event_queue_conf``,
> > +  ``rte_event_port_conf``, ``rte_event_timer_adapter``,
> > +  ``rte_event_timer_adapter_data``.
> > +  Reference:
> > +  http://patches.dpdk.org/project/dpdk/list/?series=10728&archive=both&state=*
> > --  
> 
> I don't like this idea of adding lots of padding to the ends of these
> structures. For some structures, such as the public arrays for devices it
> may be necessary, but for all the conf structures passed as parameters to
> functions I think we can do better. Since these structures are passed by
> the user to various functions, function versioning can be used to ensure
> that the correct function in eventdev is always called. From there to the
> individual PMDs, we can implement ABI compatibility by either:
> 1. including the length of the struct as a parameter to the driver. (This is
>   a bit similar to my proposal for rawdev [1])
> 2. including the ABI version as a parameter to the driver.
> 
> Regards
> /Bruce
> 
> [1] http://inbox.dpdk.org/dev/?q=enhance+rawdev+APIs

This is a bad idea.

Reserved fields won't work because nothing requires that the application
zero them. You can't start using them later because the application
may put uninitialized or junk data there.
Bruce Richardson Aug. 4, 2020, 4:24 p.m. UTC | #6
On Tue, Aug 04, 2020 at 09:33:14PM +0530, Jerin Jacob wrote:
> On Tue, Aug 4, 2020 at 7:55 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
> >
> > On Tue, Aug 04, 2020 at 05:07:12PM +0530, Jerin Jacob wrote:
> > > On Tue, Aug 4, 2020 at 4:12 PM Bruce Richardson
> > > <bruce.richardson@intel.com> wrote:
> > > >
> > > > On Mon, Aug 03, 2020 at 12:59:03PM +0530, pbhagavatula@marvell.com wrote:
> > > > > From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > > > >
> > > > > Add 64 byte padding at the end of event device public structure to allow
> > > > > future extensions.
> > > > >
> > > > > Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > > > > Acked-by: Jerin Jacob <jerinj@marvell.com>
> > > > > ---
> > > > >  v2 Changes:
> > > > >  - Modify commit title.
> > > > >  - Add patch reference to doc.
> > > > >
> > > > >  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 ea4cfa7a4..ec5db68e9 100644
> > > > > --- a/doc/guides/rel_notes/deprecation.rst
> > > > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > > > @@ -151,3 +151,14 @@ Deprecation Notices
> > > > >    Python 2 support will be completely removed in 20.11.
> > > > >    In 20.08, explicit deprecation warnings will be displayed when running
> > > > >    scripts with Python 2.
> > > > > +
> > > > > +* eventdev: A 64 byte padding is added at the end of the following structures
> > > > > +  in event device library to support future extensions:
> > > > > +  ``rte_event_crypto_adapter_conf``, ``rte_event_eth_rx_adapter_conf``,
> > > > > +  ``rte_event_eth_rx_adapter_queue_conf``, ``rte_event_eth_tx_adapter_conf``,
> > > > > +  ``rte_event_timer_adapter_conf``, ``rte_event_timer_adapter_info``,
> > > > > +  ``rte_event_dev_info``, ``rte_event_dev_config``, ``rte_event_queue_conf``,
> > > > > +  ``rte_event_port_conf``, ``rte_event_timer_adapter``,
> > > > > +  ``rte_event_timer_adapter_data``.
> > > > > +  Reference:
> > > > > +  http://patches.dpdk.org/project/dpdk/list/?series=10728&archive=both&state=*
> > > > > --
> > > >
> > > > I don't like this idea of adding lots of padding to the ends of these
> > > > structures. For some structures, such as the public arrays for devices it
> > > > may be necessary, but for all the conf structures passed as parameters to
> > > > functions I think we can do better. Since these structures are passed by
> > > > the user to various functions, function versioning can be used to ensure
> > > > that the correct function in eventdev is always called. From there to the
> > > > individual PMDs, we can implement ABI compatibility by either:
> > > > 1. including the length of the struct as a parameter to the driver. (This is
> > > >   a bit similar to my proposal for rawdev [1])
> > > > 2. including the ABI version as a parameter to the driver.
> > >
> > > But, Will the above solution work if the application is dependent on
> > > struct size?
> > > i.e change of s1 size will change offset of s3 i.e
> > > app_sepecific_struct_s3. Right?
> > > i.e DPDK version should not change the offset of s3. Right?
> > >
> > > example,
> > > struct app_struct {
> > >           struct dpdk_public_struct_s1 s1;
> > >           struct dpdk_public_struct_s2 s2;
> > >           struct app_sepecific_struct_s3 s3;
> > > }
> > >
> > Not sure what exactly you mean here. The actual offsets and sizes of the
> > structs will obviously change as you change the struct, but the end
> > compiled app has no idea of structs, all it knows of is offsets, which is
> > why you provide ABI compatible versions of the functions which use "legacy"
> > copies of the structs to ensure correct offsets. It's pretty much standard
> > practice for ABI versioning.
> 
> Currently, We have only function versioning(not structure versioning).
> Are you suggesting having structure versioning?
> Will it complicate the code in terms of readability and supporting multiple
> structure versions aginst its support functions.
> 

We don't, and can't version structures, only functions are versioned.
Even if we do what you suggest and add a block of 64-bytes expansion room
at the end of the structures, how is the function you are calling expected
to know what the structure actually contains? For example, if you add a
field to the end, and reduce the padding by 8 bytes, your structure is
still the same size, and how does the called function know whether X or X+8
bytes are valid in it. Basically, you still need to version all
functions using the structure, just as if you didn't bother extending the
struct.

> >
> > The real complication arises because the actual eventdev driver functions
> > are not called directly with the linker resolving symbol versioning.
> > Instead they are called using function pointers from the code. This is why
> > one needs to add in the additional parameter to the driver APIs so that the
> > ABI info - be it struct size or version - can be passed from the versioned
> > eventdev library function through to the driver.
> 
> If I understand it correctly, it is easy for rawdev subsystem as
> config structure as _opaque_.
> But in the case for ethdev, cryptodev, eventdev, config structure are
> not opaque.
> 

Well, actually it's more complicated for rawdev because the structures are
opaque, so the value passed could be anything. With eventdev, they will
ever only be one or two possible values, so you just need enough info to
distinguish if it's the structure from version X, or the structure from
version Y you are passing.

> If we take structure versioning, Is n't call for adding structure
> version change to
> functions that's been associated with(Kind of M x N case to make a
> structure change. Here M means
> offending structure to make change and N means function associated
> with structure)
> 
Yes, to version any structure you basically do a new version of every
function using it, or using the changed fields. However, adding padding is
not going to remove that need.

Regards,
/Bruce
Jerin Jacob Aug. 4, 2020, 5:18 p.m. UTC | #7
On Tue, Aug 4, 2020 at 9:54 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Tue, Aug 04, 2020 at 09:33:14PM +0530, Jerin Jacob wrote:
> > On Tue, Aug 4, 2020 at 7:55 PM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> > >
> > > On Tue, Aug 04, 2020 at 05:07:12PM +0530, Jerin Jacob wrote:
> > > > On Tue, Aug 4, 2020 at 4:12 PM Bruce Richardson
> > > > <bruce.richardson@intel.com> wrote:
> > > > >
> > > > > On Mon, Aug 03, 2020 at 12:59:03PM +0530, pbhagavatula@marvell.com wrote:
> > > > > > From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > > > > >
> > > > > > Add 64 byte padding at the end of event device public structure to allow
> > > > > > future extensions.
> > > > > >
> > > > > > Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > > > > > Acked-by: Jerin Jacob <jerinj@marvell.com>
> > > > > > ---
> > > > > >  v2 Changes:
> > > > > >  - Modify commit title.
> > > > > >  - Add patch reference to doc.
> > > > > >
> > > > > >  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 ea4cfa7a4..ec5db68e9 100644
> > > > > > --- a/doc/guides/rel_notes/deprecation.rst
> > > > > > +++ b/doc/guides/rel_notes/deprecation.rst
> > > > > > @@ -151,3 +151,14 @@ Deprecation Notices
> > > > > >    Python 2 support will be completely removed in 20.11.
> > > > > >    In 20.08, explicit deprecation warnings will be displayed when running
> > > > > >    scripts with Python 2.
> > > > > > +
> > > > > > +* eventdev: A 64 byte padding is added at the end of the following structures
> > > > > > +  in event device library to support future extensions:
> > > > > > +  ``rte_event_crypto_adapter_conf``, ``rte_event_eth_rx_adapter_conf``,
> > > > > > +  ``rte_event_eth_rx_adapter_queue_conf``, ``rte_event_eth_tx_adapter_conf``,
> > > > > > +  ``rte_event_timer_adapter_conf``, ``rte_event_timer_adapter_info``,
> > > > > > +  ``rte_event_dev_info``, ``rte_event_dev_config``, ``rte_event_queue_conf``,
> > > > > > +  ``rte_event_port_conf``, ``rte_event_timer_adapter``,
> > > > > > +  ``rte_event_timer_adapter_data``.
> > > > > > +  Reference:
> > > > > > +  http://patches.dpdk.org/project/dpdk/list/?series=10728&archive=both&state=*
> > > > > > --
> > > > >
> > > > > I don't like this idea of adding lots of padding to the ends of these
> > > > > structures. For some structures, such as the public arrays for devices it
> > > > > may be necessary, but for all the conf structures passed as parameters to
> > > > > functions I think we can do better. Since these structures are passed by
> > > > > the user to various functions, function versioning can be used to ensure
> > > > > that the correct function in eventdev is always called. From there to the
> > > > > individual PMDs, we can implement ABI compatibility by either:
> > > > > 1. including the length of the struct as a parameter to the driver. (This is
> > > > >   a bit similar to my proposal for rawdev [1])
> > > > > 2. including the ABI version as a parameter to the driver.
> > > >
> > > > But, Will the above solution work if the application is dependent on
> > > > struct size?
> > > > i.e change of s1 size will change offset of s3 i.e
> > > > app_sepecific_struct_s3. Right?
> > > > i.e DPDK version should not change the offset of s3. Right?
> > > >
> > > > example,
> > > > struct app_struct {
> > > >           struct dpdk_public_struct_s1 s1;
> > > >           struct dpdk_public_struct_s2 s2;
> > > >           struct app_sepecific_struct_s3 s3;
> > > > }
> > > >
> > > Not sure what exactly you mean here. The actual offsets and sizes of the
> > > structs will obviously change as you change the struct, but the end
> > > compiled app has no idea of structs, all it knows of is offsets, which is
> > > why you provide ABI compatible versions of the functions which use "legacy"
> > > copies of the structs to ensure correct offsets. It's pretty much standard
> > > practice for ABI versioning.
> >
> > Currently, We have only function versioning(not structure versioning).
> > Are you suggesting having structure versioning?
> > Will it complicate the code in terms of readability and supporting multiple
> > structure versions aginst its support functions.
> >
>
> We don't, and can't version structures, only functions are versioned.
> Even if we do what you suggest and add a block of 64-bytes expansion room
> at the end of the structures, how is the function you are calling expected
> to know what the structure actually contains? For example, if you add a
> field to the end, and reduce the padding by 8 bytes, your structure is
> still the same size, and how does the called function know whether X or X+8
> bytes are valid in it. Basically, you still need to version all
> functions using the structure, just as if you didn't bother extending the
> struct.

Yes. We need function versioning for sure if we change the behavior of
the function.
Is function version + reserved field enough to decode the correct value from
the reserved filed from the structure.

My concern is, Let say, we are making the change in structure a->b and
function c->d
assisted with it.

In the reserved filed case:
- struct a remains same(we will adding the fields in reserved filed)
- the function will have c and d version and both using struct a

In another scheme:
- The application needs to change where versioned function(c or d) need to
give associate structure manually. Right? If it is manually, it will
be huge change
in application. Right?

How an application can express the correct structure mapping?
Will it it be like
rte_substrem_v21(struct rte_subsystem_conf_v21 *config)?
vs
rte_substrem_v21(struct rte_subsystem_conf *config)?
where rte_subsystem_conf  has reserved filed.

>
> > >
> > > The real complication arises because the actual eventdev driver functions
> > > are not called directly with the linker resolving symbol versioning.
> > > Instead they are called using function pointers from the code. This is why
> > > one needs to add in the additional parameter to the driver APIs so that the
> > > ABI info - be it struct size or version - can be passed from the versioned
> > > eventdev library function through to the driver.
> >
> > If I understand it correctly, it is easy for rawdev subsystem as
> > config structure as _opaque_.
> > But in the case for ethdev, cryptodev, eventdev, config structure are
> > not opaque.
> >
>
> Well, actually it's more complicated for rawdev because the structures are
> opaque, so the value passed could be anything. With eventdev, they will
> ever only be one or two possible values, so you just need enough info to
> distinguish if it's the structure from version X, or the structure from
> version Y you are passing.
>
> > If we take structure versioning, Is n't call for adding structure
> > version change to
> > functions that's been associated with(Kind of M x N case to make a
> > structure change. Here M means
> > offending structure to make change and N means function associated
> > with structure)
> >
> Yes, to version any structure you basically do a new version of every
> function using it, or using the changed fields. However, adding padding is
> not going to remove that need.

See above.

>
> Regards,
> /Bruce
Kinsella, Ray Aug. 5, 2020, 8:46 a.m. UTC | #8
On 04/08/2020 17:20, Stephen Hemminger wrote:
> On Tue, 4 Aug 2020 11:41:53 +0100
> Bruce Richardson <bruce.richardson@intel.com> wrote:
> 
>> On Mon, Aug 03, 2020 at 12:59:03PM +0530, pbhagavatula@marvell.com wrote:
>>> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>>
>>> Add 64 byte padding at the end of event device public structure to allow
>>> future extensions.
>>>
>>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>> Acked-by: Jerin Jacob <jerinj@marvell.com>
>>> ---
>>>  v2 Changes:
>>>  - Modify commit title.
>>>  - Add patch reference to doc.
>>>
>>>  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 ea4cfa7a4..ec5db68e9 100644
>>> --- a/doc/guides/rel_notes/deprecation.rst
>>> +++ b/doc/guides/rel_notes/deprecation.rst
>>> @@ -151,3 +151,14 @@ Deprecation Notices
>>>    Python 2 support will be completely removed in 20.11.
>>>    In 20.08, explicit deprecation warnings will be displayed when running
>>>    scripts with Python 2.
>>> +
>>> +* eventdev: A 64 byte padding is added at the end of the following structures
>>> +  in event device library to support future extensions:
>>> +  ``rte_event_crypto_adapter_conf``, ``rte_event_eth_rx_adapter_conf``,
>>> +  ``rte_event_eth_rx_adapter_queue_conf``, ``rte_event_eth_tx_adapter_conf``,
>>> +  ``rte_event_timer_adapter_conf``, ``rte_event_timer_adapter_info``,
>>> +  ``rte_event_dev_info``, ``rte_event_dev_config``, ``rte_event_queue_conf``,
>>> +  ``rte_event_port_conf``, ``rte_event_timer_adapter``,
>>> +  ``rte_event_timer_adapter_data``.
>>> +  Reference:
>>> +  http://patches.dpdk.org/project/dpdk/list/?series=10728&archive=both&state=*
>>> --  
>>
>> I don't like this idea of adding lots of padding to the ends of these
>> structures. For some structures, such as the public arrays for devices it
>> may be necessary, but for all the conf structures passed as parameters to
>> functions I think we can do better. Since these structures are passed by
>> the user to various functions, function versioning can be used to ensure
>> that the correct function in eventdev is always called. From there to the
>> individual PMDs, we can implement ABI compatibility by either:
>> 1. including the length of the struct as a parameter to the driver. (This is
>>   a bit similar to my proposal for rawdev [1])
>> 2. including the ABI version as a parameter to the driver.
>>
>> Regards
>> /Bruce
>>
>> [1] http://inbox.dpdk.org/dev/?q=enhance+rawdev+APIs
> 
> This is a bad idea.
> 
> Reserved fields won't work because nothing requires that the application
> zero them. You can't start using them later because the application
> may put uninitialized or junk data there.
> 

+1, to Stephens comments.
Jerin Jacob Aug. 5, 2020, 9:10 a.m. UTC | #9
On Wed, Aug 5, 2020 at 2:16 PM Kinsella, Ray <mdr@ashroe.eu> wrote:
>
>
>
> On 04/08/2020 17:20, Stephen Hemminger wrote:
> > On Tue, 4 Aug 2020 11:41:53 +0100
> > Bruce Richardson <bruce.richardson@intel.com> wrote:
> >
> >> On Mon, Aug 03, 2020 at 12:59:03PM +0530, pbhagavatula@marvell.com wrote:
> >>> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> >>>
> >>> Add 64 byte padding at the end of event device public structure to allow
> >>> future extensions.
> >>>
> >>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> >>> Acked-by: Jerin Jacob <jerinj@marvell.com>
> >>> ---
> >>>  v2 Changes:
> >>>  - Modify commit title.
> >>>  - Add patch reference to doc.
> >>>
> >>>  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 ea4cfa7a4..ec5db68e9 100644
> >>> --- a/doc/guides/rel_notes/deprecation.rst
> >>> +++ b/doc/guides/rel_notes/deprecation.rst
> >>> @@ -151,3 +151,14 @@ Deprecation Notices
> >>>    Python 2 support will be completely removed in 20.11.
> >>>    In 20.08, explicit deprecation warnings will be displayed when running
> >>>    scripts with Python 2.
> >>> +
> >>> +* eventdev: A 64 byte padding is added at the end of the following structures
> >>> +  in event device library to support future extensions:
> >>> +  ``rte_event_crypto_adapter_conf``, ``rte_event_eth_rx_adapter_conf``,
> >>> +  ``rte_event_eth_rx_adapter_queue_conf``, ``rte_event_eth_tx_adapter_conf``,
> >>> +  ``rte_event_timer_adapter_conf``, ``rte_event_timer_adapter_info``,
> >>> +  ``rte_event_dev_info``, ``rte_event_dev_config``, ``rte_event_queue_conf``,
> >>> +  ``rte_event_port_conf``, ``rte_event_timer_adapter``,
> >>> +  ``rte_event_timer_adapter_data``.
> >>> +  Reference:
> >>> +  http://patches.dpdk.org/project/dpdk/list/?series=10728&archive=both&state=*
> >>> --
> >>
> >> I don't like this idea of adding lots of padding to the ends of these
> >> structures. For some structures, such as the public arrays for devices it
> >> may be necessary, but for all the conf structures passed as parameters to
> >> functions I think we can do better. Since these structures are passed by
> >> the user to various functions, function versioning can be used to ensure
> >> that the correct function in eventdev is always called. From there to the
> >> individual PMDs, we can implement ABI compatibility by either:
> >> 1. including the length of the struct as a parameter to the driver. (This is
> >>   a bit similar to my proposal for rawdev [1])
> >> 2. including the ABI version as a parameter to the driver.
> >>
> >> Regards
> >> /Bruce
> >>
> >> [1] http://inbox.dpdk.org/dev/?q=enhance+rawdev+APIs
> >
> > This is a bad idea.
> >
> > Reserved fields won't work because nothing requires that the application
> > zero them. You can't start using them later because the application
> > may put uninitialized or junk data there.
> >
>
> +1, to Stephens comments.

Since the problem is not specific to one substem, if we need to add a
field in config structures,
What will the expected way of handling across the DPDK?

How about

1) Public init functions to clear the params?
2) Different struct version for specific functions like
http://mails.dpdk.org/archives/dev/2020-August/177357.html

Or any other scheme in mind?
Kinsella, Ray Aug. 5, 2020, 9:18 a.m. UTC | #10
On 04/08/2020 18:18, Jerin Jacob wrote:
> On Tue, Aug 4, 2020 at 9:54 PM Bruce Richardson
> <bruce.richardson@intel.com> wrote:
>>
>> On Tue, Aug 04, 2020 at 09:33:14PM +0530, Jerin Jacob wrote:
>>> On Tue, Aug 4, 2020 at 7:55 PM Bruce Richardson
>>> <bruce.richardson@intel.com> wrote:
>>>>
>>>> On Tue, Aug 04, 2020 at 05:07:12PM +0530, Jerin Jacob wrote:
>>>>> On Tue, Aug 4, 2020 at 4:12 PM Bruce Richardson
>>>>> <bruce.richardson@intel.com> wrote:
>>>>>>
>>>>>> On Mon, Aug 03, 2020 at 12:59:03PM +0530, pbhagavatula@marvell.com wrote:
>>>>>>> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>>>>>>
>>>>>>> Add 64 byte padding at the end of event device public structure to allow
>>>>>>> future extensions.
>>>>>>>
>>>>>>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
>>>>>>> Acked-by: Jerin Jacob <jerinj@marvell.com>
>>>>>>> ---
>>>>>>>  v2 Changes:
>>>>>>>  - Modify commit title.
>>>>>>>  - Add patch reference to doc.
>>>>>>>
>>>>>>>  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 ea4cfa7a4..ec5db68e9 100644
>>>>>>> --- a/doc/guides/rel_notes/deprecation.rst
>>>>>>> +++ b/doc/guides/rel_notes/deprecation.rst
>>>>>>> @@ -151,3 +151,14 @@ Deprecation Notices
>>>>>>>    Python 2 support will be completely removed in 20.11.
>>>>>>>    In 20.08, explicit deprecation warnings will be displayed when running
>>>>>>>    scripts with Python 2.
>>>>>>> +
>>>>>>> +* eventdev: A 64 byte padding is added at the end of the following structures
>>>>>>> +  in event device library to support future extensions:
>>>>>>> +  ``rte_event_crypto_adapter_conf``, ``rte_event_eth_rx_adapter_conf``,
>>>>>>> +  ``rte_event_eth_rx_adapter_queue_conf``, ``rte_event_eth_tx_adapter_conf``,
>>>>>>> +  ``rte_event_timer_adapter_conf``, ``rte_event_timer_adapter_info``,
>>>>>>> +  ``rte_event_dev_info``, ``rte_event_dev_config``, ``rte_event_queue_conf``,
>>>>>>> +  ``rte_event_port_conf``, ``rte_event_timer_adapter``,
>>>>>>> +  ``rte_event_timer_adapter_data``.
>>>>>>> +  Reference:
>>>>>>> +  http://patches.dpdk.org/project/dpdk/list/?series=10728&archive=both&state=*
>>>>>>> --
>>>>>>
>>>>>> I don't like this idea of adding lots of padding to the ends of these
>>>>>> structures. For some structures, such as the public arrays for devices it
>>>>>> may be necessary, but for all the conf structures passed as parameters to
>>>>>> functions I think we can do better. Since these structures are passed by
>>>>>> the user to various functions, function versioning can be used to ensure
>>>>>> that the correct function in eventdev is always called. From there to the
>>>>>> individual PMDs, we can implement ABI compatibility by either:
>>>>>> 1. including the length of the struct as a parameter to the driver. (This is
>>>>>>   a bit similar to my proposal for rawdev [1])
>>>>>> 2. including the ABI version as a parameter to the driver.
>>>>>
>>>>> But, Will the above solution work if the application is dependent on
>>>>> struct size?
>>>>> i.e change of s1 size will change offset of s3 i.e
>>>>> app_sepecific_struct_s3. Right?
>>>>> i.e DPDK version should not change the offset of s3. Right?
>>>>>
>>>>> example,
>>>>> struct app_struct {
>>>>>           struct dpdk_public_struct_s1 s1;
>>>>>           struct dpdk_public_struct_s2 s2;
>>>>>           struct app_sepecific_struct_s3 s3;
>>>>> }
>>>>>
>>>> Not sure what exactly you mean here. The actual offsets and sizes of the
>>>> structs will obviously change as you change the struct, but the end
>>>> compiled app has no idea of structs, all it knows of is offsets, which is
>>>> why you provide ABI compatible versions of the functions which use "legacy"
>>>> copies of the structs to ensure correct offsets. It's pretty much standard
>>>> practice for ABI versioning.
>>>
>>> Currently, We have only function versioning(not structure versioning).
>>> Are you suggesting having structure versioning?
>>> Will it complicate the code in terms of readability and supporting multiple
>>> structure versions aginst its support functions.
>>>
>>
>> We don't, and can't version structures, only functions are versioned.
>> Even if we do what you suggest and add a block of 64-bytes expansion room
>> at the end of the structures, how is the function you are calling expected
>> to know what the structure actually contains? For example, if you add a
>> field to the end, and reduce the padding by 8 bytes, your structure is
>> still the same size, and how does the called function know whether X or X+8
>> bytes are valid in it. Basically, you still need to version all
>> functions using the structure, just as if you didn't bother extending the
>> struct.
> 
> Yes. We need function versioning for sure if we change the behavior of
> the function.
> Is function version + reserved field enough to decode the correct value from
> the reserved filed from the structure.
> 
> My concern is, Let say, we are making the change in structure a->b and
> function c->d
> assisted with it.
> 
> In the reserved filed case:
> - struct a remains same(we will adding the fields in reserved filed)
> - the function will have c and d version and both using struct a
> 
> In another scheme:
> - The application needs to change where versioned function(c or d) need to
> give associate structure manually. Right? If it is manually, it will
> be huge change
> in application. Right?
> 
> How an application can express the correct structure mapping?
> Will it it be like
> rte_substrem_v21(struct rte_subsystem_conf_v21 *config)?
> vs
> rte_substrem_v21(struct rte_subsystem_conf *config)?> where rte_subsystem_conf  has reserved filed.

So the ABI policy approach for doing this is 

rte_substrem_v21(struct rte_subsystem_conf_v21 *config)

instead of 

rte_substrem_v21(struct rte_subsystem_conf *config) (with extension padding). 

There are benefits and drawbacks with each approach, these include ... 

The padding approach assumes you are always happy to tack whatever field you want 
onto the end of the structure, when in many cases it's more natural home is usually
in the middle or beginning. Then there is also dead/unused and uninitialized memory, 
to be conscious of. 

However what you say is completely correct, if I have a v21 version of the function, 
only it should be looking at the new field/additional bytes in the structure. 

The alternative is to version the structure along with the function. 
And this is what is described in the ABI policy.

The obvious drawback with this approach is that if you have a structure that is used across
a large number of functions ... it can be a real headache as they all need to be versioned. 

The benefit with this approach is that it is completely explicit, 
which function and structure versions are associated. 

> 
>>
>>>>
>>>> The real complication arises because the actual eventdev driver functions
>>>> are not called directly with the linker resolving symbol versioning.
>>>> Instead they are called using function pointers from the code. This is why
>>>> one needs to add in the additional parameter to the driver APIs so that the
>>>> ABI info - be it struct size or version - can be passed from the versioned
>>>> eventdev library function through to the driver.
>>>
>>> If I understand it correctly, it is easy for rawdev subsystem as
>>> config structure as _opaque_.
>>> But in the case for ethdev, cryptodev, eventdev, config structure are
>>> not opaque.
>>>
>>
>> Well, actually it's more complicated for rawdev because the structures are
>> opaque, so the value passed could be anything. With eventdev, they will
>> ever only be one or two possible values, so you just need enough info to
>> distinguish if it's the structure from version X, or the structure from
>> version Y you are passing.
>>
>>> If we take structure versioning, Is n't call for adding structure
>>> version change to
>>> functions that's been associated with(Kind of M x N case to make a
>>> structure change. Here M means
>>> offending structure to make change and N means function associated
>>> with structure)
>>>
>> Yes, to version any structure you basically do a new version of every
>> function using it, or using the changed fields. However, adding padding is
>> not going to remove that need.
> 
> See above.
> 
>>
>> Regards,
>> /Bruce
Bruce Richardson Aug. 5, 2020, 10:07 a.m. UTC | #11
On Wed, Aug 05, 2020 at 10:18:41AM +0100, Kinsella, Ray wrote:
> 
> 
> On 04/08/2020 18:18, Jerin Jacob wrote:
> > On Tue, Aug 4, 2020 at 9:54 PM Bruce Richardson
> > <bruce.richardson@intel.com> wrote:
> >>
> >> On Tue, Aug 04, 2020 at 09:33:14PM +0530, Jerin Jacob wrote:
> >>> On Tue, Aug 4, 2020 at 7:55 PM Bruce Richardson
> >>> <bruce.richardson@intel.com> wrote:
> >>>>
> >>>> On Tue, Aug 04, 2020 at 05:07:12PM +0530, Jerin Jacob wrote:
> >>>>> On Tue, Aug 4, 2020 at 4:12 PM Bruce Richardson
> >>>>> <bruce.richardson@intel.com> wrote:
> >>>>>>
> >>>>>> On Mon, Aug 03, 2020 at 12:59:03PM +0530, pbhagavatula@marvell.com wrote:
> >>>>>>> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> >>>>>>>
> >>>>>>> Add 64 byte padding at the end of event device public structure to allow
> >>>>>>> future extensions.
> >>>>>>>
> >>>>>>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> >>>>>>> Acked-by: Jerin Jacob <jerinj@marvell.com>
> >>>>>>> ---
> >>>>>>>  v2 Changes:
> >>>>>>>  - Modify commit title.
> >>>>>>>  - Add patch reference to doc.
> >>>>>>>
> >>>>>>>  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 ea4cfa7a4..ec5db68e9 100644
> >>>>>>> --- a/doc/guides/rel_notes/deprecation.rst
> >>>>>>> +++ b/doc/guides/rel_notes/deprecation.rst
> >>>>>>> @@ -151,3 +151,14 @@ Deprecation Notices
> >>>>>>>    Python 2 support will be completely removed in 20.11.
> >>>>>>>    In 20.08, explicit deprecation warnings will be displayed when running
> >>>>>>>    scripts with Python 2.
> >>>>>>> +
> >>>>>>> +* eventdev: A 64 byte padding is added at the end of the following structures
> >>>>>>> +  in event device library to support future extensions:
> >>>>>>> +  ``rte_event_crypto_adapter_conf``, ``rte_event_eth_rx_adapter_conf``,
> >>>>>>> +  ``rte_event_eth_rx_adapter_queue_conf``, ``rte_event_eth_tx_adapter_conf``,
> >>>>>>> +  ``rte_event_timer_adapter_conf``, ``rte_event_timer_adapter_info``,
> >>>>>>> +  ``rte_event_dev_info``, ``rte_event_dev_config``, ``rte_event_queue_conf``,
> >>>>>>> +  ``rte_event_port_conf``, ``rte_event_timer_adapter``,
> >>>>>>> +  ``rte_event_timer_adapter_data``.
> >>>>>>> +  Reference:
> >>>>>>> +  http://patches.dpdk.org/project/dpdk/list/?series=10728&archive=both&state=*
> >>>>>>> --
> >>>>>>
> >>>>>> I don't like this idea of adding lots of padding to the ends of these
> >>>>>> structures. For some structures, such as the public arrays for devices it
> >>>>>> may be necessary, but for all the conf structures passed as parameters to
> >>>>>> functions I think we can do better. Since these structures are passed by
> >>>>>> the user to various functions, function versioning can be used to ensure
> >>>>>> that the correct function in eventdev is always called. From there to the
> >>>>>> individual PMDs, we can implement ABI compatibility by either:
> >>>>>> 1. including the length of the struct as a parameter to the driver. (This is
> >>>>>>   a bit similar to my proposal for rawdev [1])
> >>>>>> 2. including the ABI version as a parameter to the driver.
> >>>>>
> >>>>> But, Will the above solution work if the application is dependent on
> >>>>> struct size?
> >>>>> i.e change of s1 size will change offset of s3 i.e
> >>>>> app_sepecific_struct_s3. Right?
> >>>>> i.e DPDK version should not change the offset of s3. Right?
> >>>>>
> >>>>> example,
> >>>>> struct app_struct {
> >>>>>           struct dpdk_public_struct_s1 s1;
> >>>>>           struct dpdk_public_struct_s2 s2;
> >>>>>           struct app_sepecific_struct_s3 s3;
> >>>>> }
> >>>>>
> >>>> Not sure what exactly you mean here. The actual offsets and sizes of the
> >>>> structs will obviously change as you change the struct, but the end
> >>>> compiled app has no idea of structs, all it knows of is offsets, which is
> >>>> why you provide ABI compatible versions of the functions which use "legacy"
> >>>> copies of the structs to ensure correct offsets. It's pretty much standard
> >>>> practice for ABI versioning.
> >>>
> >>> Currently, We have only function versioning(not structure versioning).
> >>> Are you suggesting having structure versioning?
> >>> Will it complicate the code in terms of readability and supporting multiple
> >>> structure versions aginst its support functions.
> >>>
> >>
> >> We don't, and can't version structures, only functions are versioned.
> >> Even if we do what you suggest and add a block of 64-bytes expansion room
> >> at the end of the structures, how is the function you are calling expected
> >> to know what the structure actually contains? For example, if you add a
> >> field to the end, and reduce the padding by 8 bytes, your structure is
> >> still the same size, and how does the called function know whether X or X+8
> >> bytes are valid in it. Basically, you still need to version all
> >> functions using the structure, just as if you didn't bother extending the
> >> struct.
> > 
> > Yes. We need function versioning for sure if we change the behavior of
> > the function.
> > Is function version + reserved field enough to decode the correct value from
> > the reserved filed from the structure.
> > 
> > My concern is, Let say, we are making the change in structure a->b and
> > function c->d
> > assisted with it.
> > 
> > In the reserved filed case:
> > - struct a remains same(we will adding the fields in reserved filed)
> > - the function will have c and d version and both using struct a
> > 
> > In another scheme:
> > - The application needs to change where versioned function(c or d) need to
> > give associate structure manually. Right? If it is manually, it will
> > be huge change
> > in application. Right?
> > 
> > How an application can express the correct structure mapping?
> > Will it it be like
> > rte_substrem_v21(struct rte_subsystem_conf_v21 *config)?
> > vs
> > rte_substrem_v21(struct rte_subsystem_conf *config)?> where rte_subsystem_conf  has reserved filed.
> 
> So the ABI policy approach for doing this is 
> 
> rte_substrem_v21(struct rte_subsystem_conf_v21 *config)
> 
> instead of 
> 
> rte_substrem_v21(struct rte_subsystem_conf *config) (with extension padding). 
> 
> There are benefits and drawbacks with each approach, these include ... 
> 
> The padding approach assumes you are always happy to tack whatever field you want 
> onto the end of the structure, when in many cases it's more natural home is usually
> in the middle or beginning. Then there is also dead/unused and uninitialized memory, 
> to be conscious of. 
> 
> However what you say is completely correct, if I have a v21 version of the function, 
> only it should be looking at the new field/additional bytes in the structure. 
> 
> The alternative is to version the structure along with the function. 
> And this is what is described in the ABI policy.
> 
> The obvious drawback with this approach is that if you have a structure that is used across
> a large number of functions ... it can be a real headache as they all need to be versioned. 
> 
> The benefit with this approach is that it is completely explicit, 
> which function and structure versions are associated. 
> 

IMHO, the only time we should look to use padding is for structures like
the ethdev structure that are internal-only but used across so many
functions and inline functions that it simply becomes impossible to version
them. For any structures that are simply passed as parameters, which I
believe applies to all structs referred to by this deprecation notice,
function versioning should be sufficient to deal with any issues.

Regards,
/Bruce
Stephen Hemminger Aug. 6, 2020, 12:59 a.m. UTC | #12
On Wed, 5 Aug 2020 14:40:01 +0530
Jerin Jacob <jerinjacobk@gmail.com> wrote:

> On Wed, Aug 5, 2020 at 2:16 PM Kinsella, Ray <mdr@ashroe.eu> wrote:
> >
> >
> >
> > On 04/08/2020 17:20, Stephen Hemminger wrote:  
> > > On Tue, 4 Aug 2020 11:41:53 +0100
> > > Bruce Richardson <bruce.richardson@intel.com> wrote:
> > >  
> > >> On Mon, Aug 03, 2020 at 12:59:03PM +0530, pbhagavatula@marvell.com wrote:  
> > >>> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > >>>
> > >>> Add 64 byte padding at the end of event device public structure to allow
> > >>> future extensions.
> > >>>
> > >>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > >>> Acked-by: Jerin Jacob <jerinj@marvell.com>
> > >>> ---
> > >>>  v2 Changes:
> > >>>  - Modify commit title.
> > >>>  - Add patch reference to doc.
> > >>>
> > >>>  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 ea4cfa7a4..ec5db68e9 100644
> > >>> --- a/doc/guides/rel_notes/deprecation.rst
> > >>> +++ b/doc/guides/rel_notes/deprecation.rst
> > >>> @@ -151,3 +151,14 @@ Deprecation Notices
> > >>>    Python 2 support will be completely removed in 20.11.
> > >>>    In 20.08, explicit deprecation warnings will be displayed when running
> > >>>    scripts with Python 2.
> > >>> +
> > >>> +* eventdev: A 64 byte padding is added at the end of the following structures
> > >>> +  in event device library to support future extensions:
> > >>> +  ``rte_event_crypto_adapter_conf``, ``rte_event_eth_rx_adapter_conf``,
> > >>> +  ``rte_event_eth_rx_adapter_queue_conf``, ``rte_event_eth_tx_adapter_conf``,
> > >>> +  ``rte_event_timer_adapter_conf``, ``rte_event_timer_adapter_info``,
> > >>> +  ``rte_event_dev_info``, ``rte_event_dev_config``, ``rte_event_queue_conf``,
> > >>> +  ``rte_event_port_conf``, ``rte_event_timer_adapter``,
> > >>> +  ``rte_event_timer_adapter_data``.
> > >>> +  Reference:
> > >>> +  http://patches.dpdk.org/project/dpdk/list/?series=10728&archive=both&state=*
> > >>> --  
> > >>
> > >> I don't like this idea of adding lots of padding to the ends of these
> > >> structures. For some structures, such as the public arrays for devices it
> > >> may be necessary, but for all the conf structures passed as parameters to
> > >> functions I think we can do better. Since these structures are passed by
> > >> the user to various functions, function versioning can be used to ensure
> > >> that the correct function in eventdev is always called. From there to the
> > >> individual PMDs, we can implement ABI compatibility by either:
> > >> 1. including the length of the struct as a parameter to the driver. (This is
> > >>   a bit similar to my proposal for rawdev [1])
> > >> 2. including the ABI version as a parameter to the driver.
> > >>
> > >> Regards
> > >> /Bruce
> > >>
> > >> [1] http://inbox.dpdk.org/dev/?q=enhance+rawdev+APIs  
> > >
> > > This is a bad idea.
> > >
> > > Reserved fields won't work because nothing requires that the application
> > > zero them. You can't start using them later because the application
> > > may put uninitialized or junk data there.
> > >  
> >
> > +1, to Stephens comments.  
> 
> Since the problem is not specific to one substem, if we need to add a
> field in config structures,
> What will the expected way of handling across the DPDK?

If you need fields go through the normal enhancement process, and get it
reviewed and put them in a major release milestone.
Sorry, there is no free lunch by adding reserved fields.

Look up YAGNI
Jerin Jacob Aug. 6, 2020, 4:57 p.m. UTC | #13
On Thu, Aug 6, 2020 at 6:29 AM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Wed, 5 Aug 2020 14:40:01 +0530
> Jerin Jacob <jerinjacobk@gmail.com> wrote:
>
> > On Wed, Aug 5, 2020 at 2:16 PM Kinsella, Ray <mdr@ashroe.eu> wrote:
> > >
> > >
> > >
> > > On 04/08/2020 17:20, Stephen Hemminger wrote:
> > > > On Tue, 4 Aug 2020 11:41:53 +0100
> > > > Bruce Richardson <bruce.richardson@intel.com> wrote:
> > > >
> > > >> On Mon, Aug 03, 2020 at 12:59:03PM +0530, pbhagavatula@marvell.com wrote:
> > > >>> From: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > > >>>
> > > >>> Add 64 byte padding at the end of event device public structure to allow
> > > >>> future extensions.
> > > >>>
> > > >>> Signed-off-by: Pavan Nikhilesh <pbhagavatula@marvell.com>
> > > >>> Acked-by: Jerin Jacob <jerinj@marvell.com>
> > > >>> ---
> > > >>>  v2 Changes:
> > > >>>  - Modify commit title.
> > > >>>  - Add patch reference to doc.
> > > >>>
> > > >>>  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 ea4cfa7a4..ec5db68e9 100644
> > > >>> --- a/doc/guides/rel_notes/deprecation.rst
> > > >>> +++ b/doc/guides/rel_notes/deprecation.rst
> > > >>> @@ -151,3 +151,14 @@ Deprecation Notices
> > > >>>    Python 2 support will be completely removed in 20.11.
> > > >>>    In 20.08, explicit deprecation warnings will be displayed when running
> > > >>>    scripts with Python 2.
> > > >>> +
> > > >>> +* eventdev: A 64 byte padding is added at the end of the following structures
> > > >>> +  in event device library to support future extensions:
> > > >>> +  ``rte_event_crypto_adapter_conf``, ``rte_event_eth_rx_adapter_conf``,
> > > >>> +  ``rte_event_eth_rx_adapter_queue_conf``, ``rte_event_eth_tx_adapter_conf``,
> > > >>> +  ``rte_event_timer_adapter_conf``, ``rte_event_timer_adapter_info``,
> > > >>> +  ``rte_event_dev_info``, ``rte_event_dev_config``, ``rte_event_queue_conf``,
> > > >>> +  ``rte_event_port_conf``, ``rte_event_timer_adapter``,
> > > >>> +  ``rte_event_timer_adapter_data``.
> > > >>> +  Reference:
> > > >>> +  http://patches.dpdk.org/project/dpdk/list/?series=10728&archive=both&state=*
> > > >>> --
> > > >>
> > > >> I don't like this idea of adding lots of padding to the ends of these
> > > >> structures. For some structures, such as the public arrays for devices it
> > > >> may be necessary, but for all the conf structures passed as parameters to
> > > >> functions I think we can do better. Since these structures are passed by
> > > >> the user to various functions, function versioning can be used to ensure
> > > >> that the correct function in eventdev is always called. From there to the
> > > >> individual PMDs, we can implement ABI compatibility by either:
> > > >> 1. including the length of the struct as a parameter to the driver. (This is
> > > >>   a bit similar to my proposal for rawdev [1])
> > > >> 2. including the ABI version as a parameter to the driver.
> > > >>
> > > >> Regards
> > > >> /Bruce
> > > >>
> > > >> [1] http://inbox.dpdk.org/dev/?q=enhance+rawdev+APIs
> > > >
> > > > This is a bad idea.
> > > >
> > > > Reserved fields won't work because nothing requires that the application
> > > > zero them. You can't start using them later because the application
> > > > may put uninitialized or junk data there.
> > > >
> > >
> > > +1, to Stephens comments.
> >
> > Since the problem is not specific to one substem, if we need to add a
> > field in config structures,
> > What will the expected way of handling across the DPDK?
>
> If you need fields go through the normal enhancement process, and get it
> reviewed and put them in a major release milestone.
> Sorry, there is no free lunch by adding reserved fields.
>
> Look up YAGNI

YAGNI is useful. But If we need to wait for one year to change the API
then it is the problem.
That's time frame silicon companies are making the next generation of
silicon nowadays.

We just tried to follow the existing scheme of things in the codebase[1].

But I am also not a great fan of the Reserved filed scheme either.
Probably it is better to add the new feature as a new API or config
structure with
out breaking anything(as experimental) and upcoming next release, rework to
adapt to subsystem API philosophy.


[1]
lib/librte_ethdev/rte_ethdev.h: uint64_t reserved_64s[2]; /**<
Reserved for future fields */
lib/librte_ethdev/rte_ethdev.h: void *reserved_ptrs[2];   /**<
Reserved for future fields */
lib/librte_ethdev/rte_ethdev.h: uint64_t reserved_64s[2]; /**<
Reserved for future fields */
lib/librte_ethdev/rte_ethdev.h: void *reserved_ptrs[2];   /**<
Reserved for future fields */
lib/librte_ethdev/rte_ethdev.h: uint64_t reserved_64s[2]; /**<
Reserved for future fields */
lib/librte_ethdev/rte_ethdev.h: void *reserved_ptrs[2];   /**<
Reserved for future fields */
lib/librte_ethdev/rte_ethdev.h: uint64_t reserved_64s[2]; /**<
Reserved for future fields */
lib/librte_ethdev/rte_ethdev.h: void *reserved_ptrs[2];   /**<
Reserved for future fields */
lib/librte_ethdev/rte_ethdev.h: uint64_t reserved_64s[2]; /**<
Reserved for future fields */
lib/librte_ethdev/rte_ethdev.h: void *reserved_ptrs[2];   /**<
Reserved for future fields */
lib/librte_ethdev/rte_ethdev_core.h:    uint64_t reserved_64s[4]; /**<
Reserved for future fields */
lib/librte_ethdev/rte_ethdev_core.h:    void *reserved_ptrs[4];   /**<
Reserved for future fields */
lib/librte_ethdev/rte_ethdev_core.h:    uint64_t reserved_64s[4]; /**<
Reserved for future fields */
lib/librte_ethdev/rte_ethdev_core.h:    void *reserved_ptrs[4];   /**<
Reserved for future fields */
lib/librte_eal/linux/eal_timer.c:       uint64_t reserved0;
/**< Reserved for future use. */
lib/librte_eal/linux/eal_timer.c:       uint64_t reserved1;
/**< Reserved for future use. */
lib/librte_eal/linux/eal_timer.c:       uint64_t reserved2[25];
/**< Reserved for future use. */
lib/librte_eal/linux/eal_timer.c:       uint64_t reserved3;
/**< Reserved for future use. */
lib/librte_eal/linux/eal_timer.c:               uint64_t reserved4;
/**< Reserved for future use. */
lib/librte_eventdev/rte_eventdev.h:                     /**< Reserved
for future use */
lib/librte_eventdev/rte_eventdev.h:     uint64_t reserved_64s[4]; /**<
Reserved for future fields */
lib/librte_eventdev/rte_eventdev.h:     void *reserved_ptrs[4];   /**<
Reserved for future fields */
lib/librte_eventdev/rte_eventdev.h:     uint64_t reserved_64s[4]; /**<
Reserved for future fields */
lib/librte_eventdev/rte_eventdev.h:     void *reserved_ptrs[4];   /**<
Reserved for future fields */

Patch
diff mbox series

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index ea4cfa7a4..ec5db68e9 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -151,3 +151,14 @@  Deprecation Notices
   Python 2 support will be completely removed in 20.11.
   In 20.08, explicit deprecation warnings will be displayed when running
   scripts with Python 2.
+
+* eventdev: A 64 byte padding is added at the end of the following structures
+  in event device library to support future extensions:
+  ``rte_event_crypto_adapter_conf``, ``rte_event_eth_rx_adapter_conf``,
+  ``rte_event_eth_rx_adapter_queue_conf``, ``rte_event_eth_tx_adapter_conf``,
+  ``rte_event_timer_adapter_conf``, ``rte_event_timer_adapter_info``,
+  ``rte_event_dev_info``, ``rte_event_dev_config``, ``rte_event_queue_conf``,
+  ``rte_event_port_conf``, ``rte_event_timer_adapter``,
+  ``rte_event_timer_adapter_data``.
+  Reference:
+  http://patches.dpdk.org/project/dpdk/list/?series=10728&archive=both&state=*