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

[Vlad Zolotarov <vladz@cloudius-systems.com>]

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?
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.
>>
>>> 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.