diff mbox

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

Message ID 1425554885-16901-6-git-send-email-vladz@cloudius-systems.com (mailing list archive)
State Superseded, archived
Headers show

Commit Message

Vladislav Zolotarov March 5, 2015, 11:28 a.m. UTC
Enables LRO support in PMDs.

Signed-off-by: Vlad Zolotarov <vladz@cloudius-systems.com>
---
 config/common_linuxapp | 1 +
 1 file changed, 1 insertion(+)

Comments

Thomas Monjalon March 5, 2015, 1:19 p.m. UTC | #1
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.
What is the benefit of disabling it?
And if really needed, this patch would make more sense merged with the
code under ifdef.
Vladislav Zolotarov March 5, 2015, 1:39 p.m. UTC | #2
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 
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).


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


>
Thomas Monjalon March 5, 2015, 2:01 p.m. UTC | #3
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.
Vladislav Zolotarov March 5, 2015, 2:18 p.m. UTC | #4
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.
Thomas Monjalon March 5, 2015, 7:13 p.m. UTC | #5
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.
Vladislav Zolotarov March 5, 2015, 8:41 p.m. UTC | #6
On Mar 5, 2015 9:14 PM, "Thomas Monjalon" <thomas.monjalon@6wind.com> wrote:
>
> 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.

Ok. So i'll change it in v4.

>
> > 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.
>
>
diff mbox

Patch

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
 
 #
 # Support NIC bypass logic