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 16:18, Vlad Zolotarov:
> 
> On 03/05/15 16:01, Thomas Monjalon wrote:
> > 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.
> 
> Of course! When the version bumps, the #ifdef I've mentioned above may 
> be replaced with the version check.
> 
> >
> >> 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.
> 
> This is exactly what my patch does. But that's not ending there - there 
> is a new feature bit added in rte_eth_rxmode (enable_lro) and in order 
> to compile the application has to know somehow if this bit is present or 
> not. How do u propose to do this now?

I think it would be better to define something like RTE_HAS_LRO in rte_ethdev.h.

> Of course, I can put such macro in my own tree but then I'll have to 
> rebase all the time and inform other developers that will have to work 
> against my tree (and not upstream as it's supposed to be) - to update. 
> This sounds like a hassle thus I added this macro to resolve this issue 
> until the version is bumped.
> 
> >
> >>> What is the benefit of disabling it?
> >> No benefit whatsoever.