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? > > /Bruce >
