Re: [dpdk-dev] [RFC 19.11 v2 1/3] ethdev: hide key ethdev structures from public API

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 9/9/2019 11:02 AM, Zapolski, MarcinX A wrote:
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Monday, September 9, 2019 12:00 PM
>> To: Zapolski, MarcinX A <marcinx.a.zapolski@intel.com>; dev@dpdk.org
>> Subject: Re: [dpdk-dev] [RFC 19.11 v2 1/3] ethdev: hide key ethdev
>> structures from public API
>>
>> On 9/9/2019 9:07 AM, Zapolski, MarcinX A wrote:
>>>> -----Original Message-----
>>>> From: Yigit, Ferruh
>>>> Sent: Friday, September 6, 2019 4:38 PM
>>>> To: Zapolski, MarcinX A <marcinx.a.zapolski@intel.com>; dev@dpdk.org
>>>> Subject: Re: [dpdk-dev] [RFC 19.11 v2 1/3] ethdev: hide key ethdev
>>>> structures from public API
>>>>
>>>> On 9/6/2019 2:18 PM, Marcin Zapolski wrote:
>>>>> Split rte_eth_dev structure to two parts: head that is available for
>>>>> user applications, and rest which is DPDK internal.
>>>>> Make an array of pointers to rte_eth_dev structures available for
>>>>> user applications.
>>>>>
>>>>> Signed-off-by: Marcin Zapolski <marcinx.a.zapolski@intel.com>
>>>>
>>>> <...>
>>>>
>>>>> diff --git a/lib/librte_bitratestats/rte_bitrate.c
>>>>> b/lib/librte_bitratestats/rte_bitrate.c
>>>>> index 639e47547..82d469514 100644
>>>>> --- a/lib/librte_bitratestats/rte_bitrate.c
>>>>> +++ b/lib/librte_bitratestats/rte_bitrate.c
>>>>> @@ -3,7 +3,7 @@
>>>>>   */
>>>>>
>>>>>  #include <rte_common.h>
>>>>> -#include <rte_ethdev.h>
>>>>> +#include <rte_ethdev_driver.h>
>>>>
>>>> This is in the library, not sure if libraries should include the
>>>> header file for the drivers, can you please explain why this change is
>> needed?
>>>>
>>> It is needed to make rte_eth_dev structure available. But yes, I agree that
>> it will be more appropriate to include rte_ethdev.h and rte_ethdev_core.h
>> separately. I probably wanted less includes, silly me.
>>>> <...>
>>>>
>>>>> @@ -6,6 +6,7 @@
>>>>>  #define _RTE_ETHDEV_PROFILE_H_
>>>>>
>>>>>  #include "rte_ethdev.h"
>>>>> +#include "rte_ethdev_core.h"
>>>>>
>>>>>  /**
>>>>>   * Initialization of the Ethernet device profiling.
>>>>> diff --git a/lib/librte_ethdev/rte_ethdev.c
>>>>> b/lib/librte_ethdev/rte_ethdev.c index 17d183e1f..5c6cc640a 100644
>>>>> --- a/lib/librte_ethdev/rte_ethdev.c
>>>>> +++ b/lib/librte_ethdev/rte_ethdev.c
>>>>> @@ -40,6 +40,7 @@
>>>>>
>>>>>  #include "rte_ether.h"
>>>>>  #include "rte_ethdev.h"
>>>>> +#include "rte_ethdev_core.h"
>>>>>  #include "rte_ethdev_driver.h"
>>>>>  #include "ethdev_profile.h"
>>>>>  #include "ethdev_private.h"
>>>>
>>>> I was hoping "rte_ethdev_core.h" can be removed completely by
>>>> distributing its content to "ethdev_private.h", "rte_ethdev_driver.h"
>>>> and perhaps even to "rte_ethdev.h".
>>>>
>>>> Can you please explain what prevents removing "rte_ethdev_core.h"?
>>> I could rename it to rte_ethdev_private. There is just rte_eth_dev and
>> rte_eth_dev_data left in it.
>>>
>>
>> I think drivers access to both 'rte_eth_dev' and 'rte_eth_dev_data' so can't
>> move them to 'ethdev_private.h' why not move it to 'rte_ethdev_driver.h'?
> 
> Because the libraries use them as well.
> 

These are 'librte_ethdev' library internal data, I think other libraries
shouldn't access them directly.

As far as I can see,
librte_eventdev  => rte_eth_dev_data
librte_eventdev  => rte_eth_dev
librte_telemetry => rte_eth_dev

Can you see any other library accessing 'rte_eth_dev' and 'rte_eth_dev_data' ?


I am not sure about 'eventdev', specially because of the Rx/Tx adapters of it,
perhaps they can include the "rte_ethdev_driver.h", cc'ed Jerin.

I didn't check the telemetry code, but I assume it can be fixed too, but for now
"rte_ethdev_driver.h" can be included to highlight something is wrong there.

And related "ethdev_private.h", as far as I understand it is not private to all
libraries, it is private to the ethdev library, which means only should be used
to share the definitions withing ethdev library .c files. So no other library
should include it.