DPDK patches and discussions
 help / color / mirror / Atom feed
From: David Marchand <david.marchand@redhat.com>
To: Andrew Rybchenko <arybchenko@solarflare.com>,
	Thomas Monjalon <thomas@monjalon.net>
Cc: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
	Jerin Jacob <jerinjacobk@gmail.com>,
	 Ray Kinsella <mdr@ashroe.eu>, dpdk-dev <dev@dpdk.org>,
	 "Richardson, Bruce" <bruce.richardson@intel.com>,
	Jerin Jacob Kollanukkaran <jerinj@marvell.com>,
	 Hemant Agrawal <hemant.agrawal@nxp.com>,
	Stephen Hemminger <stephen@networkplumber.org>,
	 "Yigit, Ferruh" <ferruh.yigit@intel.com>,
	 "maxime.coquelin@redhat.com" <maxime.coquelin@redhat.com>,
	 "Zapolski, MarcinX A" <marcinx.a.zapolski@intel.com>,
	Ian Stokes <ian.stokes@intel.com>,
	Ilya Maximets <i.maximets@samsung.com>
Subject: Re: [dpdk-dev] RFC: hiding struct rte_eth_dev
Date: Thu, 26 Sep 2019 13:50:29 +0200	[thread overview]
Message-ID: <CAJFAV8yq_b0myBOacRQ6JNhTAOjhb7yEq46tb80QmiggkhiCgg@mail.gmail.com> (raw)
In-Reply-To: <90610982-ac3e-314e-a623-81610e591958@solarflare.com>

On Thu, Sep 26, 2019 at 1:13 PM Andrew Rybchenko
<arybchenko@solarflare.com> wrote:
>
> On 9/24/19 7:50 PM, Ananyev, Konstantin wrote:
>
> Hi everyone,
>
> Hi folks,
>
> The ABI Stability proposals should be pretty well known at this point.
> The latest rev is here ...
>
> http://inbox.dpdk.org/dev/1565864619-17206-1-git-send-email-mdr@ashroe.eu/
>
> As has been discussed public data structure's are risky for ABI
> stability, as any changes to a data structure can change the ABI. As a
> general rule you want to expose as few as possible (ideally none), and
> keep them as small as possible.
>
> One of the key data structures in DPDK is `struct rte_eth_dev`. In this
> case, rte_eth_dev is exposed public-ally, as a side-effect of the
> inlining of the [rx,tx]_burst functions.
>
> Marcin Zapolski has been looking at what to do about it, with no current
> consensus on a path forward. The options on our table is:-
>
> 1. Do nothing, live with the risk to DPDK v20 ABI stability.
>
> 2. Pad rte_eth_dev, add some extra bytes to the structure "in case" we
> need to add a field during the v20 ABI (through to 20.11).
>
> 3. Break rte_eth_dev into public and private structs.
>   - See
> http://inbox.dpdk.org/dev/20190906131813.1343-1-marcinx.a.zapolski@intel.com/
>   - This ends up quiet an invasive patch, late in the cycle, however it
> does have no performance penalty.
>
> 4. Uninline [rx,tx]_burst functions
>  -  See
> http://inbox.dpdk.org/dev/20190730124950.1293-1-marcinx.a.zapolski@intel.com/
>  - This has a performance penalty of ~2% with testpmd, impact on a "real
> workload" is likely to be in the noise.
>
> We need to agree an approach for v19.11, and that may be we agree to do
> nothing. My personal vote is 4. as the simplest with minimal impact.
>
> My preference NOT to do #4. Reasons are:
> - I have seen performance drop from 1.5% to 3.5% based on the arm64
> cores in use(Embedded vs Server cores)
> -  We need the correct approach to cater to cryptodev and eventdev as
> well. If #4 is checked in, We will
> take shotcut for cryptodev and eventdev
>
> My preference  #1, do nothing, is probably ok and could live with #2,
> adding padding,
> and fix properly with #3 as when needed and use #3 scheme for crypto
> dev and eventdev as well.
>
>
> My preference would be #4 also.
> If that's not an option, then I suppose #1 for 19.11 and #3 for next release
> when ABI breakage would be allowed.
> BTW, good point that we need similar thing for other dev types too.
> Konstantin
>
>
> My preference would be #4 or #1.
> #2 and #3 are both tradeoffs and do not resolve ABI breaking completely.
> #3 is really invasive, it requires changes of driverRx/Tx burst prototypes and
> uninline descriptor status functions (may be it would be better to change
> callback prototypes as well, but keep functions inline).
> #4 is better since it is really a step to ABI stability and it still allow to
> do many generic checks (dev->data dependent) on ethdev API level.

Did we ensure that external users have all the required api before
hiding the rte_eth_dev struct?
ovs still accesses rte_eth_devices[].

CC Ian and Ilya.


-- 
David Marchand

  reply	other threads:[~2019-09-26 11:50 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-09-23 16:19 Ray Kinsella
2019-09-23 16:35 ` Bruce Richardson
2019-09-24  9:07 ` Morten Brørup
2019-09-24 16:42 ` Jerin Jacob
2019-09-24 16:50   ` Ananyev, Konstantin
2019-09-26 11:13     ` Andrew Rybchenko
2019-09-26 11:50       ` David Marchand [this message]
2019-09-26 11:52         ` David Marchand

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=CAJFAV8yq_b0myBOacRQ6JNhTAOjhb7yEq46tb80QmiggkhiCgg@mail.gmail.com \
    --to=david.marchand@redhat.com \
    --cc=arybchenko@solarflare.com \
    --cc=bruce.richardson@intel.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@intel.com \
    --cc=hemant.agrawal@nxp.com \
    --cc=i.maximets@samsung.com \
    --cc=ian.stokes@intel.com \
    --cc=jerinj@marvell.com \
    --cc=jerinjacobk@gmail.com \
    --cc=konstantin.ananyev@intel.com \
    --cc=marcinx.a.zapolski@intel.com \
    --cc=maxime.coquelin@redhat.com \
    --cc=mdr@ashroe.eu \
    --cc=stephen@networkplumber.org \
    --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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).