diff mbox

[dpdk-dev,RFC,v1,1/4] ethdev: add support for PMD-tuned Tx/Rx parameters

Message ID 20180307120851.5822-2-remy.horton@intel.com (mailing list archive)
State Superseded, archived
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Remy Horton March 7, 2018, 12:08 p.m. UTC 
  The optimal values of several transmission & reception related
parameters, such as burst sizes, descriptor ring sizes, and number
of queues, varies between different network interface devices. This
patch allows individual PMDs to specify preferred parameter values.

Signed-off-by: Remy Horton <remy.horton@intel.com>
---
 lib/librte_ether/rte_ethdev.c | 18 ++++++++++++++++++
 lib/librte_ether/rte_ethdev.h | 15 +++++++++++++++
 2 files changed, 33 insertions(+)

Comments

Shreyansh Jain March 14, 2018, 12:28 p.m. UTC | #1 
> -----Original Message-----
> From: Remy Horton [mailto:remy.horton@intel.com]
> Sent: Wednesday, March 7, 2018 5:39 PM
> To: 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: [RFC PATCH v1 1/4] ethdev: add support for PMD-tuned Tx/Rx
> parameters
> 
> The optimal values of several transmission & reception related
> parameters, such as burst sizes, descriptor ring sizes, and number
> of queues, varies between different network interface devices. This
> patch allows individual PMDs to specify preferred parameter values.
> 
> Signed-off-by: Remy Horton <remy.horton@intel.com>
> ---
>  lib/librte_ether/rte_ethdev.c | 18 ++++++++++++++++++
>  lib/librte_ether/rte_ethdev.h | 15 +++++++++++++++
>  2 files changed, 33 insertions(+)
> 
> diff --git a/lib/librte_ether/rte_ethdev.c
> b/lib/librte_ether/rte_ethdev.c
> index 0590f0c..1630407 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -1461,6 +1461,10 @@ rte_eth_rx_queue_setup(uint16_t port_id,
> uint16_t rx_queue_id,
>  		return -EINVAL;
>  	}
> 
> +	/* Use default specified by driver, if nb_rc_desc is zero */
                                            ^^^^^^^^^^^
                                         Should be '_rx_'

> +	if (nb_rx_desc == 0)
> +		nb_rx_desc = dev_info.preferred_queue_values.rx_ring_size;
> +
>  	if (nb_rx_desc > dev_info.rx_desc_lim.nb_max ||
>  			nb_rx_desc < dev_info.rx_desc_lim.nb_min ||
>  			nb_rx_desc % dev_info.rx_desc_lim.nb_align != 0) {
> @@ -1584,6 +1588,10 @@ rte_eth_tx_queue_setup(uint16_t port_id,
> uint16_t tx_queue_id,
> 

[...]

Other than the above;

Acked-by: Shreyansh Jain <Shreyansh.jain@nxp.com>
Remy Horton March 14, 2018, 2:09 p.m. UTC | #2 
On 14/03/2018 12:28, Shreyansh Jain wrote:
[..]
>> +	/* Use default specified by driver, if nb_rc_desc is zero */
>                                             ^^^^^^^^^^^
>                                          Should be '_rx_'
[..]
> Other than the above;

Ok - v2 on the way :)

>
> Acked-by: Shreyansh Jain <Shreyansh.jain@nxp.com>
>
Ferruh Yigit March 14, 2018, 2:43 p.m. UTC | #3 
On 3/7/2018 12:08 PM, Remy Horton wrote:
> The optimal values of several transmission & reception related
> parameters, such as burst sizes, descriptor ring sizes, and number
> of queues, varies between different network interface devices. This
> patch allows individual PMDs to specify preferred parameter values.
> 
> Signed-off-by: Remy Horton <remy.horton@intel.com>
> ---
>  lib/librte_ether/rte_ethdev.c | 18 ++++++++++++++++++
>  lib/librte_ether/rte_ethdev.h | 15 +++++++++++++++

Can you please remove deprecation notice in this patch.

>  2 files changed, 33 insertions(+)
> 
> diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
> index 0590f0c..1630407 100644
> --- a/lib/librte_ether/rte_ethdev.c
> +++ b/lib/librte_ether/rte_ethdev.c
> @@ -1461,6 +1461,10 @@ rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
>  		return -EINVAL;
>  	}
>  
> +	/* Use default specified by driver, if nb_rc_desc is zero */
> +	if (nb_rx_desc == 0)
> +		nb_rx_desc = dev_info.preferred_queue_values.rx_ring_size;
> +
>  	if (nb_rx_desc > dev_info.rx_desc_lim.nb_max ||
>  			nb_rx_desc < dev_info.rx_desc_lim.nb_min ||
>  			nb_rx_desc % dev_info.rx_desc_lim.nb_align != 0) {
> @@ -1584,6 +1588,10 @@ rte_eth_tx_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
>  
>  	rte_eth_dev_info_get(port_id, &dev_info);
>  
> +	/* Use default specified by driver, if nb_tx_desc is zero */
> +	if (nb_tx_desc == 0)
> +		nb_tx_desc = dev_info.preferred_queue_values.tx_ring_size;
> +
>  	if (nb_tx_desc > dev_info.tx_desc_lim.nb_max ||
>  	    nb_tx_desc < dev_info.tx_desc_lim.nb_min ||
>  	    nb_tx_desc % dev_info.tx_desc_lim.nb_align != 0) {
> @@ -2394,6 +2402,16 @@ rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
>  	dev_info->rx_desc_lim = lim;
>  	dev_info->tx_desc_lim = lim;
>  
> +	/* Defaults for drivers that don't implement preferred
> +	 * queue parameters.
> +	 */
> +	dev_info->preferred_queue_values.rx_burst_size = 0;
> +	dev_info->preferred_queue_values.tx_burst_size = 0;
> +	dev_info->preferred_queue_values.nb_rx_queues = 1;
> +	dev_info->preferred_queue_values.nb_tx_queues = 1;
> +	dev_info->preferred_queue_values.rx_ring_size = 1024;
> +	dev_info->preferred_queue_values.tx_ring_size = 1024;


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?

> +
>  	RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_infos_get);
>  	(*dev->dev_ops->dev_infos_get)(dev, dev_info);
>  	dev_info->driver_name = dev->device->driver->name;
> diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
> index 0361533..67ce82d 100644
> --- a/lib/librte_ether/rte_ethdev.h
> +++ b/lib/librte_ether/rte_ethdev.h
> @@ -988,6 +988,18 @@ struct rte_eth_conf {
>  
>  struct rte_pci_device;
>  
> +/*
> + * Preferred queue parameters.
> + */
> +struct rte_eth_dev_pref_queue_info {
> +	uint16_t rx_burst_size;
> +	uint16_t tx_burst_size;
> +	uint16_t rx_ring_size;
> +	uint16_t tx_ring_size;
> +	uint16_t nb_rx_queues;
> +	uint16_t nb_tx_queues;
> +};
> +
>  /**
>   * Ethernet device information
>   */
> @@ -1029,6 +1041,9 @@ struct rte_eth_dev_info {
>  	/** Configured number of rx/tx queues */
>  	uint16_t nb_rx_queues; /**< Number of RX queues. */
>  	uint16_t nb_tx_queues; /**< Number of TX queues. */
> +
> +	/** Queue size recommendations */

Not only queue size.

> +	struct rte_eth_dev_pref_queue_info preferred_queue_values;

Although these are queue related values, not per-queue but per-port, the
variable name "preferred_queue_values" gives the impression that these are per
queue. And "rx_burst_size" is not related to queue at all I think.

What do you think renaming structure and variable name, "preferred_dev_config"
perhaps?

>  };
>  
>  /**
>
Shreyansh Jain March 14, 2018, 3:10 p.m. UTC | #4 
> -----Original Message-----

> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]

> Sent: Wednesday, March 14, 2018 8:14 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/7/2018 12:08 PM, Remy Horton wrote:

> > The optimal values of several transmission & reception related

> > parameters, such as burst sizes, descriptor ring sizes, and number

> > of queues, varies between different network interface devices. This

> > patch allows individual PMDs to specify preferred parameter values.

> >

> > Signed-off-by: Remy Horton <remy.horton@intel.com>

> > ---

> >  lib/librte_ether/rte_ethdev.c | 18 ++++++++++++++++++

> >  lib/librte_ether/rte_ethdev.h | 15 +++++++++++++++

> 


[...]

> 

> > +	struct rte_eth_dev_pref_queue_info preferred_queue_values;

> 

> Although these are queue related values, not per-queue but per-port,

> the

> variable name "preferred_queue_values" gives the impression that these

> are per

> queue. And "rx_burst_size" is not related to queue at all I think.

> 

> What do you think renaming structure and variable name,

> "preferred_dev_config"

> perhaps?


I missed this naming while reading this patch.
In the deprecation notice, 'preferred_size' was the name we came up with precisely on this issue of structure having queue length and burst size.

What about using that same name?
 
> 

> >  };

> >

> >  /**

> >
Remy Horton March 14, 2018, 3:48 p.m. UTC | #5 
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.
Ferruh Yigit March 14, 2018, 4:42 p.m. UTC | #6 
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.
Shreyansh Jain March 14, 2018, 5:23 p.m. UTC | #7 
> -----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).

Existing example applications should be changed for this. It is tedious, but gives a true example usage.
Ferruh Yigit March 14, 2018, 5:52 p.m. UTC | #8 
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.

> 
> Existing example applications should be changed for this. It is tedious, but gives a true example usage.

Applications already needs to be updated to use this, important part is
modification is optional.

>
Ananyev, Konstantin March 14, 2018, 6:53 p.m. UTC | #9 
> -----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.
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. 

Konstantin

> 

> >

> > Existing example applications should be changed for this. It is tedious, but gives a true example usage.

> 

> Applications already needs to be updated to use this, important part is

> modification is optional.

> 

> >
Ferruh Yigit March 14, 2018, 9:02 p.m. UTC | #10 
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.

> 
> Konstantin
> 
>>
>>>
>>> Existing example applications should be changed for this. It is tedious, but gives a true example usage.
>>
>> Applications already needs to be updated to use this, important part is
>> modification is optional.
>>
>>>
>
Bruce Richardson March 14, 2018, 9:36 p.m. UTC | #11 
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?

/Bruce
Remy Horton March 15, 2018, 9:02 a.m. UTC | #12 
On 14/03/2018 15:10, Shreyansh Jain wrote:
[..]
>> What do you think renaming structure and variable name,
>> "preferred_dev_config" perhaps?
>
> I missed this naming while reading this patch. In the deprecation
> notice, 'preferred_size' was the name we came up with precisely on
> this issue of structure having queue length and burst size.
>
> What about using that same name?

Because of namespace issues it will need at least the rte_ prefix. 
Otherwise seems good to me as I cannot think of anything else that is 
relatively short.
Ananyev, Konstantin March 15, 2018, 12:51 p.m. UTC | #13 
> -----Original Message-----

> From: Yigit, Ferruh

> Sent: Wednesday, March 14, 2018 9:03 PM

> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; 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 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.


That's ok by me, but it means every PMD has to provide some preferred values?
Konstantin
Ferruh Yigit March 15, 2018, 1:57 p.m. UTC | #14 
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?

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?

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?

> 
> /Bruce
>
Ferruh Yigit March 15, 2018, 1:57 p.m. UTC | #15 
On 3/15/2018 12:51 PM, Ananyev, Konstantin wrote:
> 
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Wednesday, March 14, 2018 9:03 PM
>> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; 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 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.
> 
> That's ok by me, but it means every PMD has to provide some preferred values?

No don't have to, if PMD provides preferred values app will use it, if not app
defined values will be used.

> Konstantin
>
Bruce Richardson March 15, 2018, 2:39 p.m. UTC | #16 
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.

/Bruce
Bruce Richardson March 15, 2018, 2:42 p.m. UTC | #17 
On Thu, Mar 15, 2018 at 01:57:31PM +0000, Ferruh Yigit wrote:
> On 3/15/2018 12:51 PM, Ananyev, Konstantin wrote:
> > 
> > 
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Wednesday, March 14, 2018 9:03 PM
> >> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; 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 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.
> > 
> > That's ok by me, but it means every PMD has to provide some preferred values?
> 
> No don't have to, if PMD provides preferred values app will use it, if not app
> defined values will be used.
> 
I don't like forcing apps to provide values for this. For usability the
DPDK should be set up in a way such that an app writer can use defaults
without having to think about it. We also need to provide hooks for
specific use cases where the app writer does care, but we should provide
the "easy" path too. It's worked well for the queue configuration structs,
and I believe it will work fine for ring sizes too.

/Bruce
Ferruh Yigit March 15, 2018, 2:57 p.m. UTC | #18 
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?

> 
> /Bruce
>
Shreyansh Jain March 16, 2018, 1:54 p.m. UTC | #19 
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.

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.
Bruce Richardson March 16, 2018, 2:18 p.m. UTC | #20 
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.
Remy Horton March 16, 2018, 3:36 p.m. UTC | #21 
On 16/03/2018 13:54, 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:
[..]
>> 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.

At the moment thinking of the names below, based in what I've read so far..

struct rte_eth_dev_preferred_size {
	uint16_t rx_burst;
	uint16_t tx_burst;
	uint16_t rx_ring;
	uint16_t tx_ring;
	uint16_t rx_nb_queues;
	uint16_t tx_nb_queues;
};
struct rte_eth_dev_info {
	/* ... */
	struct rte_eth_dev_preferred_size preferred_size;
};

Since Rx and Tx use the same parameters, a possible alternative is 
below, although such separation of Rx & Tx was not something I was 
planning on doing:

struct rte_eth_dev_preferred_size {
	uint16_t burst;
	uint16_t ring;
	uint16_t nb_queues;
};
struct rte_eth_dev_info {
	/* ... */
	struct rte_eth_dev_preferred_size preferred_rx;
	struct rte_eth_dev_preferred_size preferred_tx;
};


> 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).

There was also the suggestion of adding a completely new API function 
rather than using info_get() although there might be some resistance as 
struct eth_dev_ops is already pretty large.


> 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.

Since rte_eth_*_burst() are fast-path functions, they are places I would 
prefer not to put conditionals.


> :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.

There's been quite a bit of discussion whether ethdev should provide 
fall-back values if the PMD doesn't. In this case applications can 
assume the value is always valid and it makes the 0-as-indicator issue 
disappear, but it comes with its own set of issues.


> D) Would there be no non-Rx and non-Tx defaults which need to be shared?
> I am not sure about this, though.

I can't think of any off-hand.

>
> Sorry if I am repeating everything again, but I got lost in the
> conversation and needed to break it again.
>
Ferruh Yigit March 20, 2018, 2:54 p.m. UTC | #22 
On 3/16/2018 1:54 PM, 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.

In Bruce's suggestion where ethdev keep defaults is changed.
Initial suggestion was rte_eth_dev_info_get() filling default data, now defaults
will be defined in functions like rte_eth_rx_queue_setup().

This is a little different from filling defaults in rte_eth_dev_info_get():
- Application can know where the defaults are coming from because dev_info
fields are only modified by PMD. Application still prefer to use ethdev defaults.

- The default values in ethdev library provided in function related to that
data, instead of separate rte_eth_dev_info_get() function.


What application can do:
-  Application can call rte_eth_dev_info_get() and can learn if PMD provided
defaults or not.
- If PMD doesn't provided any default values application can prefer to use
application defined values. This may be an option for the application looking
for most optimized values.
- Although PMD doesn't provide any defaults, application still can use defaults
provided by ethdev by providing '0' as arguments.


So how related ethdev functions will be updated:
if argument != 0
  use argument
else
  dev_info_get()
    if dev_info->argument != 0
      use dev_info->argument
    else
      use function_prov

> 
> :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.

This is no more true after Bruce's comment.
If application provides any values it will overwrite everything else,
application has the final word.
But application may prefer to use provided default values.

> 
> 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.

Agree that we can ignore it 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.

There won't be new members, which ones are you talking about?

> 
> 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.
>
Ferruh Yigit March 20, 2018, 3:03 p.m. UTC | #23 
On 3/16/2018 3:36 PM, Remy Horton wrote:
> 
> On 16/03/2018 13:54, 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:
> [..]
>>> 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.
> 
> At the moment thinking of the names below, based in what I've read so far..
> 
> struct rte_eth_dev_preferred_size {
> 	uint16_t rx_burst;
> 	uint16_t tx_burst;
> 	uint16_t rx_ring;
> 	uint16_t tx_ring;
> 	uint16_t rx_nb_queues;
> 	uint16_t tx_nb_queues;
> };
> struct rte_eth_dev_info {
> 	/* ... */
> 	struct rte_eth_dev_preferred_size preferred_size;
> };
> 
> Since Rx and Tx use the same parameters, a possible alternative is 
> below, although such separation of Rx & Tx was not something I was 
> planning on doing:
> 
> struct rte_eth_dev_preferred_size {
> 	uint16_t burst;
> 	uint16_t ring;
> 	uint16_t nb_queues;
> };
> struct rte_eth_dev_info {
> 	/* ... */
> 	struct rte_eth_dev_preferred_size preferred_rx;
> 	struct rte_eth_dev_preferred_size preferred_tx;
> };

Hi Remy,

There are already two members in "struct rte_eth_dev_info":
"struct rte_eth_rxconf default_rxconf;"
"struct rte_eth_txconf default_txconf;"

These two are filled by PMDs. I think we can say these are PMD preferred values
for rte_eth_[rt]xconf structs.

Right now we are extending the preferred values that PMDs can provide.

So what about using same naming convention to be consistent with existing usage?
Something like "struct rte_eth_portconf default_portconf"?

> 
> 
>> 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).
> 
> There was also the suggestion of adding a completely new API function 
> rather than using info_get() although there might be some resistance as 
> struct eth_dev_ops is already pretty large.
> 
> 
>> 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.
> 
> Since rte_eth_*_burst() are fast-path functions, they are places I would 
> prefer not to put conditionals.
> 
> 
>> :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.
> 
> There's been quite a bit of discussion whether ethdev should provide 
> fall-back values if the PMD doesn't. In this case applications can 
> assume the value is always valid and it makes the 0-as-indicator issue 
> disappear, but it comes with its own set of issues.
> 
> 
>> D) Would there be no non-Rx and non-Tx defaults which need to be shared?
>> I am not sure about this, though.
> 
> I can't think of any off-hand.
> 
>>
>> Sorry if I am repeating everything again, but I got lost in the
>> conversation and needed to break it again.
>>
Shreyansh Jain March 21, 2018, 6:51 a.m. UTC | #24 
On Tue, Mar 20, 2018 at 8:24 PM, Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> On 3/16/2018 1:54 PM, Shreyansh Jain wrote:
>> On Thu, Mar 15, 2018 at 8:27 PM, Ferruh Yigit <ferruh.yigit@intel.com> wrote:

[...]

>>> 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.
>
> In Bruce's suggestion where ethdev keep defaults is changed.
> Initial suggestion was rte_eth_dev_info_get() filling default data, now defaults
> will be defined in functions like rte_eth_rx_queue_setup().
>
> This is a little different from filling defaults in rte_eth_dev_info_get():
> - Application can know where the defaults are coming from because dev_info
> fields are only modified by PMD. Application still prefer to use ethdev defaults.
>
> - The default values in ethdev library provided in function related to that
> data, instead of separate rte_eth_dev_info_get() function.

It seems we both are on same page (almost) - just that I couldn't
articulate my comments properly earlier, maybe.

rte_eth_dev_info_get is only a method to get defaults set by PMDs.
dev_info_get is not setting defaults by itself. I get this.

>
>
> What application can do:
> -  Application can call rte_eth_dev_info_get() and can learn if PMD provided
> defaults or not.
> - If PMD doesn't provided any default values application can prefer to use
> application defined values. This may be an option for the application looking
> for most optimized values.
> - Although PMD doesn't provide any defaults, application still can use defaults
> provided by ethdev by providing '0' as arguments.

Yes, agree - and only comment I added previously in this case is that
this is not applicable for burst APIs. So, optimal [rt]x burst size
cannot be 'defaulted' to EAL layer. Other values like ring size, queue
count can be delegated to EAL for overwriting if passed as '0'.

>
>
> So how related ethdev functions will be updated:
> if argument != 0
>   use argument
> else
>   dev_info_get()
>     if dev_info->argument != 0
>       use dev_info->argument
>     else
>       use function_prov

Perfect, but only for eth_dev_configure and eth_[rt]x_queue_setup functions -
and that is OK with me.

>
>>
>> :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.
>
> This is no more true after Bruce's comment.
> If application provides any values it will overwrite everything else,
> application has the final word.
> But application may prefer to use provided default values.

I am not sure what has changed with Bruce's comment - but I agree with
what you are stating.

>
>>
>> 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.
>
> Agree that we can ignore it 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.
>
> There won't be new members, which ones are you talking about?

original proposal: (ignore change of names, please)

 rte_eth_dev_preferred_info {
     rx_burst_size
     tx_burst_size
     rx_ring_size
     tx_ring_size
     ...
  }

And this is what I think last few comments intended:

 rte_eth_rxpreferred {
   ...
   rx_burst_size
   rx_ring_size
   ...
 }

 rte_eth_txpreferred {
   ...
   tx_burst_size
   tx_ring_size
   ...
 }

both the above added rte_eth_dev_info{}

This is what I meant when I stated "...this would add two separate
members to rte_eth_dev_info - one each for Rx and Tx..."

[...]

-
Shreyansh
Ferruh Yigit March 21, 2018, 10:02 a.m. UTC | #25 
On 3/21/2018 6:51 AM, Shreyansh Jain wrote:
> On Tue, Mar 20, 2018 at 8:24 PM, Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>> On 3/16/2018 1:54 PM, Shreyansh Jain wrote:
>>> On Thu, Mar 15, 2018 at 8:27 PM, Ferruh Yigit <ferruh.yigit@intel.com> wrote:
> 
> [...]
> 
>>>> 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.
>>
>> In Bruce's suggestion where ethdev keep defaults is changed.
>> Initial suggestion was rte_eth_dev_info_get() filling default data, now defaults
>> will be defined in functions like rte_eth_rx_queue_setup().
>>
>> This is a little different from filling defaults in rte_eth_dev_info_get():
>> - Application can know where the defaults are coming from because dev_info
>> fields are only modified by PMD. Application still prefer to use ethdev defaults.
>>
>> - The default values in ethdev library provided in function related to that
>> data, instead of separate rte_eth_dev_info_get() function.
> 
> It seems we both are on same page (almost) - just that I couldn't
> articulate my comments properly earlier, maybe.
> 
> rte_eth_dev_info_get is only a method to get defaults set by PMDs.
> dev_info_get is not setting defaults by itself. I get this.
> 
>>
>>
>> What application can do:
>> -  Application can call rte_eth_dev_info_get() and can learn if PMD provided
>> defaults or not.
>> - If PMD doesn't provided any default values application can prefer to use
>> application defined values. This may be an option for the application looking
>> for most optimized values.
>> - Although PMD doesn't provide any defaults, application still can use defaults
>> provided by ethdev by providing '0' as arguments.
> 
> Yes, agree - and only comment I added previously in this case is that
> this is not applicable for burst APIs. So, optimal [rt]x burst size
> cannot be 'defaulted' to EAL layer. Other values like ring size, queue
> count can be delegated to EAL for overwriting if passed as '0'.

Yes you are right.

> 
>>
>>
>> So how related ethdev functions will be updated:
>> if argument != 0
>>   use argument
>> else
>>   dev_info_get()
>>     if dev_info->argument != 0
>>       use dev_info->argument
>>     else
>>       use function_prov
> 
> Perfect, but only for eth_dev_configure and eth_[rt]x_queue_setup functions -
> and that is OK with me.
> 
>>
>>>
>>> :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.
>>
>> This is no more true after Bruce's comment.
>> If application provides any values it will overwrite everything else,
>> application has the final word.
>> But application may prefer to use provided default values.
> 
> I am not sure what has changed with Bruce's comment - but I agree with
> what you are stating.
> 
>>
>>>
>>> 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.
>>
>> Agree that we can ignore it 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.
>>
>> There won't be new members, which ones are you talking about?
> 
> original proposal: (ignore change of names, please)
> 
>  rte_eth_dev_preferred_info {
>      rx_burst_size
>      tx_burst_size
>      rx_ring_size
>      tx_ring_size
>      ...
>   }
> 
> And this is what I think last few comments intended:
> 
>  rte_eth_rxpreferred {
>    ...
>    rx_burst_size
>    rx_ring_size
>    ...
>  }
> 
>  rte_eth_txpreferred {
>    ...
>    tx_burst_size
>    tx_ring_size
>    ...
>  }
> 
> both the above added rte_eth_dev_info{}
> 
> This is what I meant when I stated "...this would add two separate
> members to rte_eth_dev_info - one each for Rx and Tx..."

Got it. I don't have any strong opinion on adding single struct or two (one for
Rx and one for Tx).
Since these will be public structs, do you think will there be any difference
from ABI stability point of view?

> 
> [...]
> 
> -
> Shreyansh
>
Remy Horton March 21, 2018, 10:14 a.m. UTC | #26 
On 20/03/2018 15:03, Ferruh Yigit wrote:
> On 3/16/2018 3:36 PM, Remy Horton wrote:
[..]

>> struct rte_eth_dev_preferred_size {
>> 	uint16_t burst;
>> 	uint16_t ring;
>> 	uint16_t nb_queues;
>> };
>> struct rte_eth_dev_info {
>> 	/* ... */
>> 	struct rte_eth_dev_preferred_size preferred_rx;
>> 	struct rte_eth_dev_preferred_size preferred_tx;
>> };
>
> Hi Remy,
>
> There are already two members in "struct rte_eth_dev_info":
> "struct rte_eth_rxconf default_rxconf;"
> "struct rte_eth_txconf default_txconf;"
>
> These two are filled by PMDs. I think we can say these are PMD preferred values
> for rte_eth_[rt]xconf structs.
>
> Right now we are extending the preferred values that PMDs can provide.
>
> So what about using same naming convention to be consistent with existing usage?
> Something like "struct rte_eth_portconf default_portconf"?


Would default_[rt]xportconf be ok?

I would consider adding the parameters to rte_eth_[rt]xconf rather than 
creating a new rte_eth_portconf but since the former is used elsewhere 
this might cause complications.
Shreyansh Jain March 21, 2018, 10:45 a.m. UTC | #27 
> -----Original Message-----

> From: Ferruh Yigit [mailto:ferruh.yigit@intel.com]

> Sent: Wednesday, March 21, 2018 3:33 PM

> To: Shreyansh Jain <shreyansh.jain@nxp.com>

> Cc: Bruce Richardson <bruce.richardson@intel.com>; Horton, Remy

> <remy.horton@intel.com>; Ananyev, Konstantin

> <konstantin.ananyev@intel.com>; dev@dpdk.org; 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/21/2018 6:51 AM, Shreyansh Jain wrote:

> > On Tue, Mar 20, 2018 at 8:24 PM, Ferruh Yigit <ferruh.yigit@intel.com>

> wrote:

> >> On 3/16/2018 1:54 PM, Shreyansh Jain wrote:

> >>> On Thu, Mar 15, 2018 at 8:27 PM, Ferruh Yigit

> <ferruh.yigit@intel.com> wrote:

> >

> > [...]

> >


[...]

> >>>

> >>> 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.

> >>

> >> There won't be new members, which ones are you talking about?

> >

> > original proposal: (ignore change of names, please)

> >

> >  rte_eth_dev_preferred_info {

> >      rx_burst_size

> >      tx_burst_size

> >      rx_ring_size

> >      tx_ring_size

> >      ...

> >   }

> >

> > And this is what I think last few comments intended:

> >

> >  rte_eth_rxpreferred {

> >    ...

> >    rx_burst_size

> >    rx_ring_size

> >    ...

> >  }

> >

> >  rte_eth_txpreferred {

> >    ...

> >    tx_burst_size

> >    tx_ring_size

> >    ...

> >  }

> >

> > both the above added rte_eth_dev_info{}

> >

> > This is what I meant when I stated "...this would add two separate

> > members to rte_eth_dev_info - one each for Rx and Tx..."

> 

> Got it. I don't have any strong opinion on adding single struct or two

> (one for

> Rx and one for Tx).

> Since these will be public structs, do you think will there be any

> difference

> from ABI stability point of view?


No. It was just an observation. To me, it doesn't matter which approach is selected.

-
Shreyansh
Ferruh Yigit March 21, 2018, 1:56 p.m. UTC | #28 
On 3/21/2018 10:14 AM, Remy Horton wrote:
> 
> On 20/03/2018 15:03, Ferruh Yigit wrote:
>> On 3/16/2018 3:36 PM, Remy Horton wrote:
> [..]
> 
>>> struct rte_eth_dev_preferred_size {
>>> 	uint16_t burst;
>>> 	uint16_t ring;
>>> 	uint16_t nb_queues;
>>> };
>>> struct rte_eth_dev_info {
>>> 	/* ... */
>>> 	struct rte_eth_dev_preferred_size preferred_rx;
>>> 	struct rte_eth_dev_preferred_size preferred_tx;
>>> };
>>
>> Hi Remy,
>>
>> There are already two members in "struct rte_eth_dev_info":
>> "struct rte_eth_rxconf default_rxconf;"
>> "struct rte_eth_txconf default_txconf;"
>>
>> These two are filled by PMDs. I think we can say these are PMD preferred values
>> for rte_eth_[rt]xconf structs.
>>
>> Right now we are extending the preferred values that PMDs can provide.
>>
>> So what about using same naming convention to be consistent with existing usage?
>> Something like 
> 
> 
> Would default_[rt]xportconf be ok?

not sure, rxportconf seems long word we can put some "_" perhaps, and "port"
seems not used in existing data structures but I can't think of anything to
replace it.

> 
> I would consider adding the parameters to rte_eth_[rt]xconf rather than 
> creating a new rte_eth_portconf but since the former is used elsewhere 
> this might cause complications.

Also they are specific to Rx/Tx queues but the values we are adding are not
specific per queue.

But what we are adding is more like:
"struct rte_eth_txmode"
"struct rte_eth_rxmode"

Which are documented as "Rx / Tx features of an Ethernet port" but these are not
part of dev_info.
diff mbox

Patch

diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 0590f0c..1630407 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1461,6 +1461,10 @@  rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
 		return -EINVAL;
 	}
 
+	/* Use default specified by driver, if nb_rc_desc is zero */
+	if (nb_rx_desc == 0)
+		nb_rx_desc = dev_info.preferred_queue_values.rx_ring_size;
+
 	if (nb_rx_desc > dev_info.rx_desc_lim.nb_max ||
 			nb_rx_desc < dev_info.rx_desc_lim.nb_min ||
 			nb_rx_desc % dev_info.rx_desc_lim.nb_align != 0) {
@@ -1584,6 +1588,10 @@  rte_eth_tx_queue_setup(uint16_t port_id, uint16_t tx_queue_id,
 
 	rte_eth_dev_info_get(port_id, &dev_info);
 
+	/* Use default specified by driver, if nb_tx_desc is zero */
+	if (nb_tx_desc == 0)
+		nb_tx_desc = dev_info.preferred_queue_values.tx_ring_size;
+
 	if (nb_tx_desc > dev_info.tx_desc_lim.nb_max ||
 	    nb_tx_desc < dev_info.tx_desc_lim.nb_min ||
 	    nb_tx_desc % dev_info.tx_desc_lim.nb_align != 0) {
@@ -2394,6 +2402,16 @@  rte_eth_dev_info_get(uint16_t port_id, struct rte_eth_dev_info *dev_info)
 	dev_info->rx_desc_lim = lim;
 	dev_info->tx_desc_lim = lim;
 
+	/* Defaults for drivers that don't implement preferred
+	 * queue parameters.
+	 */
+	dev_info->preferred_queue_values.rx_burst_size = 0;
+	dev_info->preferred_queue_values.tx_burst_size = 0;
+	dev_info->preferred_queue_values.nb_rx_queues = 1;
+	dev_info->preferred_queue_values.nb_tx_queues = 1;
+	dev_info->preferred_queue_values.rx_ring_size = 1024;
+	dev_info->preferred_queue_values.tx_ring_size = 1024;
+
 	RTE_FUNC_PTR_OR_RET(*dev->dev_ops->dev_infos_get);
 	(*dev->dev_ops->dev_infos_get)(dev, dev_info);
 	dev_info->driver_name = dev->device->driver->name;
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 0361533..67ce82d 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -988,6 +988,18 @@  struct rte_eth_conf {
 
 struct rte_pci_device;
 
+/*
+ * Preferred queue parameters.
+ */
+struct rte_eth_dev_pref_queue_info {
+	uint16_t rx_burst_size;
+	uint16_t tx_burst_size;
+	uint16_t rx_ring_size;
+	uint16_t tx_ring_size;
+	uint16_t nb_rx_queues;
+	uint16_t nb_tx_queues;
+};
+
 /**
  * Ethernet device information
  */
@@ -1029,6 +1041,9 @@  struct rte_eth_dev_info {
 	/** Configured number of rx/tx queues */
 	uint16_t nb_rx_queues; /**< Number of RX queues. */
 	uint16_t nb_tx_queues; /**< Number of TX queues. */
+
+	/** Queue size recommendations */
+	struct rte_eth_dev_pref_queue_info preferred_queue_values;
 };
 
 /**