[dpdk-dev] RFC: hiding struct rte_eth_dev

[dpdk-dev] RFC: hiding struct rte_eth_dev

[Ray Kinsella]

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.

Thanks,

Ray K

Re: [dpdk-dev] RFC: hiding struct rte_eth_dev

[Bruce Richardson]

On Mon, Sep 23, 2019 at 05:19:27PM +0100, Ray Kinsella wrote:
> 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.
> 
Thanks for calling out these potential options, Ray.

#4, uninlining, would also be my preference, though I think #1, do nothing,
is probably ok and could live with #2, adding padding, if others like the
idea. While #3, splitting structures, has advantages, I just dislike how
invasive it is, and don't think it's a good candidate for 19.11.

/Bruce

Re: [dpdk-dev] RFC: hiding struct rte_eth_dev

[Morten Brørup]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ray Kinsella
> Sent: Monday, September 23, 2019 6:19 PM
> To: dpdk-dev
> Cc: Richardson, Bruce; Jerin Jacob Kollanukkaran; Hemant Agrawal;
> Thomas Monjalon; Stephen Hemminger; Yigit, Ferruh; Ananyev, Konstantin;
> maxime.coquelin@redhat.com; David Marchand; Marcin Zapolski
> Subject: [dpdk-dev] RFC: hiding struct rte_eth_dev
> 
> 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.
> 
> Thanks,
> 
> Ray K

First of all, consider why an application would need to access the rte_eth_dev structure directly. I don't see why. So I assume that the only issue with hiding it behind an opaque handle is its use in the [rx,tx]_burst functions.

Next, let's consider the impact of uninlining the [rx,tx]_burst functions. An actual application would have to be extremely simple for the impact to be anywhere near the 2 % seen with testpmd.

Thus I vote for #4.
#3 is even better than #4, assuming performance *of core functionality* trumps general readability and maintainability. And it probably doesn't affect readability and maintainability for DPDK users/consumers. So I also vote for #3.


I think the decision about uninlining core functions should be a decision about the general principle, which should be discussed thoroughly, perhaps using this as a reference example.


Furthermore, I would like to repeat my old request for a more realistic reference performance benchmark application than testpmd if performance benchmark results are used as arguments for or against suggestions like this.

With testpmd, everything flies directly through the cache. I would be so bold as to claim that if you shuffle the mbuf fields between the first and second cache line, you probably wouldn't see any difference with testpmd; the packet goes directly to the tx step after the rx step, and the cache miss that would normally be seen in the tx step is instead taken in the rx step, so there is no performance difference.

A reference performance benchmark application should have at least two or three separate steps, each processing a sufficiently large amount of packets, so the cache is only efficient within each step. It could be an ingress step for receiving and enqueueing the packets, a delay step holding enough packets for their information to disappear from the cache, and an egress step for dequeueing and transmitting the packets.



Med venlig hilsen / kind regards
- Morten Brørup

Re: [dpdk-dev] RFC: hiding struct rte_eth_dev

[Jerin Jacob]

On Mon, Sep 23, 2019 at 9:49 PM Ray Kinsella <mdr@ashroe.eu> wrote:
>
> 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.


>
> Thanks,
>
> Ray K

Re: [dpdk-dev] RFC: hiding struct rte_eth_dev

[Ananyev, Konstantin]

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

Re: [dpdk-dev] RFC: hiding struct rte_eth_dev

[Andrew Rybchenko]

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.

Andrew

Re: [dpdk-dev] RFC: hiding struct rte_eth_dev

[David Marchand]

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

Re: [dpdk-dev] RFC: hiding struct rte_eth_dev

[David Marchand]

Fixed Ilya address.

On Thu, Sep 26, 2019 at 1:50 PM David Marchand
<david.marchand@redhat.com> wrote:
>
> 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