From: "Wiles, Keith" <keith.wiles@intel.com>
To: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
Cc: Thomas Monjalon <thomas@monjalon.net>,
Ilya Maximets <i.maximets@samsung.com>,
"dev@dpdk.org" <dev@dpdk.org>,
"Yigit, Ferruh" <ferruh.yigit@intel.com>,
"ovs-dev@openvswitch.org" <ovs-dev@openvswitch.org>,
"Stokes, Ian" <ian.stokes@intel.com>,
"Kevin Traynor" <ktraynor@redhat.com>,
Ophir Munk <ophirmu@mellanox.com>,
"Shahaf Shuler" <shahafs@mellanox.com>,
Eelco Chaudron <echaudro@redhat.com>,
"arybchenko@solarflare.com" <arybchenko@solarflare.com>
Subject: Re: [dpdk-dev] Direct using of 'rte_eth_devices' in DPDK apps.
Date: Fri, 16 Nov 2018 14:19:40 +0000 [thread overview]
Message-ID: <037382C8-F699-4327-B7D0-B385AA8A0EFE@intel.com> (raw)
In-Reply-To: <2601191342CEEE43887BDE71AB977258010CEB8C73@IRSMSX106.ger.corp.intel.com>
> On Nov 16, 2018, at 3:51 AM, Ananyev, Konstantin <konstantin.ananyev@intel.com> wrote:
>
> Hi everyone,
>
>>
>> Hi,
>>
>> 16/11/2018 09:42, Ilya Maximets:
>>> Hi,
>>> While discussing the ways to enable DPDK 18.11 new features in OVS
>>> there was suggestions to use 'rte_eth_devices[]' array directly.
>>> But this array is marked as '@internal' and also it located in
>>> the internal header 'lib/librte_ethdev/rte_ethdev_core.h' with the
>>> following disclaimer:
>>>
>>> /**
>>> * @file
>>> *
>>> * RTE Ethernet Device internal header.
>>> *
>>> * This header contains internal data types. But they are still part of the
>>> * public API because they are used by inline functions in the published API.
>>> *
>>> * Applications should not use these directly.
>>> *
>>> */
>>>
>>> From the other hand, test-pmd and some example apps in DPDK source
>>> tree are using this array for various reasons.
>>>
>>> So, is it OK to use this array directly or not?
>>
>> Good question :)
>> Thanks for bringing this discussion.
>>
>> As you said, it is public because of inline functions using it directly
>> for performance purpose. The DPDK API is bad for separating public and
>> internal stuff. And over time, there is not a lot of attention on trying
>> to not use internal symbols in applications.
>>
>>> In general we need to change the API, i.e. make 'rte_eth_devices' part
>>> of a public API. Or change the test-pmd and example apps to stop
>>> using it.
>>
>> I agree we need to decide an option and make it clear.
>>
>> We can try to make this variable private and add more public functions
>> to use it (I'm thinking at more iterators like sibling ones).
>> It would clarify the API.
>> It can be evaluated what is the real cost after compiler optimization
>> for Rx/Tx functions. It can also be evaluated to uninline functions.
>>
>> On the other hand, we can wonder what is the real benefit of trying to
>> hide access to internal resources. Should we make all public?
>
> In that case every change in any of such structures will be an ABI breakage.
> Even now any change in rte_eth_dev is quite problematic because of that.
> I think we better keep them private as much as possible and cleanup
> our examples and testpmd code.
> Konstantin
I Agree here, I have noticed a few places we allow direct access to internal data structures, which we need to restrict access by making them private with getter/setter functions or just getter/setter functions even if we can not make them private. At least with moving members and adding members we can state that it is not a ABI breakage as long as everyone uses the getter/setter functions. These functions could not be inline functions correct as that would still break API?
>
>>
>>> One more related question: Is it OK to access internal device
>>> stuff using 'device' pointer obtained by 'rte_eth_dev_info'?
>>> This looks really dangerous. It's unclear why pointers like this
>>> exposed to user.
>>
>> It's a lot easier to expose pointers than doing a good API for all uses.
>> We need to question what is really dangerous and what we want to avoid?
Regards,
Keith
next prev parent reply other threads:[~2018-11-16 14:19 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <CGME20181116084233eucas1p2ae806fd36b2fa1ea77d1a450facb0922@eucas1p2.samsung.com>
2018-11-16 8:42 ` Ilya Maximets
2018-11-16 9:29 ` Thomas Monjalon
2018-11-16 9:51 ` Ananyev, Konstantin
2018-11-16 14:16 ` Wiles, Keith
2018-11-16 14:19 ` Wiles, Keith [this message]
2018-11-16 9:30 ` Ferruh Yigit
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=037382C8-F699-4327-B7D0-B385AA8A0EFE@intel.com \
--to=keith.wiles@intel.com \
--cc=arybchenko@solarflare.com \
--cc=dev@dpdk.org \
--cc=echaudro@redhat.com \
--cc=ferruh.yigit@intel.com \
--cc=i.maximets@samsung.com \
--cc=ian.stokes@intel.com \
--cc=konstantin.ananyev@intel.com \
--cc=ktraynor@redhat.com \
--cc=ophirmu@mellanox.com \
--cc=ovs-dev@openvswitch.org \
--cc=shahafs@mellanox.com \
--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).