DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] Direct using of 'rte_eth_devices' in DPDK apps.
       [not found] <CGME20181116084233eucas1p2ae806fd36b2fa1ea77d1a450facb0922@eucas1p2.samsung.com>
@ 2018-11-16  8:42 ` Ilya Maximets
  2018-11-16  9:29   ` Thomas Monjalon
  2018-11-16  9:30   ` Ferruh Yigit
  0 siblings, 2 replies; 6+ messages in thread
From: Ilya Maximets @ 2018-11-16  8:42 UTC (permalink / raw)
  To: dev, Thomas Monjalon, Ferruh Yigit
  Cc: ovs-dev, Konstantin Ananyev, Stokes, Ian, Kevin Traynor,
	Ophir Munk, Shahaf Shuler, Eelco Chaudron

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?

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.

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.

Thoughts?

Best regards, Ilya Maximets.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-dev] Direct using of 'rte_eth_devices' in DPDK apps.
  2018-11-16  8:42 ` [dpdk-dev] Direct using of 'rte_eth_devices' in DPDK apps Ilya Maximets
@ 2018-11-16  9:29   ` Thomas Monjalon
  2018-11-16  9:51     ` Ananyev, Konstantin
  2018-11-16  9:30   ` Ferruh Yigit
  1 sibling, 1 reply; 6+ messages in thread
From: Thomas Monjalon @ 2018-11-16  9:29 UTC (permalink / raw)
  To: Ilya Maximets
  Cc: dev, Ferruh Yigit, ovs-dev, Konstantin Ananyev, Stokes, Ian,
	Kevin Traynor, Ophir Munk, Shahaf Shuler, Eelco Chaudron,
	arybchenko

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?

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-dev] Direct using of 'rte_eth_devices' in DPDK apps.
  2018-11-16  8:42 ` [dpdk-dev] Direct using of 'rte_eth_devices' in DPDK apps Ilya Maximets
  2018-11-16  9:29   ` Thomas Monjalon
@ 2018-11-16  9:30   ` Ferruh Yigit
  1 sibling, 0 replies; 6+ messages in thread
From: Ferruh Yigit @ 2018-11-16  9:30 UTC (permalink / raw)
  To: Ilya Maximets, dev, Thomas Monjalon
  Cc: ovs-dev, Konstantin Ananyev, Stokes, Ian, Kevin Traynor,
	Ophir Munk, Shahaf Shuler, Eelco Chaudron

On 11/16/2018 8:42 AM, Ilya Maximets wrote:
> 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?
> 
> 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.
> 
> 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.
> 
> Thoughts?

Agreed that applications shouldn't access 'rte_eth_devices' directly, they
should use handler (port_id) to access devices. Only drivers are allowed to
access these data structure directly.

Because of inline functions we are not able to completely hide these from
applications, and preferring inline functions for performance. Moving these data
structures to their own header and marking as internal was to clarify the
intended usage for them.

I support to clean testpmd and sample applications to prevent using internal
data structures.

> 
> Best regards, Ilya Maximets.
> 

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-dev] Direct using of 'rte_eth_devices' in DPDK apps.
  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
  0 siblings, 2 replies; 6+ messages in thread
From: Ananyev, Konstantin @ 2018-11-16  9:51 UTC (permalink / raw)
  To: Thomas Monjalon, Ilya Maximets
  Cc: dev, Yigit, Ferruh, ovs-dev, Stokes, Ian, Kevin Traynor,
	Ophir Munk, Shahaf Shuler, Eelco Chaudron, arybchenko

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

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-dev] Direct using of 'rte_eth_devices' in DPDK apps.
  2018-11-16  9:51     ` Ananyev, Konstantin
@ 2018-11-16 14:16       ` Wiles, Keith
  2018-11-16 14:19       ` Wiles, Keith
  1 sibling, 0 replies; 6+ messages in thread
From: Wiles, Keith @ 2018-11-16 14:16 UTC (permalink / raw)
  To: Ananyev, Konstantin
  Cc: Thomas Monjalon, Ilya Maximets, dev, Yigit, Ferruh, ovs-dev,
	Stokes, Ian, Kevin Traynor, Ophir Munk, Shahaf Shuler,
	Eelco Chaudron, arybchenko



On Nov 16, 2018, at 3:51 AM, Ananyev, Konstantin <konstantin.ananyev@intel.com<mailto: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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [dpdk-dev] Direct using of 'rte_eth_devices' in DPDK apps.
  2018-11-16  9:51     ` Ananyev, Konstantin
  2018-11-16 14:16       ` Wiles, Keith
@ 2018-11-16 14:19       ` Wiles, Keith
  1 sibling, 0 replies; 6+ messages in thread
From: Wiles, Keith @ 2018-11-16 14:19 UTC (permalink / raw)
  To: Ananyev, Konstantin
  Cc: Thomas Monjalon, Ilya Maximets, dev, Yigit, Ferruh, ovs-dev,
	Stokes, Ian, Kevin Traynor, Ophir Munk, Shahaf Shuler,
	Eelco Chaudron, arybchenko



> 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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2018-11-16 14:19 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20181116084233eucas1p2ae806fd36b2fa1ea77d1a450facb0922@eucas1p2.samsung.com>
2018-11-16  8:42 ` [dpdk-dev] Direct using of 'rte_eth_devices' in DPDK apps 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
2018-11-16  9:30   ` Ferruh Yigit

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git