| Message ID | 1425554885-16901-6-git-send-email-vladz@cloudius-systems.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers |
Return-Path: <dev-bounces@dpdk.org> X-Original-To: patchwork@dpdk.org Delivered-To: patchwork@dpdk.org Received: from [92.243.14.124] (localhost [IPv6:::1]) by dpdk.org (Postfix) with ESMTP id 21C886A95; Thu, 5 Mar 2015 12:28:18 +0100 (CET) Received: from mail-wg0-f43.google.com (mail-wg0-f43.google.com [74.125.82.43]) by dpdk.org (Postfix) with ESMTP id 8D75D6837 for <dev@dpdk.org>; Thu, 5 Mar 2015 12:28:15 +0100 (CET) Received: by wggy19 with SMTP id y19so52715505wgg.10 for <dev@dpdk.org>; Thu, 05 Mar 2015 03:28:15 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references; bh=Z4enEZc0TPs8WE5lmooh6td0BiZpjqGdx2er2rX90J4=; b=iqI81fXFvTu7NeYtHDeo2X3CgMeUKrs1ssfdsgdxn4w/shGMDvC3jSEWmQ1qaMHS+d 3eWCLDUpBvdoyyW9x1Z6TTqjBX3DNxJOrZlBe8fEjqkrnBCyacygYrtCnZH5d2RoaI0T UvA1U1Qz0tx2exLGAxLdG7tx2qhNbbB5Rr9hgaEOrnHxrMzOo8CqaDWwiTrPaB7D1LsM t6Af1QYlmXScWk72d8uCWJMXWfwfD/ypntd8Sxte10cfLfKtaIDDm9erWJMKQDA7dcP+ cVquG6PYZCcAlkrON6LFp86NYU4sPtlRZ4h6qN16H8e4F/TidZW0XEwg1eHUc1yDsnhw +jlA== X-Gm-Message-State: ALoCoQl+zSJrLH+o6USjxnuLCGxwDMs8gj1wc+jiAAgA4klmCOlJr9/7cHs1ivMYppH8G2tngj6l X-Received: by 10.194.47.201 with SMTP id f9mr17253539wjn.17.1425554895482; Thu, 05 Mar 2015 03:28:15 -0800 (PST) Received: from vladz-laptop.cloudius-systems.com. ([212.143.139.214]) by mx.google.com with ESMTPSA id e2sm10069621wjy.46.2015.03.05.03.28.14 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Thu, 05 Mar 2015 03:28:15 -0800 (PST) From: Vlad Zolotarov <vladz@cloudius-systems.com> To: dev@dpdk.org Date: Thu, 5 Mar 2015 13:28:04 +0200 Message-Id: <1425554885-16901-6-git-send-email-vladz@cloudius-systems.com> X-Mailer: git-send-email 2.1.0 In-Reply-To: <1425554885-16901-1-git-send-email-vladz@cloudius-systems.com> References: <1425554885-16901-1-git-send-email-vladz@cloudius-systems.com> Subject: [dpdk-dev] [PATCH v2 5/6] common_linuxapp: Added CONFIG_RTE_ETHDEV_LRO_SUPPORT option X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK <dev.dpdk.org> List-Unsubscribe: <http://dpdk.org/ml/options/dev>, <mailto:dev-request@dpdk.org?subject=unsubscribe> List-Archive: <http://dpdk.org/ml/archives/dev/> List-Post: <mailto:dev@dpdk.org> List-Help: <mailto:dev-request@dpdk.org?subject=help> List-Subscribe: <http://dpdk.org/ml/listinfo/dev>, <mailto:dev-request@dpdk.org?subject=subscribe> Errors-To: dev-bounces@dpdk.org Sender: "dev" <dev-bounces@dpdk.org> |
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
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.
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. >
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.
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.
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.
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 --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