DPDK patches and discussions
 help / color / mirror / Atom feed
From: Ferruh Yigit <ferruh.yigit@intel.com>
To: Andrew Rybchenko <arybchenko@solarflare.com>,
	Stephen Hemminger <stephen@networkplumber.org>
Cc: Neil Horman <nhorman@tuxdriver.com>,
	John McNamara <john.mcnamara@intel.com>,
	Marko Kovacevic <marko.kovacevic@intel.com>,
	dev@dpdk.org, David Marchand <david.marchand@redhat.com>,
	Thomas Monjalon <thomas@monjalon.net>,
	"Kinsella, Ray" <ray.kinsella@intel.com>,
	Olivier MATZ <olivier.matz@6wind.com>
Subject: Re: [dpdk-dev] [PATCH] doc: plan splitting the ethdev ops struct
Date: Tue, 25 Feb 2020 12:28:49 +0000	[thread overview]
Message-ID: <36992cfb-0b39-85f0-a02a-0c49ed9c9eec@intel.com> (raw)
In-Reply-To: <6d9cfcfa-cade-8281-37d5-b048b14d8d0f@solarflare.com>

On 2/25/2020 10:35 AM, Andrew Rybchenko wrote:
> 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@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@intel.com>
>>>> ---
>>>> Cc: David Marchand <david.marchand@redhat.com>
>>>> Cc: Thomas Monjalon <thomas@monjalon.net>
>>>> Cc: Andrew Rybchenko <arybchenko@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. 

Yes, this is the intention.

> 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).

I think this is reasonable, but this can be done only in 20.11 with ABI break.

What do you think doing the initial hide in 20.05 with three places and do
proper splitting in 20.11 as suggested above.

Or it is possible to drop the interim work and do the all changes in 20.11, not
sure.


> 
> Also I'd consider to deprecate and remove rx_queue_count
> and rx_descriptor_done.

+1 to deprecate the 'rx_descriptor_done', we have already 'rx_descriptor_status'
& 'tx_descriptor_status' to replace the functionality.

@Thomas, @Ray, can you please remind how deprecation was done, is it marking
API, 'rte_eth_rx_descriptor_done', as deprecated in 20.11 and removing it in
21.11? If so I guess deprecation notice should be send before 20.11? (And make
sure all PMDs did the switch before 20.11)


Not sure about 'rx_queue_count', if any application may need it or not.

> 
>>>
>>> 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.
>>>
> 


  parent reply	other threads:[~2020-02-25 12:28 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-02-17 15:38 Ferruh Yigit
2020-02-18  5:07 ` Jerin Jacob
2020-02-25 12:42   ` Ferruh Yigit
2020-05-26 13:01   ` Thomas Monjalon
2020-02-18  6:01 ` Stephen Hemminger
2020-02-21 10:40   ` Ferruh Yigit
2020-02-25 10:35     ` Andrew Rybchenko
2020-02-25 11:07       ` Ananyev, Konstantin
2020-02-25 11:19         ` Andrew Rybchenko
2020-02-25 12:28       ` Ferruh Yigit [this message]
2020-02-25 12:44 ` [dpdk-dev] [PATCH v2] " Ferruh Yigit
2020-02-25 15:51   ` Andrew Rybchenko
2020-02-25 16:13     ` Ferruh Yigit
2020-02-25 16:41       ` Andrew Rybchenko
2020-02-25 18:13   ` David Marchand
2020-02-25 18:18     ` Ferruh Yigit
2020-03-04  9:57   ` [dpdk-dev] [PATCH v3] " Ferruh Yigit
2020-05-24 23:18     ` Thomas Monjalon
2020-05-25  9:11       ` Andrew Rybchenko
2020-05-26 13:55         ` Thomas Monjalon
2020-05-25 10:24     ` 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=36992cfb-0b39-85f0-a02a-0c49ed9c9eec@intel.com \
    --to=ferruh.yigit@intel.com \
    --cc=arybchenko@solarflare.com \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=john.mcnamara@intel.com \
    --cc=marko.kovacevic@intel.com \
    --cc=nhorman@tuxdriver.com \
    --cc=olivier.matz@6wind.com \
    --cc=ray.kinsella@intel.com \
    --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).