[dpdk-dev] [PATCH v2 5/6] common_linuxapp: Added CONFIG_RTE_ETHDEV_LRO_SUPPORT option
Enables LRO support in PMDs. Signed-off-by: Vlad Zolotarov --- 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 # # Support NIC bypass logic -- 2.1.0
[dpdk-dev] [PATCH v2 5/6] common_linuxapp: Added CONFIG_RTE_ETHDEV_LRO_SUPPORT option
2015-03-05 13:28, Vlad Zolotarov: > Enables LRO support in PMDs. > > Signed-off-by: Vlad Zolotarov > --- > 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.
[dpdk-dev] [PATCH v2 5/6] common_linuxapp: Added CONFIG_RTE_ETHDEV_LRO_SUPPORT option
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 >> --- >> 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. >
[dpdk-dev] [PATCH v2 5/6] common_linuxapp: Added CONFIG_RTE_ETHDEV_LRO_SUPPORT option
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 > >> --- > >> 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.
[dpdk-dev] [PATCH v2 5/6] common_linuxapp: Added CONFIG_RTE_ETHDEV_LRO_SUPPORT option
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 --- 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.
[dpdk-dev] [PATCH v2 5/6] common_linuxapp: Added CONFIG_RTE_ETHDEV_LRO_SUPPORT option
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 > --- > 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.
[dpdk-dev] [PATCH v2 5/6] common_linuxapp: Added CONFIG_RTE_ETHDEV_LRO_SUPPORT option
On Mar 5, 2015 9:14 PM, "Thomas Monjalon" 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 > > --- > > 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. > >