From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com (mga07.intel.com [134.134.136.100]) by dpdk.org (Postfix) with ESMTP id C2CB35B32 for ; Thu, 15 Mar 2018 15:57:29 +0100 (CET) X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga006.fm.intel.com ([10.253.24.20]) by orsmga105.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 15 Mar 2018 07:57:27 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,311,1517904000"; d="scan'208";a="211609297" Received: from fyigit-mobl.ger.corp.intel.com (HELO [10.237.221.62]) ([10.237.221.62]) by fmsmga006.fm.intel.com with ESMTP; 15 Mar 2018 07:57:25 -0700 To: Bruce Richardson , "Horton, Remy" Cc: "Ananyev, Konstantin" , Shreyansh Jain , "dev@dpdk.org" , "Lu, Wenzhuo" , "Wu, Jingjing" , "Zhang, Qi Z" , "Xing, Beilei" , Thomas Monjalon References: <20180307120851.5822-2-remy.horton@intel.com> <023fbd6c-7cac-6c8b-9a40-7a62e5d47bb7@intel.com> <30b8575d-4aeb-912d-6f74-c49ad7ce879a@intel.com> <591e1a23-8d27-0c59-fd39-0bde9e48e96f@intel.com> <2601191342CEEE43887BDE71AB9772589E28FD57@irsmsx105.ger.corp.intel.com> <2b3a2579-6bce-55f5-6e03-0974729cc95b@intel.com> <20180314213658.GA108@bricha3-MOBL.ger.corp.intel.com> <20180315143924.GA9172@bricha3-MOBL.ger.corp.intel.com> From: Ferruh Yigit Message-ID: <97dc9f9d-041b-ef99-2ca6-1f557c4f6039@intel.com> Date: Thu, 15 Mar 2018 14:57:24 +0000 User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <20180315143924.GA9172@bricha3-MOBL.ger.corp.intel.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 8bit Subject: Re: [dpdk-dev] [RFC PATCH v1 1/4] ethdev: add support for PMD-tuned Tx/Rx parameters 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: Thu, 15 Mar 2018 14:57:30 -0000 On 3/15/2018 2:39 PM, Bruce Richardson wrote: > On Thu, Mar 15, 2018 at 01:57:13PM +0000, Ferruh Yigit wrote: >> On 3/14/2018 9:36 PM, Bruce Richardson wrote: >>> On Wed, Mar 14, 2018 at 09:02:47PM +0000, Ferruh Yigit wrote: >>>> On 3/14/2018 6:53 PM, Ananyev, Konstantin wrote: >>>>> >>>>> >>>>>> -----Original Message----- >>>>>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit >>>>>> Sent: Wednesday, March 14, 2018 5:52 PM >>>>>> To: Shreyansh Jain ; Horton, Remy ; dev@dpdk.org >>>>>> Cc: Lu, Wenzhuo ; Wu, Jingjing ; Zhang, Qi Z ; Xing, Beilei >>>>>> ; Thomas Monjalon >>>>>> Subject: Re: [dpdk-dev] [RFC PATCH v1 1/4] ethdev: add support for PMD-tuned Tx/Rx parameters >>>>>> >>>>>> On 3/14/2018 5:23 PM, Shreyansh Jain wrote: >>>>>>>> -----Original Message----- >>>>>>>> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com] >>>>>>>> Sent: Wednesday, March 14, 2018 10:13 PM >>>>>>>> To: Remy Horton ; dev@dpdk.org >>>>>>>> Cc: Wenzhuo Lu ; Jingjing Wu >>>>>>>> ; Qi Zhang ; Beilei Xing >>>>>>>> ; Shreyansh Jain ; >>>>>>>> Thomas Monjalon >>>>>>>> Subject: Re: [dpdk-dev] [RFC PATCH v1 1/4] ethdev: add support for PMD- >>>>>>>> tuned Tx/Rx parameters >>>>>>>> >>>>>>>> On 3/14/2018 3:48 PM, Remy Horton wrote: >>>>>>>>> >>>>>>>>> On 14/03/2018 14:43, Ferruh Yigit wrote: >>>>>>>>> [..] >>>>>>>>>>> lib/librte_ether/rte_ethdev.c | 18 ++++++++++++++++++ >>>>>>>>>>> lib/librte_ether/rte_ethdev.h | 15 +++++++++++++++ >>>>>>>>>> >>>>>>>>>> Can you please remove deprecation notice in this patch. >>>>>>>>> >>>>>>>>> Done. >>>>>>>>> >>>>>>>>>>> + /* Defaults for drivers that don't implement preferred >>>>>>>>>>> + * queue parameters. >>>>>>>>> [..] >>>>>>>>>> Not sure about having these defaults here. It is OK to have defaults >>>>>>>> in driver, >>>>>>>>>> in application or in config file, but I am not sure if putting them >>>>>>>> into device >>>>>>>>>> abstraction layer hides them. >>>>>>>>>> >>>>>>>>>> What about not providing any default in ethdev layer, and get zero >>>>>>>> as invalid >>>>>>>>>> when using them? >>>>>>>>> >>>>>>>>> This is something I have been thinking about, and I am going to >>>>>>>> remove >>>>>>>>> them for the V2. Original motive was to avoid breaking testpmd for >>>>>>>> PMDs >>>>>>>>> that don't give defaults (i.e. almost all of them). The alternative >>>>>>>> is >>>>>>>>> to put place-holders into all the PMDs themselves, but I am not sure >>>>>>>> if >>>>>>>>> this is appropriate. >>>>>>>> >>>>>>>> I think preferred values should be optional, PMD should have right to >>>>>>>> not >>>>>>>> provide any. Implementation in 4/4 forces preferred values, either in >>>>>>>> all PMDs >>>>>>>> or in ethdev layer. >>>>>>>> >>>>>>>> What about changing approach in application: >>>>>>>> is preferred value provided [1] ? >>>>>>>> yes => use it by sending value 0 >>>>>>>> no => use application provided value, same as now, so control should >>>>>>>> be in >>>>>>>> application. >>>>>>>> >>>>>>>> I am aware this breaks the comfort of just providing 0 and PMD values >>>>>>>> will be >>>>>>>> used but covers the case there is no PMD values. >>>>>>>> >>>>>>>> [1] >>>>>>>> it can be possible to check if preferred value provided by comparing 0, >>>>>>>> but if 0 >>>>>>>> is a valid value that can be problem. It may not be problem with >>>>>>>> current >>>>>>>> variables but it may be when this struct extended, it may be good to >>>>>>>> think about >>>>>>>> alternative here. >>>>>>> >>>>>>> I don't think we should use the condition of "yes => use it by sending value 0". That is non-intuitive. Ideally, the application should query >>>>>> and then if query responds with value as '0' (which can be valid for some variables in future), it sends its own value to setup functions >>>>>> (whether '0' or something else, in case of 0 response, would depend on the knob). >>>>>> >>>>>> Right, at that stage application already knows what is the preferred value and >>>>>> can directly use it. >>>>>> >>>>>> >>>>>> Will it be too much to: >>>>>> >>>>>> 1) Adding a new field into "rte_eth_[rt]xconf" to say if exists prefer PMD >>>>>> values. "prefer_device_values" ? >>>>>> Application can provide values as usual, but if that field is set, abstraction >>>>>> layer overwrites the application values with PMD preferred ones. If there is no >>>>>> PMD preferred values continue using application ones. >>>>>> >>>>>> >>>>>> 2) Add a bitwise "is_set" field to new "preferred_size" struct, which may show >>>>>> status of other fields in the struct, if PMD set a valid value for them or not, >>>>>> so won't have to rely on the 0 check. >>>>> >>>>> That all seems like too much hassle for such small thing. >>>> >>>> Fair enough. >>>> >>>>> If we really want to allow PMD not to provide preferred values - >>>>> then instead of adding rte_eth_dev_pref_info into dev_info we can simply >>>>> introduce a new optional ethdev API call: >>>>> rte_eth_get_pref_params() or so. >>>>> If the PMD doesn’t want to provide preferred params to the user it simply >>>>> wouldn't implement that function. >>>> >>>> Same can be done with updated rte_eth_dev_info. >>>> Only application needs to check and use PMD preferred values, so this will mean >>>> dropping "pass 0 to get preferred values" feature in initial set. >>>> >>>>> >>> I actually don't see the issue with having ethdev provide reasonable >>> default values. If those don't work for a driver, then let the driver >>> provide it's own values. If the defaults don't work for an app, then let >>> the app override the provided values. >>> >>> It really is going to make the app writers job easier if we do things this >>> way. The only thing you are missing is the info as to whether it's ethdev >>> or the driver that's providing the values, but in the case that it's >>> ethdev, then the driver by definition "doesn't care", so we can treat them >>> as driver provided values. What's the downside? >> Abstraction layer having hardcoded config options doesn't look right to me. In >> long term who will ensure to make those values relevant? >> > > Let me turn that question around - in the long-term how likely are the > values to change significantly? Also, long-term all PMDs should provide > their own default values and then we can remove the values in the ethdev > layer. > >> When application provides a value of 0, it won't know if it is using PMD >> preferred values or some other defaults, what if application explicitly wants >> use PMD preferred values? > > If the PMD has preferred values, they will be automatically used. Is there > are case where the app would actually care about it? If the driver doesn't > provide default values, how is the app supposed to know what the correct > value for that driver is? And if the app *does* know what the best value > for a driver is - even if the driver itself doesn't, it can easily detect > when a port is using the driver and provide it's own ring setup defaults. > If you want, we can provide a flag field to indicate that fields are ethdev > defaults not driver defaults or something, but I'm struggling to come up > with a scenario where it would make a practical difference to an app. > >> >> The new fields are very similar to "default_[rt]xconf" in dev_info. Indeed >> perhaps we should use same naming convention because intention seems same. >> And we can continue to use new fields same as how "default_[rt]xconf" used. >> >> What about having something like rte_eth_tx_queue_setup_relaxed() where >> application really don't care about values, not sure why, which can get config >> values as much as from PMDs and fill the missing ones with the values defined in >> function? >> > > Or how about having the ethdev defaults in the rx/tx setup function instead > of in the dev_info one? If user specifies a zero size, we use the dev_info > value if provided by driver, otherwise ethdev default. That allows the > majority of apps to never worry about ring sizes, but for those that do, > they can query the driver defaults directly, or if not present set their > own. OK this at least gives a way to application to know where defaults are coming from. Hi Remy, Shreyansh, What do you think about using a variable name consistent with existing "default_[rt]xconf" in dev_info? > > /Bruce >