On Thu, Mar 15, 2018 at 8:27 PM, Ferruh Yigit <ferruh.yi...@intel.com> 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-boun...@dpdk.org] On Behalf Of Ferruh Yigit >>>>>>> Sent: Wednesday, March 14, 2018 5:52 PM >>>>>>> To: Shreyansh Jain <shreyansh.j...@nxp.com>; Horton, Remy >>>>>>> <remy.hor...@intel.com>; dev@dpdk.org >>>>>>> Cc: Lu, Wenzhuo <wenzhuo...@intel.com>; Wu, Jingjing >>>>>>> <jingjing...@intel.com>; Zhang, Qi Z <qi.z.zh...@intel.com>; Xing, >>>>>>> Beilei >>>>>>> <beilei.x...@intel.com>; Thomas Monjalon <tho...@monjalon.net> >>>>>>> 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.yi...@intel.com] >>>>>>>>> Sent: Wednesday, March 14, 2018 10:13 PM >>>>>>>>> To: Remy Horton <remy.hor...@intel.com>; dev@dpdk.org >>>>>>>>> Cc: Wenzhuo Lu <wenzhuo...@intel.com>; Jingjing Wu >>>>>>>>> <jingjing...@intel.com>; Qi Zhang <qi.z.zh...@intel.com>; Beilei Xing >>>>>>>>> <beilei.x...@intel.com>; Shreyansh Jain <shreyansh.j...@nxp.com>; >>>>>>>>> Thomas Monjalon <tho...@monjalon.net> >>>>>>>>> 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. 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.
