[dpdk-dev,v2,07/12] pdump: disabled by default
Checks
Commit Message
Signed-off-by: Gaetan Rivet <gaetan.rivet@6wind.com>
---
config/common_base | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Comments
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.
Please see if rte_pci.h can be removed from the includes of rte_pdump.c , might be unnecessary include.
Thanks,
Reshma
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?
> Please see if rte_pci.h can be removed from the includes of rte_pdump.c , might be unnecessary include.
>
> Thanks,
> Reshma
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?
>
>> Please see if rte_pci.h can be removed from the includes of rte_pdump.c , might be unnecessary include.
>>
>> Thanks,
>> Reshma
>
Hi,
> -----Original Message-----
> From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> Sent: Sunday, June 11, 2017 8:42 PM
> To: Pattan, Reshma <reshma.pattan@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 07/12] pdump: disabled by default
>
> >
> > 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?
rte_pci.h is unnecessary include in rte_pdump.c, hence I removed it and sent the patch.
Thanks,
Reshma
On Wed, Jun 14, 2017 at 09:09:18AM +0000, Pattan, Reshma wrote:
> Hi,
>
> > -----Original Message-----
> > From: Gaëtan Rivet [mailto:gaetan.rivet@6wind.com]
> > Sent: Sunday, June 11, 2017 8:42 PM
> > To: Pattan, Reshma <reshma.pattan@intel.com>
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v2 07/12] pdump: disabled by default
> >
> > >
> > > 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?
>
> rte_pci.h is unnecessary include in rte_pdump.c, hence I removed it and sent the patch.
>
I tested it, no problem.
The patch disabling pdump will be removed from the next version. Thanks.
> Thanks,
> Reshma
>
>
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
> >
>
On 6/15/2017 12:01 AM, Gaëtan Rivet wrote:
> 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.
I started the discussion in wrong thread, I will copy your mail and
answer from correct thread, hoping this won't make things more confusing.
For future reference, moving to:
http://dpdk.org/ml/archives/dev/2017-June/067688.html
>
>>>
>>>> Please see if rte_pci.h can be removed from the includes of rte_pdump.c , might be unnecessary include.
>>>>
>>>> Thanks,
>>>> Reshma
>>>
>>
>
@@ -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