[dpdk-dev] [PATCH] doc: plan splitting the ethdev ops struct

[Andrew Rybchenko]

Hi Konstantin,

On 2/25/20 2:07 PM, Ananyev, Konstantin wrote:
> Hi Andew, 
>
>> On 2/21/20 1:40 PM, Ferruh Yigit wrote:
>>> On 2/18/2020 6:01 AM, Stephen Hemminger wrote:
>>>> On Mon, 17 Feb 2020 15:38:05 +0000
>>>> Ferruh Yigit <ferruh.yigit at intel.com> wrote:
>>>>
>>>>> For the ABI compatibility it is better to hide internal data structures
>>>>> from the application as much as possible. But because of some inline
>>>>> functions 'struct eth_dev_ops' can't be hidden completely.
>>>>>
>>>>> Plan is to split the 'struct eth_dev_ops' into two as ones used by
>>>>> inline functions and ones not used, and hide the second part that not
>>>>> used by inline functions completely to the application.
>>>>>
>>>>> Signed-off-by: Ferruh Yigit <ferruh.yigit at intel.com>
>>>>> ---
>>>>> Cc: David Marchand <david.marchand at redhat.com>
>>>>> Cc: Thomas Monjalon <thomas at monjalon.net>
>>>>> Cc: Andrew Rybchenko <arybchenko at solarflare.com>
>>>>> ---
>>>>>  doc/guides/rel_notes/deprecation.rst | 6 ++++++
>>>>>  1 file changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
>>>>> index dfcca87ab..2aa431028 100644
>>>>> --- a/doc/guides/rel_notes/deprecation.rst
>>>>> +++ b/doc/guides/rel_notes/deprecation.rst
>>>>> @@ -72,6 +72,12 @@ Deprecation Notices
>>>>>    In 19.11 PMDs will still update the field even when the offload is not
>>>>>    enabled.
>>>>>
>>>>> +* ethdev: Split the ``struct eth_dev_ops`` struct to hide it as much as possible.
>>>>> +  Currently the ``struct eth_dev_ops`` struct is accessible by the application
>>>>> +  because some inline functions, like ``rte_eth_tx_descriptor_status()``,
>>>>> +  access the struct directly. The struct will be separate in two, the ops used
>>>>> +  by inline functions still will be accessible to user but rest will be hidden.
>>>>> +
>>>>>  * cryptodev: support for using IV with all sizes is added, J0 still can
>>>>>    be used but only when IV length in following structs ``rte_crypto_auth_xform``,
>>>>>    ``rte_crypto_aead_xform`` is set to zero. When IV length is greater or equal
>>>> Good luck, truely hiding internals is hard. The mbuf structure is already split but not really
>>>> hidden at all (just look at dwarf output). It doesn't make sense to do it unless
>>>> you can really hide it.
>>> I believe this can be done, only following [1] dev_ops are used by inline
>>> functions, rest can be moved into separate struct and moved into ethdev driver
>>> looking header.
>>>
>>> [1]
>>> rx_queue_count
>>> rx_descriptor_done
>>> rx_descriptor_status
>>> tx_descriptor_status
>> I think having 3 places (if I understand the intention
>> correctly) with ethdev callbacks is too much. So, I think
>> that these ops should be simply moved to nearby Tx/Rx
>> burst and Tx prepare callbacks (e.g. move into inline_ops
>> structure which is located at the beginning of rte_eth_dev
>> in order to preserve 3 existing callback location).
> If you are going to change ABI anyway,  would it be worth to consider 
> moving rx/tx burst/prepare functions to be per queue,
> instead of per device? 

I'm thinking about it from time to time. In general I like the idea.
The only question I have if there is a demand for queues with
different offloads which could result in different Rx/Tx callbacks.
Also I'm not sure that it is doable without performance degradation
at all, but hopefully it will be tiny.

>> Also I'd consider to deprecate and remove rx_queue_count
>> and rx_descriptor_done.
>>
>>>> I would attack the rte_device stuff first. Make rte_device opaque to the application
>>>> that would help for future versions. Then work backwards to rte_tehtdev.
>>>>