From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga04.intel.com (mga04.intel.com [192.55.52.120]) by dpdk.org (Postfix) with ESMTP id ADA1E200 for ; Mon, 4 Dec 2017 18:54:30 +0100 (CET) Received: from fmsmga003.fm.intel.com ([10.253.24.29]) by fmsmga104.fm.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 04 Dec 2017 09:54:29 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.45,359,1508828400"; d="scan'208";a="8945335" Received: from fyigit-mobl1.ger.corp.intel.com (HELO [10.241.225.178]) ([10.241.225.178]) by FMSMGA003.fm.intel.com with ESMTP; 04 Dec 2017 09:54:29 -0800 To: Vlad Zolotarov , Thomas Monjalon Cc: dev@dpdk.org, vladz@cloudius-systems.com References: <20171201022957.64329-1-ferruh.yigit@intel.com> <49eb13f1-e12f-071a-b58b-1cd4852b7c8b@scylladb.com> <2588d596-19be-483a-507f-565b01ff5615@intel.com> From: Ferruh Yigit Message-ID: <3ccbeca0-79d5-d94d-ad77-e5a11e387049@intel.com> Date: Mon, 4 Dec 2017 09:54:29 -0800 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH 1/7] ethdev: remove unused flag from header X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 04 Dec 2017 17:54:31 -0000 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. > 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 >