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

[Andrew Rybchenko <arybchenko@solarflare.com>]

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