From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vlad Zolotarov Subject: Re: [PATCH 1/7] ethdev: remove unused flag from header Date: Mon, 4 Dec 2017 14:37:46 -0500 Message-ID: <6cec5132-7c8e-42a1-4c69-5c4c4bf1ef2a@scylladb.com> References: <20171201022957.64329-1-ferruh.yigit@intel.com> <49eb13f1-e12f-071a-b58b-1cd4852b7c8b@scylladb.com> <2588d596-19be-483a-507f-565b01ff5615@intel.com> <3ccbeca0-79d5-d94d-ad77-e5a11e387049@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Cc: dev@dpdk.org, vladz@cloudius-systems.com To: Ferruh Yigit , Thomas Monjalon Return-path: Received: from mail-vk0-f65.google.com (mail-vk0-f65.google.com [209.85.213.65]) by dpdk.org (Postfix) with ESMTP id E78A91C00 for ; Mon, 4 Dec 2017 20:37:49 +0100 (CET) Received: by mail-vk0-f65.google.com with SMTP id l187so10366517vke.5 for ; Mon, 04 Dec 2017 11:37:49 -0800 (PST) In-Reply-To: <3ccbeca0-79d5-d94d-ad77-e5a11e387049@intel.com> Content-Language: en-US List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On 12/04/2017 12:54 PM, Ferruh Yigit wrote: > On 12/1/2017 6:22 PM, Vlad Zolotarov wrote: >> >> On 12/01/2017 06:51 PM, Ferruh Yigit wrote: >>> On 12/1/2017 2:17 PM, Vlad Zolotarov wrote: >>>> On 11/30/2017 09:29 PM, Ferruh Yigit wrote: >>>>> remove RTE_ETHDEV_HAS_LRO_SUPPORT flag from header. >>>>> >>>>> Flag seems added with the patch that adds LRO support, and intention >>>>> looks like giving a pointer to application that library supports LRO. >>>> Exactly. Removing this flag may make the existing application "think" that LRO >>>> is not supported. >>>> Why do you want to remove it to begin with? >>> I agree that this is a little controversial, and I don't have a strong opinion >>> about it, I thought twice before sending or not sending the patch :) >>> >>> This flag can be useful for the applications that use different versions of the >>> DPDK library (with the ones does and doesn't support LRO), so that application >>> can be more portable. >>> I would understand a dynamic approach, which would be useful for distributed >>> applications that you don't know and can't control what version of the DPDK >>> library used. >>> But here to benefit from this flag you need to compile your application against >>> DPDK library, and if you are compiling it you already know if your DPDK supports >>> LRO or not. And this is not something keeps changing platform to platform etc, >>> after a specific release LRO is always supported. >> True but DPDK is usually not a part of your source tree - it (should) come as a >> library and currently there isn't any proper way to query feature set. >> This flag was also added after a lot of consideration as a rather ugly >> compromise since there wasn't (and there isn't) a proper way to do that at that >> (this) time. See more on that below. >> >>> And this is the only capability support flag in the ethdev, there are many new >>> features introduced in each release but they are not advertised by a capability >>> flag. Only having LRO flag can be confusing. >>> >>> _Perhaps_ DPDK should have a way to expose the supported features, and a dynamic >>> runtime way can be better option than compile time macros, but it should be >>> generic nothing specific to LRO support. >> IMO it's not "perhaps" it's a "must" _however_ > This is even wider than ethdev, more like in DPDK scope, we need someone to > attack this issue. absolutely! > >> you can't remove this flag without giving some other tool to do the same. > Fair enough, I will drop the patch. > >> Querying a DPDK version would be a wrong direction because some Linux >> distributions (yes, I'm talking about you, Ubuntu) tend to have it's own trees >> with their own backports and these trees may be light years ahead in their patch >> level compared to vanilla trees with the same version. > +1 to have better way than version check. > >>>>> Fixes: 8eecb3295aed ("ixgbe: add LRO support") >>>>> Cc: vladz@cloudius-systems.com >>>>> >>>>> Signed-off-by: Ferruh Yigit >>>>> --- >>>>> lib/librte_ether/rte_ethdev.h | 3 --- >>>>> 1 file changed, 3 deletions(-) >>>>> >>>>> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h >>>>> index 341c2d624..e620c3706 100644 >>>>> --- a/lib/librte_ether/rte_ethdev.h >>>>> +++ b/lib/librte_ether/rte_ethdev.h >>>>> @@ -172,9 +172,6 @@ extern "C" { >>>>> >>>>> #include >>>>> >>>>> -/* Use this macro to check if LRO API is supported */ >>>>> -#define RTE_ETHDEV_HAS_LRO_SUPPORT >>>>> - >>>>> #include >>>>> #include >>>>> #include