Re: [RFC PATCH v1 1/4] ethdev: add support for PMD-tuned Tx/Rx parameters

[Bruce Richardson <bruce.richardson@intel.com>]

On Fri, Mar 16, 2018 at 07:24:14PM +0530, Shreyansh Jain wrote:
> On Thu, Mar 15, 2018 at 8:27 PM, Ferruh Yigit <ferruh.yigit@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-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> >>>>>>> Sent: Wednesday, March 14, 2018 5:52 PM
> >>>>>>> To: Shreyansh Jain <shreyansh.jain@nxp.com>; Horton, Remy <remy.horton@intel.com>; dev@dpdk.org
> >>>>>>> Cc: Lu, Wenzhuo <wenzhuo.lu@intel.com>; Wu, Jingjing <jingjing.wu@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>; Xing, Beilei
> >>>>>>> <beilei.xing@intel.com>; Thomas Monjalon <thomas@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.yigit@intel.com]
> >>>>>>>>> Sent: Wednesday, March 14, 2018 10:13 PM
> >>>>>>>>> To: Remy Horton <remy.horton@intel.com>; dev@dpdk.org
> >>>>>>>>> Cc: Wenzhuo Lu <wenzhuo.lu@intel.com>; Jingjing Wu
> >>>>>>>>> <jingjing.wu@intel.com>; Qi Zhang <qi.z.zhang@intel.com>; Beilei Xing
> >>>>>>>>> <beilei.xing@intel.com>; Shreyansh Jain <shreyansh.jain@nxp.com>;
> >>>>>>>>> Thomas Monjalon <thomas@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.

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.