On Mon, Sep 9, 2019 at 3:54 PM Ferruh Yigit <ferruh.yi...@intel.com> wrote: > > 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.zapol...@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.zapol...@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.zapol...@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.
Yes. Overall this patch theme looks good to me. I will wait for v3 to change PMDs I maintains. My only comment is, We should make rte_ethdev_driver.h as private i.e it should not be included by applications. Currently. On dpdk install, the private header file is copied to prefix ie make install T=x86_64-native-linux-gcc DESTDIR=install [master][dpdk.org] $ ls install/include/dpdk/rte_ethdev_driver.h install/include/dpdk/rte_ethdev_driver.h The internal libraries can access the header file through the following scheme without exposing it as public. CFLAGS += -I$(RTE_SDK)/lib/librte_ethdev/ in .c file #include "ethdev_driver.h"
