Re: [dpdk-dev] [PATCH v2 5/6] common_linuxapp: Added CONFIG_RTE_ETHDEV_LRO_SUPPORT option

[Thomas Monjalon <thomas.monjalon@6wind.com>]

2015-03-05 15:39, Vlad Zolotarov:
> 
> On 03/05/15 15:19, Thomas Monjalon wrote:
> > 2015-03-05 13:28, Vlad Zolotarov:
> >> Enables LRO support in PMDs.
> >>
> >> Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
> >> ---
> >>   config/common_linuxapp | 1 +
> >>   1 file changed, 1 insertion(+)
> >>
> >> diff --git a/config/common_linuxapp b/config/common_linuxapp
> >> index 97f1c9e..5b98595 100644
> >> --- a/config/common_linuxapp
> >> +++ b/config/common_linuxapp
> >> @@ -137,6 +137,7 @@ CONFIG_RTE_MAX_ETHPORTS=32
> >>   CONFIG_RTE_LIBRTE_IEEE1588=n
> >>   CONFIG_RTE_ETHDEV_QUEUE_STAT_CNTRS=16
> >>   CONFIG_RTE_ETHDEV_RXTX_CALLBACKS=y
> >> +CONFIG_RTE_ETHDEV_LRO_SUPPORT=y
> > 
> > Sorry I don't really follow this ixgbe discussion but I wonder why you
> > would add a compile time option for this feature.
> 
> The only reason is to be able to detect that the feature is present in 
> the DPDK version u r compiling against because of the API change.
> Currently, this can't be done using the DPDK version thus we may either 

Why you cannot use version? In development phase?
When released, you'll be able to test >= 2.1.

> do a try-compilation and if it fails define some application-level macro 
> disabling
> the feature usage or we may define a macro in the library level 
> (together with tons of other such macros like those in the patch snippet 
> above).

I'd prefer a request rte_eth_dev_info_get() to advertise that the feature
is available with the device and driver.
Please let's try to remove config options and #ifdefs.

> > What is the benefit of disabling it?
> 
> No benefit whatsoever.
> 
> > And if really needed, this patch would make more sense merged with the
> > code under ifdef.
> 
> I strongly disagree - the amount of #ifdefs in the DPDK source is 
> absolutely enormous. It makes reading and  understanding the code really 
> hard.

I agree. You misunderstand me.
I was just saying that this patch should be merged.

> Therefore, I tried to reduce the amount of time the already existing 
> macros have to be queried (see PATCH4). And of course I don't see any 
> sense of adding new ones more than really needed. And in LRO case - it's 
> a single time, where the feature is manifested by the HW.