From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by dpdk.org (Postfix) with ESMTP id B73CD5F59 for ; Fri, 16 Mar 2018 15:18:53 +0100 (CET) X-Amp-Result: UNSCANNABLE X-Amp-File-Uploaded: False Received: from orsmga002.jf.intel.com ([10.7.209.21]) by orsmga103.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 16 Mar 2018 07:18:52 -0700 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.48,316,1517904000"; d="scan'208";a="42617983" Received: from bricha3-mobl.ger.corp.intel.com ([10.237.221.118]) by orsmga002.jf.intel.com with SMTP; 16 Mar 2018 07:18:48 -0700 Received: by (sSMTP sendmail emulation); Fri, 16 Mar 2018 14:18:47 +0000 Date: Fri, 16 Mar 2018 14:18:47 +0000 From: Bruce Richardson To: Shreyansh Jain Cc: Ferruh Yigit , "Horton, Remy" , "Ananyev, Konstantin" , "dev@dpdk.org" , "Lu, Wenzhuo" , "Wu, Jingjing" , "Zhang, Qi Z" , "Xing, Beilei" , Thomas Monjalon Message-ID: <20180316141847.GA13900@bricha3-MOBL.ger.corp.intel.com> References: <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> <97dc9f9d-041b-ef99-2ca6-1f557c4f6039@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Organization: Intel Research and Development Ireland Ltd. User-Agent: Mutt/1.9.4 (2018-02-28) 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: Fri, 16 Mar 2018 14:18:54 -0000 On Fri, Mar 16, 2018 at 07:24:14PM +0530, Shreyansh Jain wrote: > On Thu, Mar 15, 2018 at 8:27 PM, Ferruh Yigit wrote: > > 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? > > It just turned out to be much more complex than I initially thought :) > Is this what the above conversation merging at (for Rx, as example): > > 1. 'default_rx_size_conf' is added in rte_eth_dev_info (and this > includes I/O params like burst size, besides configure time nb_queue, > nb_desc etc). Driver would return these values filled in when > info_get() is called. > > 2a. If an application needs the defaults, it would perform info_get() > and get the values. then, use the values in configuration APIs > (rx_queue_setup for nb_rx_desc, eth_dev_dev_configure for > nb_rx_queues). > For rx_burst calls, it would use the burst_size fields obtained from info_get(). > This is good enough for configuration and datapath (rx_burst). > > OR, another case > > 2b. Application wants to use default vaules provided by driver without > calling info_get. In which case, it would call > rx_queue_setup(nb_rx_desc=0..) or eth_dev_configure(nb_rx_queue=0, > nb_tx_queue=0). The implementation would query the value from > 'default_rx_size_conf' through info_get() and use those values. > Though, in this case, rte_eth_rx_burst(burst=0) might not work for > picking up the default within rte_ethdev.h. > > :Four observations: > A). For burst size (or any other I/O time value added in future), > values would have to be explicitly used by application - always. If > value reported by info_get() is '0' (see (B) below), application to > use its own judgement. No default override by lib_eal. > IMO, This is good enough assumption. > > B). '0' as an indicator for 'no-default-value-available-from-driver' > is still an open point. It is good enough for current proposed > parameters, but may be a valid numerical value in future. > IMO, this can be ignored for now. It may be safer to choose -1 as default here, though whatever value we choose there is always a change it could be valid. > > C) Unlike the original proposal, this would add two separate members > to rte_eth_dev_info - one each for Rx and Tx. They both are still > expected to be populated through the info_get() implementation but not > by lib_eal. > IMO, doesn't matter. > > D) Would there be no non-Rx and non-Tx defaults which need to be shared? > I am not sure about this, though. > > Sorry if I am repeating everything again, but I got lost in the > conversation and needed to break it again.