DPDK patches and discussions
 help / color / mirror / Atom feed
From: Bruce Richardson <bruce.richardson@intel.com>
To: Jerin Jacob <jerinjacobk@gmail.com>
Cc: Pavan Nikhilesh <pbhagavatula@marvell.com>,
	Jerin Jacob <jerinj@marvell.com>, Ray Kinsella <mdr@ashroe.eu>,
	Neil Horman <nhorman@tuxdriver.com>,
	John McNamara <john.mcnamara@intel.com>,
	Marko Kovacevic <marko.kovacevic@intel.com>,
	dpdk-dev <dev@dpdk.org>, Thomas Monjalon <thomas@monjalon.net>,
	David Marchand <david.marchand@redhat.com>
Subject: Re: [dpdk-dev] [PATCH v2] doc: add reserve fields to eventdev public structures
Date: Tue, 4 Aug 2020 17:24:10 +0100
Message-ID: <20200804162410.GD1704@bricha3-MOBL.ger.corp.intel.com> (raw)
In-Reply-To: <CALBAE1OOG4VhtydOS0Z5PMDh7_7XjbHXt8UjgHT6SxZ5JyTD2Q@mail.gmail.com>

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

  reply	other threads:[~2020-08-04 16:24 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-02 10:51 [dpdk-dev] [PATCH] doc: announce changes " pbhagavatula
2020-08-03  6:14 ` Jerin Jacob
2020-08-03  7:29 ` [dpdk-dev] [PATCH v2] doc: add reserve fields " pbhagavatula
2020-08-04 10:41   ` Bruce Richardson
2020-08-04 11:37     ` Jerin Jacob
2020-08-04 14:24       ` Bruce Richardson
2020-08-04 16:03         ` Jerin Jacob
2020-08-04 16:24           ` Bruce Richardson [this message]
2020-08-04 17:18             ` Jerin Jacob
2020-08-05  9:18               ` Kinsella, Ray
2020-08-05 10:07                 ` Bruce Richardson
2020-08-04 16:20     ` Stephen Hemminger
2020-08-05  8:46       ` Kinsella, Ray
2020-08-05  9:10         ` Jerin Jacob
2020-08-06  0:59           ` Stephen Hemminger
2020-08-06 16:57             ` Jerin Jacob

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200804162410.GD1704@bricha3-MOBL.ger.corp.intel.com \
    --to=bruce.richardson@intel.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=jerinj@marvell.com \
    --cc=jerinjacobk@gmail.com \
    --cc=john.mcnamara@intel.com \
    --cc=marko.kovacevic@intel.com \
    --cc=mdr@ashroe.eu \
    --cc=nhorman@tuxdriver.com \
    --cc=pbhagavatula@marvell.com \
    --cc=thomas@monjalon.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git