Re: [dpdk-dev] [PATCH v2 07/12] pdump: disabled by default

["Gaëtan Rivet" <gaetan.rivet@6wind.com>]

Hi Ferruh,

On Tue, Jun 13, 2017 at 06:15:45PM +0100, Ferruh Yigit wrote:
> On 6/11/2017 8:42 PM, Gaëtan Rivet wrote:
> > Hi Reshma,
> > 
> > On Fri, Jun 09, 2017 at 02:24:58PM +0000, Pattan, Reshma wrote:
> >> Hi,
> >>
> >>> -----Original Message-----
> >>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Gaetan Rivet
> >>> Sent: Thursday, June 8, 2017 12:59 AM
> >>> To: dev@dpdk.org
> >>> Cc: Gaetan Rivet <gaetan.rivet@6wind.com>
> >>> Subject: [dpdk-dev] [PATCH v2 07/12] pdump: disabled by default
> >>>
> >>> Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
> >>> ---
> >>>  config/common_base | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/config/common_base b/config/common_base index
> >>> cade611..8ec5e4e 100644
> >>> --- a/config/common_base
> >>> +++ b/config/common_base
> >>> @@ -700,7 +700,7 @@ CONFIG_RTE_KNI_PREEMPT_DEFAULT=y  #  # Compile
> >>> the pdump library  # -CONFIG_RTE_LIBRTE_PDUMP=y
> >>> +CONFIG_RTE_LIBRTE_PDUMP=n
> >>>
> >>>  #
> >>>  # Compile vhost user library
> >>> --
> >>> 2.1.4
> >>
> >> Since, you already mentioned in other mail to Ferruh that config flag disabling patches are only for testers compilation purpose and you have plans to make proper fix by end of June.  I will wait on for actual patch. 
> >>
> > 
> > I said I planned to do so, but found out that I would not have enough
> > time before the end of June. Sorry about the ambiguous phrasing.
> > 
> > Do you think you will be able to fix this library in time?
> 
> KNI uses / depends pci, I am not sure what to fix here.
> 
> The problem to enable the KNI is build dependency problem, right?
> 
> I guess problem will be fixes if we can build in following order:
> - lib/eal
> - drivers/bus
> - lib
> - drivers
> 
> This was the case when bus drives compiled within eal. What do you think
> about this build order?
> 

Yes, that build order would fix the issue.
However, IMO this is not the proper way to proceed.
It obscures the architecture, the distinction between DPDK abstractions
and their implementations.

Looking quickly into this dependency, it seems that the PCI info is only
used during allocation, and only to register PCI information within
device infos. They do not seem used afterward at the library level,
except to print some device description upon device start.

They can be completely removed from KNI (both from the lib and the
driver), without breaking the compilation.
This however changes the API of rte_kni_alloc() and the ABI of
rte_kni_conf.

But it seems better than changing the build order and opening a can of
all kind of worms, allowing a few libraries to skirt around their duty to
remain generic and independent from abstraction implementations.

Ideally KNI interfaces should be able to use any rte_device, not only
PCI. But if it is forced to use only PCI devices, then pointing to an
rte_pci_device seems a better way to proceed, as it has all those infos
readily available. It would allow the PCI device to grow and evolve without
breaking the KNI lib.

Anyway, I think there are several possible solutions to this, before
resorting to modifying the build order.

> > 
> >> Please see if rte_pci.h can be removed from the includes of rte_pdump.c , might be unnecessary include.
> >>
> >> Thanks,
> >> Reshma
> > 
> 

-- 
Gaëtan Rivet
6WIND