[RFC PATCH v1 0/4] ethdev: add per-PMD tuning of RxTx parmeters

[RFC PATCH v1 0/4] ethdev: add per-PMD tuning of RxTx parmeters

[Remy Horton]

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 patchset allows individual
PMDs to specify their preferred parameter values, and if so indicated by an
application, for them to be used automatically by the ethdev layer.

This RFC/V1 includes per-PMD values for e1000 and i40e but it is expected
that subsequent patchsets will cover other PMDs. A deprecation notice
covering the API/ABI change is in place.


Remy Horton (4):
  ethdev: add support for PMD-tuned Tx/Rx parameters
  net/e1000: add TxRx tuning parameters
  net/i40e: add TxRx tuning parameters
  testpmd: make use of per-PMD TxRx parameters

 app/test-pmd/testpmd.c         |  5 +++--
 drivers/net/e1000/em_ethdev.c  |  8 ++++++++
 drivers/net/i40e/i40e_ethdev.c | 35 ++++++++++++++++++++++++++++++++---
 lib/librte_ether/rte_ethdev.c  | 18 ++++++++++++++++++
 lib/librte_ether/rte_ethdev.h  | 15 +++++++++++++++
 5 files changed, 76 insertions(+), 5 deletions(-)

-- 
2.9.5

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

[Remy Horton]

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 */
+	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;
 };
 
 /**
-- 
2.9.5

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

[Shreyansh Jain]

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

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

[Remy Horton]

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

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

[Ferruh Yigit]

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?

>  };
>  
>  /**
> 

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

[Shreyansh Jain]

> -----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?
 
> 
> >  };
> >
> >  /**
> >

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

[Remy Horton]

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.

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

[Remy Horton]

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.

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

[Ferruh Yigit]

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.

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

[Shreyansh Jain]

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

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

[Ferruh Yigit]

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.

> 

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

[Ananyev, Konstantin]

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

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

[Ferruh Yigit]

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

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

[Bruce Richardson]

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

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

[Ferruh Yigit]

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
> 

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

[Bruce Richardson]

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

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

[Ferruh Yigit]

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
> 

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

[Shreyansh Jain]

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.

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

[Bruce Richardson]

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.

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

[Remy Horton]

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

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

[Ferruh Yigit]

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

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

[Remy Horton]

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.

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

[Ferruh Yigit]

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.

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

[Ferruh Yigit]

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

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

[Shreyansh Jain]

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

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

[Ferruh Yigit]

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
> 

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

[Shreyansh Jain]

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

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

[Ananyev, Konstantin]

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

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

[Ferruh Yigit]

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
> 

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

[Bruce Richardson]

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

[RFC PATCH v1 2/4] net/e1000: add TxRx tuning parameters

[Remy Horton]

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>
---
 drivers/net/e1000/em_ethdev.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
index 242375f..e81abd1 100644
--- a/drivers/net/e1000/em_ethdev.c
+++ b/drivers/net/e1000/em_ethdev.c
@@ -1099,6 +1099,8 @@ static void
 eth_em_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 {
 	struct e1000_hw *hw = E1000_DEV_PRIVATE_TO_HW(dev->data->dev_private);
+	struct rte_eth_dev_pref_queue_info *pref_q_info =
+		&dev_info->preferred_queue_values;
 
 	dev_info->pci_dev = RTE_ETH_DEV_TO_PCI(dev);
 	dev_info->min_rx_bufsize = 256; /* See BSIZE field of RCTL register. */
@@ -1152,6 +1154,12 @@ eth_em_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 	dev_info->speed_capa = ETH_LINK_SPEED_10M_HD | ETH_LINK_SPEED_10M |
 			ETH_LINK_SPEED_100M_HD | ETH_LINK_SPEED_100M |
 			ETH_LINK_SPEED_1G;
+
+	/* Preferred queue parameters */
+	pref_q_info->nb_tx_queues = 1;
+	pref_q_info->nb_rx_queues = 1;
+	pref_q_info->tx_ring_size = 256;
+	pref_q_info->rx_ring_size = 256;
 }
 
 /* return 0 means link status changed, -1 means not changed */
-- 
2.9.5

[RFC PATCH v1 3/4] net/i40e: add TxRx tuning parameters

[Remy Horton]

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>
---
 drivers/net/i40e/i40e_ethdev.c | 35 ++++++++++++++++++++++++++++++++---
 1 file changed, 32 insertions(+), 3 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 508b417..4bcd05e 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -3168,6 +3168,7 @@ i40e_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 	struct i40e_hw *hw = I40E_DEV_PRIVATE_TO_HW(dev->data->dev_private);
 	struct i40e_vsi *vsi = pf->main_vsi;
 	struct rte_pci_device *pci_dev = RTE_ETH_DEV_TO_PCI(dev);
+	struct rte_eth_dev_pref_queue_info *pref_q_info;
 
 	dev_info->pci_dev = pci_dev;
 	dev_info->max_rx_queues = vsi->nb_qps;
@@ -3248,15 +3249,43 @@ i40e_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 		dev_info->max_tx_queues += dev_info->vmdq_queue_num;
 	}
 
-	if (I40E_PHY_TYPE_SUPPORT_40G(hw->phy.phy_types))
+	pref_q_info = &dev_info->preferred_queue_values;
+	if (I40E_PHY_TYPE_SUPPORT_40G(hw->phy.phy_types)) {
 		/* For XL710 */
 		dev_info->speed_capa = ETH_LINK_SPEED_40G;
-	else if (I40E_PHY_TYPE_SUPPORT_25G(hw->phy.phy_types))
+		pref_q_info->nb_tx_queues = 2;
+		pref_q_info->nb_rx_queues = 2;
+		if (dev->data->nb_rx_queues == 1)
+			pref_q_info->rx_ring_size = 2048;
+		else
+			pref_q_info->rx_ring_size = 1024;
+		if (dev->data->nb_tx_queues == 1)
+			pref_q_info->tx_ring_size = 1024;
+		else
+			pref_q_info->tx_ring_size = 512;
+
+	} else if (I40E_PHY_TYPE_SUPPORT_25G(hw->phy.phy_types)) {
 		/* For XXV710 */
 		dev_info->speed_capa = ETH_LINK_SPEED_25G;
-	else
+		pref_q_info->nb_tx_queues = 1;
+		pref_q_info->nb_rx_queues = 1;
+		pref_q_info->rx_ring_size = 256;
+		pref_q_info->tx_ring_size = 256;
+	} else {
 		/* For X710 */
 		dev_info->speed_capa = ETH_LINK_SPEED_1G | ETH_LINK_SPEED_10G;
+		pref_q_info->nb_tx_queues = 1;
+		pref_q_info->nb_rx_queues = 1;
+		if (dev->data->dev_conf.link_speeds & ETH_LINK_SPEED_10G) {
+			pref_q_info->rx_ring_size = 512;
+			pref_q_info->tx_ring_size = 256;
+		} else {
+			pref_q_info->rx_ring_size = 256;
+			pref_q_info->tx_ring_size = 256;
+		}
+	}
+	pref_q_info->tx_burst_size = 32;
+	pref_q_info->rx_burst_size = 32;
 }
 
 static int
-- 
2.9.5

[RFC PATCH v1 4/4] testpmd: make use of per-PMD TxRx parameters

[Remy Horton]

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 testpmd to make use of per-PMD tuned parameter values.

Signed-off-by: Remy Horton <remy.horton@intel.com>
---
 app/test-pmd/testpmd.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 4c0e258..82eb197 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -210,9 +210,10 @@ queueid_t nb_txq = 1; /**< Number of TX queues per port. */
 
 /*
  * Configurable number of RX/TX ring descriptors.
+ * Defaults are supplied by drivers via ethdev.
  */
-#define RTE_TEST_RX_DESC_DEFAULT 1024
-#define RTE_TEST_TX_DESC_DEFAULT 1024
+#define RTE_TEST_RX_DESC_DEFAULT 0
+#define RTE_TEST_TX_DESC_DEFAULT 0
 uint16_t nb_rxd = RTE_TEST_RX_DESC_DEFAULT; /**< Number of RX descriptors. */
 uint16_t nb_txd = RTE_TEST_TX_DESC_DEFAULT; /**< Number of TX descriptors. */
 
-- 
2.9.5

[PATCH v2 0/4] ethdev: add per-PMD tuning of RxTx parmeters

[Remy Horton]

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 patchset allows individual
PMDs to specify their preferred parameter values, and if so indicated by an
application, for them to be used automatically by the ethdev layer.

rte_eth_dev_configure() has been changed so that specifying zero for both
nb_rx_q AND nb_tx_q causes it to use driver preferred values, and if these
are not available, falls back to EAL defaults. Setting one (but not both)
to zero does not cause the use of defaults, as having one of them zeroed is
a valid setup.

This RFC/V1 includes per-PMD values for e1000 and i40e but it is expected
that subsequent patchsets will cover other PMDs. A deprecation notice
covering the API/ABI change is in place.


Changes in v2:
* Rebased to 
* Removed fallback values from rte_eth_dev_info_get()
* Added fallback values to rte_rte_[rt]x_queue_setup()
* Added fallback values to rte_eth_dev_configure()
* Corrected comment
* Removed deprecation notice
* Split RX and Tx into seperate structures
* Changed parameter names


Remy Horton (4):
  ethdev: add support for PMD-tuned Tx/Rx parameters
  net/e1000: add TxRx tuning parameters
  net/i40e: add TxRx tuning parameters
  testpmd: make use of per-PMD TxRx parameters

 app/test-pmd/testpmd.c               |  5 ++--
 doc/guides/rel_notes/deprecation.rst | 13 -----------
 drivers/net/e1000/em_ethdev.c        |  6 +++++
 drivers/net/i40e/i40e_ethdev.c       | 33 ++++++++++++++++++++++++---
 lib/librte_ether/rte_ethdev.c        | 44 ++++++++++++++++++++++++++++--------
 lib/librte_ether/rte_ethdev.h        | 23 +++++++++++++++++++
 6 files changed, 97 insertions(+), 27 deletions(-)

-- 
2.9.5

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

[Remy Horton]

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>
---
 doc/guides/rel_notes/deprecation.rst | 13 -----------
 lib/librte_ether/rte_ethdev.c        | 44 ++++++++++++++++++++++++++++--------
 lib/librte_ether/rte_ethdev.h        | 23 +++++++++++++++++++
 3 files changed, 58 insertions(+), 22 deletions(-)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 74c18ed..803038b 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -115,19 +115,6 @@ Deprecation Notices
   The new API add rss_level field to ``rte_eth_rss_conf`` to enable a choice
   of RSS hash calculation on outer or inner header of tunneled packet.
 
-* ethdev:  Currently, if the  rte_eth_rx_burst() function returns a value less
-  than *nb_pkts*, the application will assume that no more packets are present.
-  Some of the hw queue based hardware can only support smaller burst for RX
-  and TX and thus break the expectation of the rx_burst API. Similar is the
-  case for TX burst as well as ring sizes. ``rte_eth_dev_info`` will be added
-  with following new parameters so as to support semantics for drivers to
-  define a preferred size for Rx/Tx burst and rings.
-
-  - Member ``struct preferred_size`` would be added to enclose all preferred
-    size to be fetched from driver/implementation.
-  - Members ``uint16_t rx_burst``,  ``uint16_t tx_burst``, ``uint16_t rx_ring``,
-    and ``uint16_t tx_ring`` would be added to ``struct preferred_size``.
-
 * ethdev: A work is being planned for 18.05 to expose VF port representors
   as a mean to perform control and data path operation on the different VFs.
   As VF representor is an ethdev port, new fields are needed in order to map
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 0590f0c..e5fba7c 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1045,6 +1045,26 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
+	dev = &rte_eth_devices[port_id];
+
+	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
+	(*dev->dev_ops->dev_infos_get)(dev, &dev_info);
+
+	/* If number of queues specified by application for both Rx and Tx is
+	 * zero, use driver preferred values. This cannot be done individually
+	 * as it is valid for either Tx or Rx (but not both) to be zero.
+	 * If driver does not provide any preferred valued, fall back on
+	 * EAL defaults.
+	 */
+	if (nb_rx_q == 0 && nb_tx_q == 0) {
+		nb_rx_q = dev_info.default_rxportconf.nb_queues;
+		if (nb_rx_q == 0)
+			nb_rx_q = RTE_ETH_DEV_FALLBACK_RX_NBQUEUES;
+		nb_tx_q = dev_info.default_txportconf.nb_queues;
+		if (nb_tx_q == 0)
+			nb_tx_q = RTE_ETH_DEV_FALLBACK_TX_NBQUEUES;
+	}
+
 	if (nb_rx_q > RTE_MAX_QUEUES_PER_PORT) {
 		RTE_PMD_DEBUG_TRACE(
 			"Number of RX queues requested (%u) is greater than max supported(%d)\n",
@@ -1059,8 +1079,6 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 		return -EINVAL;
 	}
 
-	dev = &rte_eth_devices[port_id];
-
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -ENOTSUP);
 
@@ -1090,13 +1108,6 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 	 * than the maximum number of RX and TX queues supported by the
 	 * configured device.
 	 */
-	(*dev->dev_ops->dev_infos_get)(dev, &dev_info);
-
-	if (nb_rx_q == 0 && nb_tx_q == 0) {
-		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d both rx and tx queue cannot be 0\n", port_id);
-		return -EINVAL;
-	}
-
 	if (nb_rx_q > dev_info.max_rx_queues) {
 		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d nb_rx_queues=%d > %d\n",
 				port_id, nb_rx_q, dev_info.max_rx_queues);
@@ -1461,6 +1472,14 @@ rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
 		return -EINVAL;
 	}
 
+	/* Use default specified by driver, if nb_rx_desc is zero */
+	if (nb_rx_desc == 0) {
+		nb_rx_desc = dev_info.default_rxportconf.ring_size;
+		/* If driver default is also zero, fall back on EAL default */
+		if (nb_rx_desc == 0)
+			nb_rx_desc = RTE_ETH_DEV_FALLBACK_RX_RINGSIZE;
+	}
+
 	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 +1603,13 @@ 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.default_txportconf.ring_size;
+		/* If driver default is zero, fall back on EAL default */
+		if (nb_tx_desc == 0)
+			nb_tx_desc = RTE_ETH_DEV_FALLBACK_TX_RINGSIZE;
+	}
 	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) {
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 0361533..72d138d 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -988,6 +988,25 @@ struct rte_eth_conf {
 
 struct rte_pci_device;
 
+/*
+ * Fallback default preferred Rx/Tx port parameters.
+ * These are used if an application requests default parameters
+ * but the PMD does not provide preferred values.
+ */
+#define RTE_ETH_DEV_FALLBACK_RX_RINGSIZE 512
+#define RTE_ETH_DEV_FALLBACK_TX_RINGSIZE 512
+#define RTE_ETH_DEV_FALLBACK_RX_NBQUEUES 1
+#define RTE_ETH_DEV_FALLBACK_TX_NBQUEUES 1
+
+/*
+ * Preferred Rx/Tx port parameters.
+ */
+struct rte_eth_dev_portconf {
+	uint16_t burst_size;
+	uint16_t ring_size;
+	uint16_t nb_queues;
+};
+
 /**
  * Ethernet device information
  */
@@ -1029,6 +1048,10 @@ 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. */
+
+	/** Tx/Rx parameter recommendations */
+	struct rte_eth_dev_portconf default_rxportconf;
+	struct rte_eth_dev_portconf default_txportconf;
 };
 
 /**
-- 
2.9.5

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

[Shreyansh Jain]

> -----Original Message-----
> From: Remy Horton [mailto:remy.horton@intel.com]
> Sent: Wednesday, March 21, 2018 7:58 PM
> To: dev@dpdk.org
> Cc: John McNamara <john.mcnamara@intel.com>; 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: [PATCH v2 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>

Acked-By: Shreyansh Jain <shreyansh.jain@nxp.com>

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

[Thomas Monjalon]

I know that this patch is already applied,
but I have some comments below.
Please try to address them.

21/03/2018 15:27, Remy Horton:
> +	/* If number of queues specified by application for both Rx and Tx is
> +	 * zero, use driver preferred values. This cannot be done individually
> +	 * as it is valid for either Tx or Rx (but not both) to be zero.
> +	 * If driver does not provide any preferred valued, fall back on
> +	 * EAL defaults.
> +	 */

The fallback could be set in rte_eth_dev_info_get() and call this function
instead of calling directly dev_ops->dev_infos_get.
It would require to never return -ENOTSUP.

At least, some default values should be set in rte_eth_dev_info_get().

> @@ -1029,6 +1048,10 @@ 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. */
> +
> +	/** Tx/Rx parameter recommendations */
> +	struct rte_eth_dev_portconf default_rxportconf;
> +	struct rte_eth_dev_portconf default_txportconf;

Some formatting comments:
Usually, there is no blank line in structs.
The doxygen comment must apply to each field.
Have you checked what is the doxygen output?

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

[Thomas Monjalon]

30/03/2018 17:40, Thomas Monjalon:
> I know that this patch is already applied,
> but I have some comments below.
> Please try to address them.
> 
> 21/03/2018 15:27, Remy Horton:
> > +	/* If number of queues specified by application for both Rx and Tx is
> > +	 * zero, use driver preferred values. This cannot be done individually
> > +	 * as it is valid for either Tx or Rx (but not both) to be zero.
> > +	 * If driver does not provide any preferred valued, fall back on
> > +	 * EAL defaults.
> > +	 */
> 
> The fallback could be set in rte_eth_dev_info_get() and call this function
> instead of calling directly dev_ops->dev_infos_get.
> It would require to never return -ENOTSUP.
> 
> At least, some default values should be set in rte_eth_dev_info_get().

I see it has been discussed in v1.
And it was decided to not have ethdev defaults in rte_eth_dev_info_get.

> > @@ -1029,6 +1048,10 @@ 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. */
> > +
> > +	/** Tx/Rx parameter recommendations */
> > +	struct rte_eth_dev_portconf default_rxportconf;
> > +	struct rte_eth_dev_portconf default_txportconf;
> 
> Some formatting comments:
> Usually, there is no blank line in structs.
> The doxygen comment must apply to each field.
> Have you checked what is the doxygen output?

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

[Thomas Monjalon]

21/03/2018 15:27, Remy Horton:
> +/*
> + * Preferred Rx/Tx port parameters.
> + */
> +struct rte_eth_dev_portconf {
> +       uint16_t burst_size;
> +       uint16_t ring_size;
> +       uint16_t nb_queues;
> +};

Please add a doxygen comment for each field, thanks.

[PATCH v2 2/4] net/e1000: add TxRx tuning parameters

[Remy Horton]

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>
---
 drivers/net/e1000/em_ethdev.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
index 242375f..c715065 100644
--- a/drivers/net/e1000/em_ethdev.c
+++ b/drivers/net/e1000/em_ethdev.c
@@ -1152,6 +1152,12 @@ eth_em_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 	dev_info->speed_capa = ETH_LINK_SPEED_10M_HD | ETH_LINK_SPEED_10M |
 			ETH_LINK_SPEED_100M_HD | ETH_LINK_SPEED_100M |
 			ETH_LINK_SPEED_1G;
+
+	/* Preferred queue parameters */
+	dev_info->default_rxportconf.nb_queues = 1;
+	dev_info->default_txportconf.nb_queues = 1;
+	dev_info->default_txportconf.ring_size = 256;
+	dev_info->default_rxportconf.ring_size = 256;
 }
 
 /* return 0 means link status changed, -1 means not changed */
-- 
2.9.5

[PATCH v2 3/4] net/i40e: add TxRx tuning parameters

[Remy Horton]

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>
---
 drivers/net/i40e/i40e_ethdev.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index 508b417..7be00ad 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -3248,15 +3248,42 @@ i40e_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 		dev_info->max_tx_queues += dev_info->vmdq_queue_num;
 	}
 
-	if (I40E_PHY_TYPE_SUPPORT_40G(hw->phy.phy_types))
+	if (I40E_PHY_TYPE_SUPPORT_40G(hw->phy.phy_types)) {
 		/* For XL710 */
 		dev_info->speed_capa = ETH_LINK_SPEED_40G;
-	else if (I40E_PHY_TYPE_SUPPORT_25G(hw->phy.phy_types))
+		dev_info->default_rxportconf.nb_queues = 2;
+		dev_info->default_txportconf.nb_queues = 2;
+		if (dev->data->nb_rx_queues == 1)
+			dev_info->default_rxportconf.ring_size = 2048;
+		else
+			dev_info->default_rxportconf.ring_size = 1024;
+		if (dev->data->nb_tx_queues == 1)
+			dev_info->default_txportconf.ring_size = 1024;
+		else
+			dev_info->default_txportconf.ring_size = 512;
+
+	} else if (I40E_PHY_TYPE_SUPPORT_25G(hw->phy.phy_types)) {
 		/* For XXV710 */
 		dev_info->speed_capa = ETH_LINK_SPEED_25G;
-	else
+		dev_info->default_rxportconf.nb_queues = 1;
+		dev_info->default_txportconf.nb_queues = 1;
+		dev_info->default_rxportconf.ring_size = 256;
+		dev_info->default_txportconf.ring_size = 256;
+	} else {
 		/* For X710 */
 		dev_info->speed_capa = ETH_LINK_SPEED_1G | ETH_LINK_SPEED_10G;
+		dev_info->default_rxportconf.nb_queues = 1;
+		dev_info->default_txportconf.nb_queues = 1;
+		if (dev->data->dev_conf.link_speeds & ETH_LINK_SPEED_10G) {
+			dev_info->default_rxportconf.ring_size = 512;
+			dev_info->default_txportconf.ring_size = 256;
+		} else {
+			dev_info->default_rxportconf.ring_size = 256;
+			dev_info->default_txportconf.ring_size = 256;
+		}
+	}
+	dev_info->default_rxportconf.burst_size = 32;
+	dev_info->default_txportconf.burst_size = 32;
 }
 
 static int
-- 
2.9.5

[PATCH v2 4/4] testpmd: make use of per-PMD TxRx parameters

[Remy Horton]

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 testpmd to make use of per-PMD tuned parameter values.

Signed-off-by: Remy Horton <remy.horton@intel.com>
---
 app/test-pmd/testpmd.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 4c0e258..82eb197 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -210,9 +210,10 @@ queueid_t nb_txq = 1; /**< Number of TX queues per port. */
 
 /*
  * Configurable number of RX/TX ring descriptors.
+ * Defaults are supplied by drivers via ethdev.
  */
-#define RTE_TEST_RX_DESC_DEFAULT 1024
-#define RTE_TEST_TX_DESC_DEFAULT 1024
+#define RTE_TEST_RX_DESC_DEFAULT 0
+#define RTE_TEST_TX_DESC_DEFAULT 0
 uint16_t nb_rxd = RTE_TEST_RX_DESC_DEFAULT; /**< Number of RX descriptors. */
 uint16_t nb_txd = RTE_TEST_TX_DESC_DEFAULT; /**< Number of TX descriptors. */
 
-- 
2.9.5

Re: [PATCH v2 4/4] testpmd: make use of per-PMD TxRx parameters

[Shreyansh Jain]

Hello Remy,

> -----Original Message-----
> From: Remy Horton [mailto:remy.horton@intel.com]
> Sent: Wednesday, March 21, 2018 7:58 PM
> To: dev@dpdk.org
> Cc: John McNamara <john.mcnamara@intel.com>; 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: [PATCH v2 4/4] testpmd: make use of per-PMD TxRx 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 testpmd to make use of per-PMD tuned parameter values.
> 
> Signed-off-by: Remy Horton <remy.horton@intel.com>
> ---
>  app/test-pmd/testpmd.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 4c0e258..82eb197 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -210,9 +210,10 @@ queueid_t nb_txq = 1; /**< Number of TX queues per
> port. */
> 
>  /*
>   * Configurable number of RX/TX ring descriptors.
> + * Defaults are supplied by drivers via ethdev.
>   */
> -#define RTE_TEST_RX_DESC_DEFAULT 1024
> -#define RTE_TEST_TX_DESC_DEFAULT 1024
> +#define RTE_TEST_RX_DESC_DEFAULT 0
> +#define RTE_TEST_TX_DESC_DEFAULT 0
>  uint16_t nb_rxd = RTE_TEST_RX_DESC_DEFAULT; /**< Number of RX
> descriptors. */
>  uint16_t nb_txd = RTE_TEST_TX_DESC_DEFAULT; /**< Number of TX
> descriptors. */

Can the change for burst size too be accommodated in this patch?
I looked through them and they might not be as trivial as the change above - but if that is incorporated in this, it might serve as example for other applications.

Or, I am also OK sending a separate patch for that change (maybe for iofwd case, at least)

Either way:

Acked-by: Shreyansh Jain <shreyansh.jain@nxp.com>

Re: [PATCH v2 4/4] testpmd: make use of per-PMD TxRx parameters

[Remy Horton]

On 28/03/2018 08:18, Shreyansh Jain wrote:
[..]
> Can the change for burst size too be accommodated in this patch? I
> looked through them and they might not be as trivial as the change
> above - but if that is incorporated in this, it might serve as
> example for other applications.

Testpmd doesn't really distinguish between Rx and TX burst sizes, having 
just a single user-specifiable nb_pkt_per_burst parameter. However 
filling it with the preferred Rx burst size seems a good balance between 
providing a usage example and amount of code changes required.

Re: [PATCH v2 4/4] testpmd: make use of per-PMD TxRx parameters

[Thomas Monjalon]

21/03/2018 15:27, Remy Horton:
>  /*
>   * Configurable number of RX/TX ring descriptors.

Configurable, really?

> + * Defaults are supplied by drivers via ethdev.

And fallback values are in ethdev.

>   */
> -#define RTE_TEST_RX_DESC_DEFAULT 1024
> -#define RTE_TEST_TX_DESC_DEFAULT 1024
> +#define RTE_TEST_RX_DESC_DEFAULT 0
> +#define RTE_TEST_TX_DESC_DEFAULT 0

We do not need a define for 0.
Better to rework a bit above and below comments.

>  uint16_t nb_rxd = RTE_TEST_RX_DESC_DEFAULT; /**< Number of RX descriptors. */
>  uint16_t nb_txd = RTE_TEST_TX_DESC_DEFAULT; /**< Number of TX descriptors. */

These doxygen comments in the middle of the code are totally useless.

Re: [PATCH v2 4/4] testpmd: make use of per-PMD TxRx parameters

[Remy Horton]

On 31/03/2018 01:01, Thomas Monjalon wrote:
[..]
>>  uint16_t nb_rxd = RTE_TEST_RX_DESC_DEFAULT; /**< Number of RX descriptors. */
>>  uint16_t nb_txd = RTE_TEST_TX_DESC_DEFAULT; /**< Number of TX descriptors. */
>
> These doxygen comments in the middle of the code are totally useless.

Did wonder why they were there. However these lines are existing code, 
and since testpmd.c uses Doxygen tags extensively, removing them in my 
view ought to be done via a separate clean-up patch.

Re: [PATCH v2 0/4] ethdev: add per-PMD tuning of RxTx parmeters

[Ferruh Yigit]

On 3/21/2018 2:27 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 patchset allows individual
> PMDs to specify their preferred parameter values, and if so indicated by an
> application, for them to be used automatically by the ethdev layer.
> 
> rte_eth_dev_configure() has been changed so that specifying zero for both
> nb_rx_q AND nb_tx_q causes it to use driver preferred values, and if these
> are not available, falls back to EAL defaults. Setting one (but not both)
> to zero does not cause the use of defaults, as having one of them zeroed is
> a valid setup.
> 
> This RFC/V1 includes per-PMD values for e1000 and i40e but it is expected
> that subsequent patchsets will cover other PMDs. A deprecation notice
> covering the API/ABI change is in place.
> 
> 
> Changes in v2:
> * Rebased to 
> * Removed fallback values from rte_eth_dev_info_get()
> * Added fallback values to rte_rte_[rt]x_queue_setup()
> * Added fallback values to rte_eth_dev_configure()
> * Corrected comment
> * Removed deprecation notice
> * Split RX and Tx into seperate structures
> * Changed parameter names
> 
> 
> Remy Horton (4):
>   ethdev: add support for PMD-tuned Tx/Rx parameters
>   net/e1000: add TxRx tuning parameters
>   net/i40e: add TxRx tuning parameters
>   testpmd: make use of per-PMD TxRx parameters

Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Re: [PATCH v2 0/4] ethdev: add per-PMD tuning of RxTx parmeters

[Ferruh Yigit]

On 3/27/2018 7:43 PM, Ferruh Yigit wrote:
> On 3/21/2018 2:27 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 patchset allows individual
>> PMDs to specify their preferred parameter values, and if so indicated by an
>> application, for them to be used automatically by the ethdev layer.
>>
>> rte_eth_dev_configure() has been changed so that specifying zero for both
>> nb_rx_q AND nb_tx_q causes it to use driver preferred values, and if these
>> are not available, falls back to EAL defaults. Setting one (but not both)
>> to zero does not cause the use of defaults, as having one of them zeroed is
>> a valid setup.
>>
>> This RFC/V1 includes per-PMD values for e1000 and i40e but it is expected
>> that subsequent patchsets will cover other PMDs. A deprecation notice
>> covering the API/ABI change is in place.
>>
>>
>> Changes in v2:
>> * Rebased to 
>> * Removed fallback values from rte_eth_dev_info_get()
>> * Added fallback values to rte_rte_[rt]x_queue_setup()
>> * Added fallback values to rte_eth_dev_configure()
>> * Corrected comment
>> * Removed deprecation notice
>> * Split RX and Tx into seperate structures
>> * Changed parameter names
>>
>>
>> Remy Horton (4):
>>   ethdev: add support for PMD-tuned Tx/Rx parameters
>>   net/e1000: add TxRx tuning parameters
>>   net/i40e: add TxRx tuning parameters
>>   testpmd: make use of per-PMD TxRx parameters
> 
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

Series applied to dpdk-next-net/master, thanks.

Re: [PATCH v2 0/4] ethdev: add per-PMD tuning of RxTx parmeters

[Thomas Monjalon]

30/03/2018 12:34, Ferruh Yigit:
> On 3/27/2018 7:43 PM, Ferruh Yigit wrote:
> > On 3/21/2018 2:27 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 patchset allows individual
> >> PMDs to specify their preferred parameter values, and if so indicated by an
> >> application, for them to be used automatically by the ethdev layer.
> >>
> >> rte_eth_dev_configure() has been changed so that specifying zero for both
> >> nb_rx_q AND nb_tx_q causes it to use driver preferred values, and if these
> >> are not available, falls back to EAL defaults. Setting one (but not both)
> >> to zero does not cause the use of defaults, as having one of them zeroed is
> >> a valid setup.
> >>
> >> This RFC/V1 includes per-PMD values for e1000 and i40e but it is expected
> >> that subsequent patchsets will cover other PMDs. A deprecation notice
> >> covering the API/ABI change is in place.
> >>
> >>
> >> Changes in v2:
> >> * Rebased to 
> >> * Removed fallback values from rte_eth_dev_info_get()
> >> * Added fallback values to rte_rte_[rt]x_queue_setup()
> >> * Added fallback values to rte_eth_dev_configure()
> >> * Corrected comment
> >> * Removed deprecation notice
> >> * Split RX and Tx into seperate structures
> >> * Changed parameter names
> >>
> >>
> >> Remy Horton (4):
> >>   ethdev: add support for PMD-tuned Tx/Rx parameters
> >>   net/e1000: add TxRx tuning parameters
> >>   net/i40e: add TxRx tuning parameters
> >>   testpmd: make use of per-PMD TxRx parameters
> > 
> > Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
> 
> Series applied to dpdk-next-net/master, thanks.

I prefer not pulling this series in master and give a chance to have
a more complete v3 for testpmd and examples.

[PATCH v3 0/4] ethdev: add per-PMD tuning of RxTx parmeters

[Remy Horton]

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 patchset allows individual
PMDs to specify their preferred parameter values, and if so indicated by an
application, for them to be used automatically by the ethdev layer.

rte_eth_dev_configure() has been changed so that specifying zero for both
nb_rx_q AND nb_tx_q causes it to use driver preferred values, and if these
are not available, falls back to EAL defaults. Setting one (but not both)
to zero does not cause the use of defaults, as having one of them zeroed is
a valid setup.

This patchset includes per-PMD values for e1000 and i40e but it is expected
that subsequent patchsets will cover other PMDs. A deprecation notice
covering the API/ABI change is in place.

Changes in v3:
* Changed formatting around new rte_eth_dev_info fields
* Added Doxygen documentation to struct rte_eth_dev_portconf
* Testpmd "port config all burst 0" and --burst=0 uses PMD 
  Rx burst recommendations.
* Added to release notes
* Rebased to 8ea081f38161

Changes in v2:
* Rebased to master
* Removed fallback values from rte_eth_dev_info_get()
* Added fallback values to rte_rte_[rt]x_queue_setup()
* Added fallback values to rte_eth_dev_configure()
* Corrected comment
* Removed deprecation notice
* Split RX and Tx into seperate structures
* Changed parameter names


Remy Horton (4):
  ethdev: add support for PMD-tuned Tx/Rx parameters
  net/e1000: add TxRx tuning parameters
  net/i40e: add TxRx tuning parameters
  testpmd: make use of per-PMD TxRx parameters

 app/test-pmd/cmdline.c                 | 31 +++++++++++++++++++++---
 app/test-pmd/parameters.c              | 38 +++++++++++++++++++++++++----
 app/test-pmd/testpmd.c                 |  5 ++--
 doc/guides/rel_notes/deprecation.rst   | 13 ----------
 doc/guides/rel_notes/release_18_05.rst |  5 ++++
 drivers/net/e1000/em_ethdev.c          |  6 +++++
 drivers/net/i40e/i40e_ethdev.c         | 33 ++++++++++++++++++++++---
 lib/librte_ether/rte_ethdev.c          | 44 +++++++++++++++++++++++++++-------
 lib/librte_ether/rte_ethdev.h          | 25 +++++++++++++++++++
 9 files changed, 165 insertions(+), 35 deletions(-)

-- 
2.9.5

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

[Remy Horton]

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>
---
 doc/guides/rel_notes/deprecation.rst   | 13 ----------
 doc/guides/rel_notes/release_18_05.rst |  5 ++++
 lib/librte_ether/rte_ethdev.c          | 44 +++++++++++++++++++++++++++-------
 lib/librte_ether/rte_ethdev.h          | 25 +++++++++++++++++++
 4 files changed, 65 insertions(+), 22 deletions(-)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index 0c696f7..920df6b 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -115,19 +115,6 @@ Deprecation Notices
   The new API add rss_level field to ``rte_eth_rss_conf`` to enable a choice
   of RSS hash calculation on outer or inner header of tunneled packet.
 
-* ethdev:  Currently, if the  rte_eth_rx_burst() function returns a value less
-  than *nb_pkts*, the application will assume that no more packets are present.
-  Some of the hw queue based hardware can only support smaller burst for RX
-  and TX and thus break the expectation of the rx_burst API. Similar is the
-  case for TX burst as well as ring sizes. ``rte_eth_dev_info`` will be added
-  with following new parameters so as to support semantics for drivers to
-  define a preferred size for Rx/Tx burst and rings.
-
-  - Member ``struct preferred_size`` would be added to enclose all preferred
-    size to be fetched from driver/implementation.
-  - Members ``uint16_t rx_burst``,  ``uint16_t tx_burst``, ``uint16_t rx_ring``,
-    and ``uint16_t tx_ring`` would be added to ``struct preferred_size``.
-
 * ethdev: A work is being planned for 18.05 to expose VF port representors
   as a mean to perform control and data path operation on the different VFs.
   As VF representor is an ethdev port, new fields are needed in order to map
diff --git a/doc/guides/rel_notes/release_18_05.rst b/doc/guides/rel_notes/release_18_05.rst
index 9cc77f8..129e874 100644
--- a/doc/guides/rel_notes/release_18_05.rst
+++ b/doc/guides/rel_notes/release_18_05.rst
@@ -58,6 +58,11 @@ New Features
   * Added support for NVGRE, VXLAN and GENEVE filters in flow API.
   * Added support for DROP action in flow API.
 
+* **Added PMD-recommended Tx and Rx parameters**
+
+  Applications can now query drivers for device-tuned values of
+  ring sizes, burst sizes, and number of queues.
+
 
 API Changes
 -----------
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 2c74f7e..209796d 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1061,6 +1061,26 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
+	dev = &rte_eth_devices[port_id];
+
+	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
+	(*dev->dev_ops->dev_infos_get)(dev, &dev_info);
+
+	/* If number of queues specified by application for both Rx and Tx is
+	 * zero, use driver preferred values. This cannot be done individually
+	 * as it is valid for either Tx or Rx (but not both) to be zero.
+	 * If driver does not provide any preferred valued, fall back on
+	 * EAL defaults.
+	 */
+	if (nb_rx_q == 0 && nb_tx_q == 0) {
+		nb_rx_q = dev_info.default_rxportconf.nb_queues;
+		if (nb_rx_q == 0)
+			nb_rx_q = RTE_ETH_DEV_FALLBACK_RX_NBQUEUES;
+		nb_tx_q = dev_info.default_txportconf.nb_queues;
+		if (nb_tx_q == 0)
+			nb_tx_q = RTE_ETH_DEV_FALLBACK_TX_NBQUEUES;
+	}
+
 	if (nb_rx_q > RTE_MAX_QUEUES_PER_PORT) {
 		RTE_PMD_DEBUG_TRACE(
 			"Number of RX queues requested (%u) is greater than max supported(%d)\n",
@@ -1075,8 +1095,6 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 		return -EINVAL;
 	}
 
-	dev = &rte_eth_devices[port_id];
-
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -ENOTSUP);
 
@@ -1106,13 +1124,6 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 	 * than the maximum number of RX and TX queues supported by the
 	 * configured device.
 	 */
-	(*dev->dev_ops->dev_infos_get)(dev, &dev_info);
-
-	if (nb_rx_q == 0 && nb_tx_q == 0) {
-		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d both rx and tx queue cannot be 0\n", port_id);
-		return -EINVAL;
-	}
-
 	if (nb_rx_q > dev_info.max_rx_queues) {
 		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d nb_rx_queues=%d > %d\n",
 				port_id, nb_rx_q, dev_info.max_rx_queues);
@@ -1477,6 +1488,14 @@ rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
 		return -EINVAL;
 	}
 
+	/* Use default specified by driver, if nb_rx_desc is zero */
+	if (nb_rx_desc == 0) {
+		nb_rx_desc = dev_info.default_rxportconf.ring_size;
+		/* If driver default is also zero, fall back on EAL default */
+		if (nb_rx_desc == 0)
+			nb_rx_desc = RTE_ETH_DEV_FALLBACK_RX_RINGSIZE;
+	}
+
 	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) {
@@ -1600,6 +1619,13 @@ 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.default_txportconf.ring_size;
+		/* If driver default is zero, fall back on EAL default */
+		if (nb_tx_desc == 0)
+			nb_tx_desc = RTE_ETH_DEV_FALLBACK_TX_RINGSIZE;
+	}
 	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) {
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 5e13dca..685145f 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -988,6 +988,27 @@ struct rte_eth_conf {
 
 struct rte_pci_device;
 
+/*
+ * Fallback default preferred Rx/Tx port parameters.
+ * These are used if an application requests default parameters
+ * but the PMD does not provide preferred values.
+ */
+#define RTE_ETH_DEV_FALLBACK_RX_RINGSIZE 512
+#define RTE_ETH_DEV_FALLBACK_TX_RINGSIZE 512
+#define RTE_ETH_DEV_FALLBACK_RX_NBQUEUES 1
+#define RTE_ETH_DEV_FALLBACK_TX_NBQUEUES 1
+
+/**
+ * Preferred Rx/Tx port parameters.
+ * There are separate instances of this structure for transmission
+ * and reception respectively.
+ */
+struct rte_eth_dev_portconf {
+	uint16_t burst_size; /**< Device-preferred burst size */
+	uint16_t ring_size; /**< Device-preferred size of queue rings */
+	uint16_t nb_queues; /**< Device-preferred number of queues */
+};
+
 /**
  * Ethernet device information
  */
@@ -1029,6 +1050,10 @@ 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. */
+	/** Rx parameter recommendations */
+	struct rte_eth_dev_portconf default_rxportconf;
+	/** Tx parameter recommendations */
+	struct rte_eth_dev_portconf default_txportconf;
 };
 
 /**
-- 
2.9.5

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

[De Lara Guarch, Pablo]

Hi Remy,

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Remy Horton
> Sent: Wednesday, April 4, 2018 6:18 PM
> To: dev@dpdk.org
> Cc: Mcnamara, John <john.mcnamara@intel.com>; 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>; Shreyansh Jain
> <shreyansh.jain@nxp.com>; Thomas Monjalon <thomas@monjalon.net>
> Subject: [dpdk-dev] [PATCH v3 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>
> ---
>  doc/guides/rel_notes/deprecation.rst   | 13 ----------
>  doc/guides/rel_notes/release_18_05.rst |  5 ++++
>  lib/librte_ether/rte_ethdev.c          | 44 +++++++++++++++++++++++++++-------
>  lib/librte_ether/rte_ethdev.h          | 25 +++++++++++++++++++
>  4 files changed, 65 insertions(+), 22 deletions(-)
> 
> diff --git a/doc/guides/rel_notes/deprecation.rst
> b/doc/guides/rel_notes/deprecation.rst
> index 0c696f7..920df6b 100644
> --- a/doc/guides/rel_notes/deprecation.rst
> +++ b/doc/guides/rel_notes/deprecation.rst
> @@ -115,19 +115,6 @@ Deprecation Notices
>    The new API add rss_level field to ``rte_eth_rss_conf`` to enable a choice
>    of RSS hash calculation on outer or inner header of tunneled packet.
> 
> -* ethdev:  Currently, if the  rte_eth_rx_burst() function returns a value less
> -  than *nb_pkts*, the application will assume that no more packets are present.
> -  Some of the hw queue based hardware can only support smaller burst for RX
> -  and TX and thus break the expectation of the rx_burst API. Similar is the
> -  case for TX burst as well as ring sizes. ``rte_eth_dev_info`` will be added
> -  with following new parameters so as to support semantics for drivers to
> -  define a preferred size for Rx/Tx burst and rings.
> -
> -  - Member ``struct preferred_size`` would be added to enclose all preferred
> -    size to be fetched from driver/implementation.
> -  - Members ``uint16_t rx_burst``,  ``uint16_t tx_burst``, ``uint16_t rx_ring``,
> -    and ``uint16_t tx_ring`` would be added to ``struct preferred_size``.
> -
>  * ethdev: A work is being planned for 18.05 to expose VF port representors
>    as a mean to perform control and data path operation on the different VFs.
>    As VF representor is an ethdev port, new fields are needed in order to map diff

API and ABI changes should be documented in release notes.

Thanks,
Pablo

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

[Thomas Monjalon]

04/04/2018 20:56, De Lara Guarch, Pablo:
> 
> API and ABI changes should be documented in release notes.

When sending a v4 for the API change, you can add my ack:

Acked-by: Thomas Monjalon <thomas@monjalon.net>

[PATCH v3 2/4] net/e1000: add TxRx tuning parameters

[Remy Horton]

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>
---
 drivers/net/e1000/em_ethdev.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
index f727382..2035ea8 100644
--- a/drivers/net/e1000/em_ethdev.c
+++ b/drivers/net/e1000/em_ethdev.c
@@ -1100,6 +1100,12 @@ eth_em_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 	dev_info->speed_capa = ETH_LINK_SPEED_10M_HD | ETH_LINK_SPEED_10M |
 			ETH_LINK_SPEED_100M_HD | ETH_LINK_SPEED_100M |
 			ETH_LINK_SPEED_1G;
+
+	/* Preferred queue parameters */
+	dev_info->default_rxportconf.nb_queues = 1;
+	dev_info->default_txportconf.nb_queues = 1;
+	dev_info->default_txportconf.ring_size = 256;
+	dev_info->default_rxportconf.ring_size = 256;
 }
 
 /* return 0 means link status changed, -1 means not changed */
-- 
2.9.5

[PATCH v3 3/4] net/i40e: add TxRx tuning parameters

[Remy Horton]

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>
---
 drivers/net/i40e/i40e_ethdev.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index d0bf4e3..acf1f99 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -3291,15 +3291,42 @@ i40e_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 		dev_info->max_tx_queues += dev_info->vmdq_queue_num;
 	}
 
-	if (I40E_PHY_TYPE_SUPPORT_40G(hw->phy.phy_types))
+	if (I40E_PHY_TYPE_SUPPORT_40G(hw->phy.phy_types)) {
 		/* For XL710 */
 		dev_info->speed_capa = ETH_LINK_SPEED_40G;
-	else if (I40E_PHY_TYPE_SUPPORT_25G(hw->phy.phy_types))
+		dev_info->default_rxportconf.nb_queues = 2;
+		dev_info->default_txportconf.nb_queues = 2;
+		if (dev->data->nb_rx_queues == 1)
+			dev_info->default_rxportconf.ring_size = 2048;
+		else
+			dev_info->default_rxportconf.ring_size = 1024;
+		if (dev->data->nb_tx_queues == 1)
+			dev_info->default_txportconf.ring_size = 1024;
+		else
+			dev_info->default_txportconf.ring_size = 512;
+
+	} else if (I40E_PHY_TYPE_SUPPORT_25G(hw->phy.phy_types)) {
 		/* For XXV710 */
 		dev_info->speed_capa = ETH_LINK_SPEED_25G;
-	else
+		dev_info->default_rxportconf.nb_queues = 1;
+		dev_info->default_txportconf.nb_queues = 1;
+		dev_info->default_rxportconf.ring_size = 256;
+		dev_info->default_txportconf.ring_size = 256;
+	} else {
 		/* For X710 */
 		dev_info->speed_capa = ETH_LINK_SPEED_1G | ETH_LINK_SPEED_10G;
+		dev_info->default_rxportconf.nb_queues = 1;
+		dev_info->default_txportconf.nb_queues = 1;
+		if (dev->data->dev_conf.link_speeds & ETH_LINK_SPEED_10G) {
+			dev_info->default_rxportconf.ring_size = 512;
+			dev_info->default_txportconf.ring_size = 256;
+		} else {
+			dev_info->default_rxportconf.ring_size = 256;
+			dev_info->default_txportconf.ring_size = 256;
+		}
+	}
+	dev_info->default_rxportconf.burst_size = 32;
+	dev_info->default_txportconf.burst_size = 32;
 }
 
 static int
-- 
2.9.5

[PATCH v3 4/4] testpmd: make use of per-PMD TxRx parameters

[Remy Horton]

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 testpmd to make use of per-PMD tuned parameter values.

Signed-off-by: Remy Horton <remy.horton@intel.com>
---
 app/test-pmd/cmdline.c    | 31 ++++++++++++++++++++++++++++---
 app/test-pmd/parameters.c | 38 +++++++++++++++++++++++++++++++++-----
 app/test-pmd/testpmd.c    |  5 +++--
 3 files changed, 64 insertions(+), 10 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 40b31ad..0914425 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -2599,6 +2599,8 @@ cmd_config_burst_parsed(void *parsed_result,
 			__attribute__((unused)) void *data)
 {
 	struct cmd_config_burst *res = parsed_result;
+	struct rte_eth_dev_info dev_info;
+	uint16_t rec_nb_pkts;
 
 	if (!all_ports_stopped()) {
 		printf("Please stop all ports first\n");
@@ -2606,11 +2608,34 @@ cmd_config_burst_parsed(void *parsed_result,
 	}
 
 	if (!strcmp(res->name, "burst")) {
-		if (res->value < 1 || res->value > MAX_PKT_BURST) {
+		if (res->value == 0) {
+			/* If user gives a value of zero, query the PMD for
+			 * its recommended Rx burst size. Testpmd uses a single
+			 * size for all ports, so assume all ports are the same
+			 * NIC model and use the values from Port 0.
+			 */
+			rte_eth_dev_info_get(0, &dev_info);
+			rec_nb_pkts = dev_info.default_rxportconf.burst_size;
+
+			if (rec_nb_pkts == 0) {
+				printf("PMD does not recommend a burst size.\n"
+					"User provided value must be between"
+					" 1 and %d\n", MAX_PKT_BURST);
+				return;
+			} else if (rec_nb_pkts > MAX_PKT_BURST) {
+				printf("PMD recommended burst size of %d"
+					" exceeds maximum value of %d\n",
+					rec_nb_pkts, MAX_PKT_BURST);
+				return;
+			}
+			printf("Using PMD-provided burst value of %d\n",
+				rec_nb_pkts);
+			nb_pkt_per_burst = rec_nb_pkts;
+		} else if (res->value > MAX_PKT_BURST) {
 			printf("burst must be >= 1 && <= %d\n", MAX_PKT_BURST);
 			return;
-		}
-		nb_pkt_per_burst = res->value;
+		} else
+			nb_pkt_per_burst = res->value;
 	} else {
 		printf("Unknown parameter\n");
 		return;
diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 2192bdc..cb6a229 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -544,6 +544,8 @@ launch_args_parse(int argc, char** argv)
 	/* Default offloads for all ports. */
 	uint64_t rx_offloads = rx_mode.offloads;
 	uint64_t tx_offloads = tx_mode.offloads;
+	struct rte_eth_dev_info dev_info;
+	uint16_t rec_nb_pkts;
 
 	static struct option lgopts[] = {
 		{ "help",			0, 0, 0 },
@@ -947,12 +949,38 @@ launch_args_parse(int argc, char** argv)
 			}
 			if (!strcmp(lgopts[opt_idx].name, "burst")) {
 				n = atoi(optarg);
-				if ((n >= 1) && (n <= MAX_PKT_BURST))
-					nb_pkt_per_burst = (uint16_t) n;
-				else
+				if (n == 0) {
+					/* A burst size of zero means that the
+					 * PMD should be queried for
+					 * recommended Rx burst size. Since
+					 * testpmd uses a single size for all
+					 * ports, port 0 is queried for the
+					 * value, on the assumption that all
+					 * ports are of the same NIC model.
+					 */
+					rte_eth_dev_info_get(0, &dev_info);
+					rec_nb_pkts = dev_info
+						.default_rxportconf.burst_size;
+
+					if (rec_nb_pkts == 0)
+						rte_exit(EXIT_FAILURE,
+							"PMD does not recommend a burst size. "
+							"Provided value must be between "
+							"1 and %d\n", MAX_PKT_BURST);
+					else if (rec_nb_pkts > MAX_PKT_BURST)
+						rte_exit(EXIT_FAILURE,
+							"PMD recommended burst size of %d"
+							" exceeds maximum value of %d\n",
+							rec_nb_pkts, MAX_PKT_BURST);
+					printf("Using PMD-provided burst value of %d\n",
+						rec_nb_pkts);
+					nb_pkt_per_burst = rec_nb_pkts;
+				} else if (n > MAX_PKT_BURST)
 					rte_exit(EXIT_FAILURE,
-						 "burst must >= 1 and <= %d]",
-						 MAX_PKT_BURST);
+						"burst must be between1 and %d\n",
+						MAX_PKT_BURST);
+				else
+					nb_pkt_per_burst = (uint16_t) n;
 			}
 			if (!strcmp(lgopts[opt_idx].name, "mbcache")) {
 				n = atoi(optarg);
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 4c0e258..82eb197 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -210,9 +210,10 @@ queueid_t nb_txq = 1; /**< Number of TX queues per port. */
 
 /*
  * Configurable number of RX/TX ring descriptors.
+ * Defaults are supplied by drivers via ethdev.
  */
-#define RTE_TEST_RX_DESC_DEFAULT 1024
-#define RTE_TEST_TX_DESC_DEFAULT 1024
+#define RTE_TEST_RX_DESC_DEFAULT 0
+#define RTE_TEST_TX_DESC_DEFAULT 0
 uint16_t nb_rxd = RTE_TEST_RX_DESC_DEFAULT; /**< Number of RX descriptors. */
 uint16_t nb_txd = RTE_TEST_TX_DESC_DEFAULT; /**< Number of TX descriptors. */
 
-- 
2.9.5

[PATCH v5 0/4] ethdev: add per-PMD tuning of RxTx parmeters

[Remy Horton]

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 patchset allows individual
PMDs to specify their preferred parameter values, and if so indicated by an
application, for them to be used automatically by the ethdev layer.

rte_eth_dev_configure() has been changed so that specifying zero for both
nb_rx_q AND nb_tx_q causes it to use driver preferred values, and if these
are not available, falls back to EAL defaults. Setting one (but not both)
to zero does not cause the use of defaults, as having one of them zeroed is
a valid setup.

This patchset includes per-PMD values for e1000 and i40e but it is expected
that subsequent patchsets will cover other PMDs. A deprecation notice
covering the API/ABI change is in place.

Changes in v5:
* uint_16_t corrected to uint16_t

Changes in v4:
* Added API/ABI change documentation
* Rebased to 78f5a2e93d74

Changes in v3:
* Changed formatting around new rte_eth_dev_info fields
* Added Doxygen documentation to struct rte_eth_dev_portconf
* Testpmd "port config all burst 0" and --burst=0 uses PMD 
  Rx burst recommendations.
* Added to release notes
* Rebased to 8ea081f38161

Changes in v2:
* Rebased to master
* Removed fallback values from rte_eth_dev_info_get()
* Added fallback values to rte_rte_[rt]x_queue_setup()
* Added fallback values to rte_eth_dev_configure()
* Corrected comment
* Removed deprecation notice
* Split RX and Tx into seperate structures
* Changed parameter names


Remy Horton (4):
  ethdev: add support for PMD-tuned Tx/Rx parameters
  net/e1000: add TxRx tuning parameters
  net/i40e: add TxRx tuning parameters
  testpmd: make use of per-PMD TxRx parameters

 app/test-pmd/cmdline.c                 | 31 +++++++++++++++++++++---
 app/test-pmd/parameters.c              | 38 +++++++++++++++++++++++++----
 app/test-pmd/testpmd.c                 |  5 ++--
 doc/guides/rel_notes/deprecation.rst   | 13 ----------
 doc/guides/rel_notes/release_18_05.rst | 35 +++++++++++++++++++++++++++
 drivers/net/e1000/em_ethdev.c          |  6 +++++
 drivers/net/i40e/i40e_ethdev.c         | 33 ++++++++++++++++++++++---
 lib/librte_ether/rte_ethdev.c          | 44 +++++++++++++++++++++++++++-------
 lib/librte_ether/rte_ethdev.h          | 25 +++++++++++++++++++
 9 files changed, 195 insertions(+), 35 deletions(-)

-- 
2.9.5

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

[Remy Horton]

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>
---
 doc/guides/rel_notes/deprecation.rst   | 13 ----------
 doc/guides/rel_notes/release_18_05.rst | 35 +++++++++++++++++++++++++++
 lib/librte_ether/rte_ethdev.c          | 44 +++++++++++++++++++++++++++-------
 lib/librte_ether/rte_ethdev.h          | 25 +++++++++++++++++++
 4 files changed, 95 insertions(+), 22 deletions(-)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index ec70b5f..d13077d 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -112,19 +112,6 @@ Deprecation Notices
   The new API add rss_level field to ``rte_eth_rss_conf`` to enable a choice
   of RSS hash calculation on outer or inner header of tunneled packet.
 
-* ethdev:  Currently, if the  rte_eth_rx_burst() function returns a value less
-  than *nb_pkts*, the application will assume that no more packets are present.
-  Some of the hw queue based hardware can only support smaller burst for RX
-  and TX and thus break the expectation of the rx_burst API. Similar is the
-  case for TX burst as well as ring sizes. ``rte_eth_dev_info`` will be added
-  with following new parameters so as to support semantics for drivers to
-  define a preferred size for Rx/Tx burst and rings.
-
-  - Member ``struct preferred_size`` would be added to enclose all preferred
-    size to be fetched from driver/implementation.
-  - Members ``uint16_t rx_burst``,  ``uint16_t tx_burst``, ``uint16_t rx_ring``,
-    and ``uint16_t tx_ring`` would be added to ``struct preferred_size``.
-
 * ethdev: A work is being planned for 18.05 to expose VF port representors
   as a mean to perform control and data path operation on the different VFs.
   As VF representor is an ethdev port, new fields are needed in order to map
diff --git a/doc/guides/rel_notes/release_18_05.rst b/doc/guides/rel_notes/release_18_05.rst
index e5fac1c..2fe9c70 100644
--- a/doc/guides/rel_notes/release_18_05.rst
+++ b/doc/guides/rel_notes/release_18_05.rst
@@ -58,6 +58,11 @@ New Features
   * Added support for NVGRE, VXLAN and GENEVE filters in flow API.
   * Added support for DROP action in flow API.
 
+* **Added PMD-recommended Tx and Rx parameters**
+
+  Applications can now query drivers for device-tuned values of
+  ring sizes, burst sizes, and number of queues.
+
 
 API Changes
 -----------
@@ -72,6 +77,29 @@ API Changes
    Also, make sure to start the actual text at the margin.
    =========================================================
 
+* **Changes to semantics of rte_eth_dev_configure() parameters.**
+
+   If both the ``nb_rx_q`` and ``nb_tx_q`` parameters are zero,
+   ``rte_eth_dev_configure`` will now use PMD-recommended queue sizes, or if
+   recommendations are not provided by the PMD the function will use ethdev
+   fall-back values. Previously setting both of the parameters to zero would
+   have resulted in ``-EINVAL`` being returned.
+
+* **Changes to semantics of rte_eth_rx_queue_setup() parameters.**
+
+   If the ``nb_rx_desc`` parameter is zero, ``rte_eth_rx_queue_setup`` will
+   now use the PMD-recommended Rx ring size, or in the case where the PMD
+   does not provide a recommendation, will use an ethdev-provided
+   fall-back value. Previously, setting ``nb_rx_desc`` to zero would have
+   resulted in an error.
+
+* **Changes to semantics of rte_eth_tx_queue_setup() parameters.**
+
+   If the ``nb_tx_desc`` parameter is zero, ``rte_eth_tx_queue_setup`` will
+   now use the PMD-recommended Tx ring size, or in the case where the PMD
+   does not provide a recoomendation, will use an ethdev-provided
+   fall-back value. Previously, setting ``nb_tx_desc`` to zero would have
+   resulted in an error.
 
 ABI Changes
 -----------
@@ -86,6 +114,13 @@ ABI Changes
    Also, make sure to start the actual text at the margin.
    =========================================================
 
+* **Additional fields in rte_eth_dev_info.**
+
+  The ``rte_eth_dev_info`` structure has had two extra entries appended to the
+  end of it: ``default_rxportconf`` and ``default_txportconf``. Each of these
+  in turn are ``rte_eth_dev_portconf`` structures containing three fields of
+  type ``uint16_t``: ``burst_size``, ``ring_size``, and ``nb_queues``. These
+  are parameter values recommended for use by the PMD.
 
 Removed Items
 -------------
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 2c74f7e..209796d 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1061,6 +1061,26 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
+	dev = &rte_eth_devices[port_id];
+
+	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
+	(*dev->dev_ops->dev_infos_get)(dev, &dev_info);
+
+	/* If number of queues specified by application for both Rx and Tx is
+	 * zero, use driver preferred values. This cannot be done individually
+	 * as it is valid for either Tx or Rx (but not both) to be zero.
+	 * If driver does not provide any preferred valued, fall back on
+	 * EAL defaults.
+	 */
+	if (nb_rx_q == 0 && nb_tx_q == 0) {
+		nb_rx_q = dev_info.default_rxportconf.nb_queues;
+		if (nb_rx_q == 0)
+			nb_rx_q = RTE_ETH_DEV_FALLBACK_RX_NBQUEUES;
+		nb_tx_q = dev_info.default_txportconf.nb_queues;
+		if (nb_tx_q == 0)
+			nb_tx_q = RTE_ETH_DEV_FALLBACK_TX_NBQUEUES;
+	}
+
 	if (nb_rx_q > RTE_MAX_QUEUES_PER_PORT) {
 		RTE_PMD_DEBUG_TRACE(
 			"Number of RX queues requested (%u) is greater than max supported(%d)\n",
@@ -1075,8 +1095,6 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 		return -EINVAL;
 	}
 
-	dev = &rte_eth_devices[port_id];
-
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -ENOTSUP);
 
@@ -1106,13 +1124,6 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 	 * than the maximum number of RX and TX queues supported by the
 	 * configured device.
 	 */
-	(*dev->dev_ops->dev_infos_get)(dev, &dev_info);
-
-	if (nb_rx_q == 0 && nb_tx_q == 0) {
-		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d both rx and tx queue cannot be 0\n", port_id);
-		return -EINVAL;
-	}
-
 	if (nb_rx_q > dev_info.max_rx_queues) {
 		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d nb_rx_queues=%d > %d\n",
 				port_id, nb_rx_q, dev_info.max_rx_queues);
@@ -1477,6 +1488,14 @@ rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
 		return -EINVAL;
 	}
 
+	/* Use default specified by driver, if nb_rx_desc is zero */
+	if (nb_rx_desc == 0) {
+		nb_rx_desc = dev_info.default_rxportconf.ring_size;
+		/* If driver default is also zero, fall back on EAL default */
+		if (nb_rx_desc == 0)
+			nb_rx_desc = RTE_ETH_DEV_FALLBACK_RX_RINGSIZE;
+	}
+
 	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) {
@@ -1600,6 +1619,13 @@ 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.default_txportconf.ring_size;
+		/* If driver default is zero, fall back on EAL default */
+		if (nb_tx_desc == 0)
+			nb_tx_desc = RTE_ETH_DEV_FALLBACK_TX_RINGSIZE;
+	}
 	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) {
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 5e13dca..685145f 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -988,6 +988,27 @@ struct rte_eth_conf {
 
 struct rte_pci_device;
 
+/*
+ * Fallback default preferred Rx/Tx port parameters.
+ * These are used if an application requests default parameters
+ * but the PMD does not provide preferred values.
+ */
+#define RTE_ETH_DEV_FALLBACK_RX_RINGSIZE 512
+#define RTE_ETH_DEV_FALLBACK_TX_RINGSIZE 512
+#define RTE_ETH_DEV_FALLBACK_RX_NBQUEUES 1
+#define RTE_ETH_DEV_FALLBACK_TX_NBQUEUES 1
+
+/**
+ * Preferred Rx/Tx port parameters.
+ * There are separate instances of this structure for transmission
+ * and reception respectively.
+ */
+struct rte_eth_dev_portconf {
+	uint16_t burst_size; /**< Device-preferred burst size */
+	uint16_t ring_size; /**< Device-preferred size of queue rings */
+	uint16_t nb_queues; /**< Device-preferred number of queues */
+};
+
 /**
  * Ethernet device information
  */
@@ -1029,6 +1050,10 @@ 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. */
+	/** Rx parameter recommendations */
+	struct rte_eth_dev_portconf default_rxportconf;
+	/** Tx parameter recommendations */
+	struct rte_eth_dev_portconf default_txportconf;
 };
 
 /**
-- 
2.9.5

[PATCH v5 2/4] net/e1000: add TxRx tuning parameters

[Remy Horton]

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>
---
 drivers/net/e1000/em_ethdev.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
index f727382..2035ea8 100644
--- a/drivers/net/e1000/em_ethdev.c
+++ b/drivers/net/e1000/em_ethdev.c
@@ -1100,6 +1100,12 @@ eth_em_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 	dev_info->speed_capa = ETH_LINK_SPEED_10M_HD | ETH_LINK_SPEED_10M |
 			ETH_LINK_SPEED_100M_HD | ETH_LINK_SPEED_100M |
 			ETH_LINK_SPEED_1G;
+
+	/* Preferred queue parameters */
+	dev_info->default_rxportconf.nb_queues = 1;
+	dev_info->default_txportconf.nb_queues = 1;
+	dev_info->default_txportconf.ring_size = 256;
+	dev_info->default_rxportconf.ring_size = 256;
 }
 
 /* return 0 means link status changed, -1 means not changed */
-- 
2.9.5

[PATCH v5 3/4] net/i40e: add TxRx tuning parameters

[Remy Horton]

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>
---
 drivers/net/i40e/i40e_ethdev.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index d0bf4e3..acf1f99 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -3291,15 +3291,42 @@ i40e_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 		dev_info->max_tx_queues += dev_info->vmdq_queue_num;
 	}
 
-	if (I40E_PHY_TYPE_SUPPORT_40G(hw->phy.phy_types))
+	if (I40E_PHY_TYPE_SUPPORT_40G(hw->phy.phy_types)) {
 		/* For XL710 */
 		dev_info->speed_capa = ETH_LINK_SPEED_40G;
-	else if (I40E_PHY_TYPE_SUPPORT_25G(hw->phy.phy_types))
+		dev_info->default_rxportconf.nb_queues = 2;
+		dev_info->default_txportconf.nb_queues = 2;
+		if (dev->data->nb_rx_queues == 1)
+			dev_info->default_rxportconf.ring_size = 2048;
+		else
+			dev_info->default_rxportconf.ring_size = 1024;
+		if (dev->data->nb_tx_queues == 1)
+			dev_info->default_txportconf.ring_size = 1024;
+		else
+			dev_info->default_txportconf.ring_size = 512;
+
+	} else if (I40E_PHY_TYPE_SUPPORT_25G(hw->phy.phy_types)) {
 		/* For XXV710 */
 		dev_info->speed_capa = ETH_LINK_SPEED_25G;
-	else
+		dev_info->default_rxportconf.nb_queues = 1;
+		dev_info->default_txportconf.nb_queues = 1;
+		dev_info->default_rxportconf.ring_size = 256;
+		dev_info->default_txportconf.ring_size = 256;
+	} else {
 		/* For X710 */
 		dev_info->speed_capa = ETH_LINK_SPEED_1G | ETH_LINK_SPEED_10G;
+		dev_info->default_rxportconf.nb_queues = 1;
+		dev_info->default_txportconf.nb_queues = 1;
+		if (dev->data->dev_conf.link_speeds & ETH_LINK_SPEED_10G) {
+			dev_info->default_rxportconf.ring_size = 512;
+			dev_info->default_txportconf.ring_size = 256;
+		} else {
+			dev_info->default_rxportconf.ring_size = 256;
+			dev_info->default_txportconf.ring_size = 256;
+		}
+	}
+	dev_info->default_rxportconf.burst_size = 32;
+	dev_info->default_txportconf.burst_size = 32;
 }
 
 static int
-- 
2.9.5

[PATCH v5 4/4] testpmd: make use of per-PMD TxRx parameters

[Remy Horton]

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 testpmd to make use of per-PMD tuned parameter values.

Signed-off-by: Remy Horton <remy.horton@intel.com>
---
 app/test-pmd/cmdline.c    | 31 ++++++++++++++++++++++++++++---
 app/test-pmd/parameters.c | 38 +++++++++++++++++++++++++++++++++-----
 app/test-pmd/testpmd.c    |  5 +++--
 3 files changed, 64 insertions(+), 10 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 40b31ad..0914425 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -2599,6 +2599,8 @@ cmd_config_burst_parsed(void *parsed_result,
 			__attribute__((unused)) void *data)
 {
 	struct cmd_config_burst *res = parsed_result;
+	struct rte_eth_dev_info dev_info;
+	uint16_t rec_nb_pkts;
 
 	if (!all_ports_stopped()) {
 		printf("Please stop all ports first\n");
@@ -2606,11 +2608,34 @@ cmd_config_burst_parsed(void *parsed_result,
 	}
 
 	if (!strcmp(res->name, "burst")) {
-		if (res->value < 1 || res->value > MAX_PKT_BURST) {
+		if (res->value == 0) {
+			/* If user gives a value of zero, query the PMD for
+			 * its recommended Rx burst size. Testpmd uses a single
+			 * size for all ports, so assume all ports are the same
+			 * NIC model and use the values from Port 0.
+			 */
+			rte_eth_dev_info_get(0, &dev_info);
+			rec_nb_pkts = dev_info.default_rxportconf.burst_size;
+
+			if (rec_nb_pkts == 0) {
+				printf("PMD does not recommend a burst size.\n"
+					"User provided value must be between"
+					" 1 and %d\n", MAX_PKT_BURST);
+				return;
+			} else if (rec_nb_pkts > MAX_PKT_BURST) {
+				printf("PMD recommended burst size of %d"
+					" exceeds maximum value of %d\n",
+					rec_nb_pkts, MAX_PKT_BURST);
+				return;
+			}
+			printf("Using PMD-provided burst value of %d\n",
+				rec_nb_pkts);
+			nb_pkt_per_burst = rec_nb_pkts;
+		} else if (res->value > MAX_PKT_BURST) {
 			printf("burst must be >= 1 && <= %d\n", MAX_PKT_BURST);
 			return;
-		}
-		nb_pkt_per_burst = res->value;
+		} else
+			nb_pkt_per_burst = res->value;
 	} else {
 		printf("Unknown parameter\n");
 		return;
diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 2192bdc..cb6a229 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -544,6 +544,8 @@ launch_args_parse(int argc, char** argv)
 	/* Default offloads for all ports. */
 	uint64_t rx_offloads = rx_mode.offloads;
 	uint64_t tx_offloads = tx_mode.offloads;
+	struct rte_eth_dev_info dev_info;
+	uint16_t rec_nb_pkts;
 
 	static struct option lgopts[] = {
 		{ "help",			0, 0, 0 },
@@ -947,12 +949,38 @@ launch_args_parse(int argc, char** argv)
 			}
 			if (!strcmp(lgopts[opt_idx].name, "burst")) {
 				n = atoi(optarg);
-				if ((n >= 1) && (n <= MAX_PKT_BURST))
-					nb_pkt_per_burst = (uint16_t) n;
-				else
+				if (n == 0) {
+					/* A burst size of zero means that the
+					 * PMD should be queried for
+					 * recommended Rx burst size. Since
+					 * testpmd uses a single size for all
+					 * ports, port 0 is queried for the
+					 * value, on the assumption that all
+					 * ports are of the same NIC model.
+					 */
+					rte_eth_dev_info_get(0, &dev_info);
+					rec_nb_pkts = dev_info
+						.default_rxportconf.burst_size;
+
+					if (rec_nb_pkts == 0)
+						rte_exit(EXIT_FAILURE,
+							"PMD does not recommend a burst size. "
+							"Provided value must be between "
+							"1 and %d\n", MAX_PKT_BURST);
+					else if (rec_nb_pkts > MAX_PKT_BURST)
+						rte_exit(EXIT_FAILURE,
+							"PMD recommended burst size of %d"
+							" exceeds maximum value of %d\n",
+							rec_nb_pkts, MAX_PKT_BURST);
+					printf("Using PMD-provided burst value of %d\n",
+						rec_nb_pkts);
+					nb_pkt_per_burst = rec_nb_pkts;
+				} else if (n > MAX_PKT_BURST)
 					rte_exit(EXIT_FAILURE,
-						 "burst must >= 1 and <= %d]",
-						 MAX_PKT_BURST);
+						"burst must be between1 and %d\n",
+						MAX_PKT_BURST);
+				else
+					nb_pkt_per_burst = (uint16_t) n;
 			}
 			if (!strcmp(lgopts[opt_idx].name, "mbcache")) {
 				n = atoi(optarg);
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 4c0e258..82eb197 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -210,9 +210,10 @@ queueid_t nb_txq = 1; /**< Number of TX queues per port. */
 
 /*
  * Configurable number of RX/TX ring descriptors.
+ * Defaults are supplied by drivers via ethdev.
  */
-#define RTE_TEST_RX_DESC_DEFAULT 1024
-#define RTE_TEST_TX_DESC_DEFAULT 1024
+#define RTE_TEST_RX_DESC_DEFAULT 0
+#define RTE_TEST_TX_DESC_DEFAULT 0
 uint16_t nb_rxd = RTE_TEST_RX_DESC_DEFAULT; /**< Number of RX descriptors. */
 uint16_t nb_txd = RTE_TEST_TX_DESC_DEFAULT; /**< Number of TX descriptors. */
 
-- 
2.9.5

Re: [PATCH v5 4/4] testpmd: make use of per-PMD TxRx parameters

[Shreyansh Jain]

On Friday 06 April 2018 08:20 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 testpmd to make use of per-PMD tuned parameter values.
> 
> Signed-off-by: Remy Horton <remy.horton@intel.com>
> ---
>   app/test-pmd/cmdline.c    | 31 ++++++++++++++++++++++++++++---
>   app/test-pmd/parameters.c | 38 +++++++++++++++++++++++++++++++++-----
>   app/test-pmd/testpmd.c    |  5 +++--
>   3 files changed, 64 insertions(+), 10 deletions(-)
> 
> diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
> index 40b31ad..0914425 100644
> --- a/app/test-pmd/cmdline.c
> +++ b/app/test-pmd/cmdline.c
> @@ -2599,6 +2599,8 @@ cmd_config_burst_parsed(void *parsed_result,
>   			__attribute__((unused)) void *data)
>   {
>   	struct cmd_config_burst *res = parsed_result;
> +	struct rte_eth_dev_info dev_info;
> +	uint16_t rec_nb_pkts;
>   
>   	if (!all_ports_stopped()) {
>   		printf("Please stop all ports first\n");
> @@ -2606,11 +2608,34 @@ cmd_config_burst_parsed(void *parsed_result,
>   	}
>   
>   	if (!strcmp(res->name, "burst")) {
> -		if (res->value < 1 || res->value > MAX_PKT_BURST) {
> +		if (res->value == 0) {

Documentation for burst mode changes to testpmd would need an update.
I guess, only when the user explicitly sets 'set burst 0' would the 
driver defaults be picked up - isn't it?

Maybe something like this:

--->8---
--- a/doc/guides/testpmd_app_ug/run_app.rst
+++ b/doc/guides/testpmd_app_ug/run_app.rst
@@ -372,7 +372,9 @@ The commandline options are:
  *   ``--burst=N``

      Set the number of packets per burst to N, where 1 <= N <= 512.
-    The default value is 16.
+    The default value is 32.
+    If set to 0, driver default is used if defined. Else, if driver default
+    is not defined, default of 32 is used.

  *   ``--mbcache=N``
--->8---

In the above, I think the existing documented default value needs to be 
changed. It is set to '#define DEF_PKT_BURST 32'

If you add that, please use my ack for next revision.
(For patch 1/4, I had already given my Ack in v2)

> +			/* If user gives a value of zero, query the PMD for
> +			 * its recommended Rx burst size. Testpmd uses a single
> +			 * size for all ports, so assume all ports are the same
> +			 * NIC model and use the values from Port 0.
> +			 */
> +			rte_eth_dev_info_get(0, &dev_info);
> +			rec_nb_pkts = dev_info.default_rxportconf.burst_size;
> +
> +			if (rec_nb_pkts == 0) {
> +				printf("PMD does not recommend a burst size.\n"
> +					"User provided value must be between"
> +					" 1 and %d\n", MAX_PKT_BURST);
> +				return;
> +			} else if (rec_nb_pkts > MAX_PKT_BURST) {
> +				printf("PMD recommended burst size of %d"
> +					" exceeds maximum value of %d\n",
> +					rec_nb_pkts, MAX_PKT_BURST);
> +				return;
> +			}
> +			printf("Using PMD-provided burst value of %d\n",
> +				rec_nb_pkts);
> +			nb_pkt_per_burst = rec_nb_pkts;
> +		} else if (res->value > MAX_PKT_BURST) {
>   			printf("burst must be >= 1 && <= %d\n", MAX_PKT_BURST);
>   			return;
> -		}
> -		nb_pkt_per_burst = res->value;
> +		} else
> +			nb_pkt_per_burst = res->value;
>   	} else {
>   		printf("Unknown parameter\n");
>   		return;
> diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
> index 2192bdc..cb6a229 100644
> --- a/app/test-pmd/parameters.c
> +++ b/app/test-pmd/parameters.c
> @@ -544,6 +544,8 @@ launch_args_parse(int argc, char** argv)
>   	/* Default offloads for all ports. */
>   	uint64_t rx_offloads = rx_mode.offloads;
>   	uint64_t tx_offloads = tx_mode.offloads;
> +	struct rte_eth_dev_info dev_info;
> +	uint16_t rec_nb_pkts;
>   
>   	static struct option lgopts[] = {
>   		{ "help",			0, 0, 0 },
> @@ -947,12 +949,38 @@ launch_args_parse(int argc, char** argv)
>   			}
>   			if (!strcmp(lgopts[opt_idx].name, "burst")) {
>   				n = atoi(optarg);
> -				if ((n >= 1) && (n <= MAX_PKT_BURST))
> -					nb_pkt_per_burst = (uint16_t) n;
> -				else
> +				if (n == 0) {
> +					/* A burst size of zero means that the
> +					 * PMD should be queried for
> +					 * recommended Rx burst size. Since
> +					 * testpmd uses a single size for all
> +					 * ports, port 0 is queried for the
> +					 * value, on the assumption that all
> +					 * ports are of the same NIC model.
> +					 */
> +					rte_eth_dev_info_get(0, &dev_info);
> +					rec_nb_pkts = dev_info
> +						.default_rxportconf.burst_size;
> +
> +					if (rec_nb_pkts == 0)
> +						rte_exit(EXIT_FAILURE,
> +							"PMD does not recommend a burst size. "
> +							"Provided value must be between "
> +							"1 and %d\n", MAX_PKT_BURST);
> +					else if (rec_nb_pkts > MAX_PKT_BURST)
> +						rte_exit(EXIT_FAILURE,
> +							"PMD recommended burst size of %d"
> +							" exceeds maximum value of %d\n",
> +							rec_nb_pkts, MAX_PKT_BURST);
> +					printf("Using PMD-provided burst value of %d\n",
> +						rec_nb_pkts);
> +					nb_pkt_per_burst = rec_nb_pkts;
> +				} else if (n > MAX_PKT_BURST)
>   					rte_exit(EXIT_FAILURE,
> -						 "burst must >= 1 and <= %d]",
> -						 MAX_PKT_BURST);
> +						"burst must be between1 and %d\n",
> +						MAX_PKT_BURST);
> +				else
> +					nb_pkt_per_burst = (uint16_t) n;
>   			}
>   			if (!strcmp(lgopts[opt_idx].name, "mbcache")) {
>   				n = atoi(optarg);
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 4c0e258..82eb197 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -210,9 +210,10 @@ queueid_t nb_txq = 1; /**< Number of TX queues per port. */
>   
>   /*
>    * Configurable number of RX/TX ring descriptors.
> + * Defaults are supplied by drivers via ethdev.
>    */
> -#define RTE_TEST_RX_DESC_DEFAULT 1024
> -#define RTE_TEST_TX_DESC_DEFAULT 1024
> +#define RTE_TEST_RX_DESC_DEFAULT 0
> +#define RTE_TEST_TX_DESC_DEFAULT 0
>   uint16_t nb_rxd = RTE_TEST_RX_DESC_DEFAULT; /**< Number of RX descriptors. */
>   uint16_t nb_txd = RTE_TEST_TX_DESC_DEFAULT; /**< Number of TX descriptors. */
>   
> 

Re: [PATCH v5 4/4] testpmd: make use of per-PMD TxRx parameters

[Remy Horton]

On 09/04/2018 13:55, Shreyansh Jain wrote:
[..]
> Documentation for burst mode changes to testpmd would need an update.
> I guess, only when the user explicitly sets 'set burst 0' would the
> driver defaults be picked up - isn't it?

Yes.


> Maybe something like this:
>
> --->8---
> --- a/doc/guides/testpmd_app_ug/run_app.rst
> +++ b/doc/guides/testpmd_app_ug/run_app.rst
> @@ -372,7 +372,9 @@ The commandline options are:
>  *   ``--burst=N``
>
>      Set the number of packets per burst to N, where 1 <= N <= 512.
> -    The default value is 16.
> +    The default value is 32.
> +    If set to 0, driver default is used if defined. Else, if driver
> default
> +    is not defined, default of 32 is used.
>
>  *   ``--mbcache=N``
> --->8---
>
> In the above, I think the existing documented default value needs to be
> changed. It is set to '#define DEF_PKT_BURST 32'

Had a quick look and it looks like that discrepancy has been there since 
the documentation was converted to .rst in 2014.


> If you add that, please use my ack for next revision.
> (For patch 1/4, I had already given my Ack in v2)

I'll add in the snippet above. 18.05 integration deadline has I think 
passed, but documentation changes will still get in.

Re: [PATCH v5 4/4] testpmd: make use of per-PMD TxRx parameters

[Shreyansh Jain]

> -----Original Message-----
> From: Remy Horton [mailto:remy.horton@intel.com]
> Sent: Monday, April 9, 2018 8:09 PM
> To: Shreyansh Jain <shreyansh.jain@nxp.com>; dev@dpdk.org
> Cc: John McNamara <john.mcnamara@intel.com>; 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>; Thomas
> Monjalon <thomas@monjalon.net>
> Subject: Re: [dpdk-dev] [PATCH v5 4/4] testpmd: make use of per-PMD TxRx
> parameters
> 
> 

[...]

> 
> 
> > If you add that, please use my ack for next revision.
> > (For patch 1/4, I had already given my Ack in v2)
> 
> I'll add in the snippet above. 18.05 integration deadline has I think
> passed, but documentation changes will still get in.

@thomas you won't be integrating these default conf patches in 18.05?

Re: [PATCH v5 4/4] testpmd: make use of per-PMD TxRx parameters

[Remy Horton]

On 10/04/2018 05:18, Shreyansh Jain wrote:
[..]
>> I'll add in the snippet above. 18.05 integration deadline has I think
>> passed, but documentation changes will still get in.
>
> @thomas you won't be integrating these default conf patches in 18.05?
>

I meant that since the v5 and v6 will only differ in terms of docs, the 
additional changes should get in..

Re: [PATCH v5 4/4] testpmd: make use of per-PMD TxRx parameters

[Shreyansh Jain]

On Tuesday 10 April 2018 11:39 AM, Remy Horton wrote:
> 
> On 10/04/2018 05:18, Shreyansh Jain wrote:
> [..]
>>> I'll add in the snippet above. 18.05 integration deadline has I think
>>> passed, but documentation changes will still get in.
>>
>> @thomas you won't be integrating these default conf patches in 18.05?
>>
> 
> I meant that since the v5 and v6 will only differ in terms of docs, the 
> additional changes should get in..
Oh, Ok. My bad.
I read it as original issue in documentation (of default burst size) can 
be merged but this patch might not ...
Thanks for clarification.

Re: [PATCH v5 0/4] ethdev: add per-PMD tuning of RxTx parmeters

[Ferruh Yigit]

On 4/6/2018 3:49 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 patchset allows individual
> PMDs to specify their preferred parameter values, and if so indicated by an
> application, for them to be used automatically by the ethdev layer.
> 
> rte_eth_dev_configure() has been changed so that specifying zero for both
> nb_rx_q AND nb_tx_q causes it to use driver preferred values, and if these
> are not available, falls back to EAL defaults. Setting one (but not both)
> to zero does not cause the use of defaults, as having one of them zeroed is
> a valid setup.
> 
> This patchset includes per-PMD values for e1000 and i40e but it is expected
> that subsequent patchsets will cover other PMDs. A deprecation notice
> covering the API/ABI change is in place.
> 
> Changes in v5:
> * uint_16_t corrected to uint16_t
> 
> Changes in v4:
> * Added API/ABI change documentation
> * Rebased to 78f5a2e93d74
> 
> Changes in v3:
> * Changed formatting around new rte_eth_dev_info fields
> * Added Doxygen documentation to struct rte_eth_dev_portconf
> * Testpmd "port config all burst 0" and --burst=0 uses PMD 
>   Rx burst recommendations.
> * Added to release notes
> * Rebased to 8ea081f38161
> 
> Changes in v2:
> * Rebased to master
> * Removed fallback values from rte_eth_dev_info_get()
> * Added fallback values to rte_rte_[rt]x_queue_setup()
> * Added fallback values to rte_eth_dev_configure()
> * Corrected comment
> * Removed deprecation notice
> * Split RX and Tx into seperate structures
> * Changed parameter names
> 
> 
> Remy Horton (4):
>   ethdev: add support for PMD-tuned Tx/Rx parameters
>   net/e1000: add TxRx tuning parameters
>   net/i40e: add TxRx tuning parameters
>   testpmd: make use of per-PMD TxRx parameters

For series,
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>

[PATCH v6 0/4] ethdev: add per-PMD tuning of RxTx parmeters

[Remy Horton]

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 patchset allows individual
PMDs to specify their preferred parameter values, and if so indicated by an
application, for them to be used automatically by the ethdev layer.

rte_eth_dev_configure() has been changed so that specifying zero for both
nb_rx_q AND nb_tx_q causes it to use driver preferred values, and if these
are not available, falls back to EAL defaults. Setting one (but not both)
to zero does not cause the use of defaults, as having one of them zeroed is
a valid setup.

This patchset includes per-PMD values for e1000 and i40e but it is expected
that subsequent patchsets will cover other PMDs. A deprecation notice
covering the API/ABI change is in place.

Changes in v6:
* Updated/corrected testpmd documentation
* Carried forward acks/review
* Rebased to d218a4d060de

Changes in v5:
* uint_16_t corrected to uint16_t

Changes in v4:
* Added API/ABI change documentation
* Rebased to 78f5a2e93d74

Changes in v3:
* Changed formatting around new rte_eth_dev_info fields
* Added Doxygen documentation to struct rte_eth_dev_portconf
* Testpmd "port config all burst 0" and --burst=0 uses PMD 
  Rx burst recommendations.
* Added to release notes
* Rebased to 8ea081f38161

Changes in v2:
* Rebased to master
* Removed fallback values from rte_eth_dev_info_get()
* Added fallback values to rte_rte_[rt]x_queue_setup()
* Added fallback values to rte_eth_dev_configure()
* Corrected comment
* Removed deprecation notice
* Split RX and Tx into seperate structures
* Changed parameter names


Remy Horton (4):
  ethdev: add support for PMD-tuned Tx/Rx parameters
  net/e1000: add TxRx tuning parameters
  net/i40e: add TxRx tuning parameters
  testpmd: make use of per-PMD TxRx parameters

 app/test-pmd/cmdline.c                 | 31 +++++++++++++++++++++---
 app/test-pmd/parameters.c              | 38 +++++++++++++++++++++++++----
 app/test-pmd/testpmd.c                 |  5 ++--
 doc/guides/rel_notes/deprecation.rst   | 13 ----------
 doc/guides/rel_notes/release_18_05.rst | 35 +++++++++++++++++++++++++++
 doc/guides/testpmd_app_ug/run_app.rst  |  4 +++-
 drivers/net/e1000/em_ethdev.c          |  6 +++++
 drivers/net/i40e/i40e_ethdev.c         | 33 ++++++++++++++++++++++---
 lib/librte_ether/rte_ethdev.c          | 44 +++++++++++++++++++++++++++-------
 lib/librte_ether/rte_ethdev.h          | 25 +++++++++++++++++++
 10 files changed, 198 insertions(+), 36 deletions(-)

-- 
2.9.5

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

[Remy Horton]

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>
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
Acked-by: Shreyansh Jain <shreyansh.jain@nxp.com>
---
 doc/guides/rel_notes/deprecation.rst   | 13 ----------
 doc/guides/rel_notes/release_18_05.rst | 35 +++++++++++++++++++++++++++
 lib/librte_ether/rte_ethdev.c          | 44 +++++++++++++++++++++++++++-------
 lib/librte_ether/rte_ethdev.h          | 25 +++++++++++++++++++
 4 files changed, 95 insertions(+), 22 deletions(-)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index ec70b5f..d13077d 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -112,19 +112,6 @@ Deprecation Notices
   The new API add rss_level field to ``rte_eth_rss_conf`` to enable a choice
   of RSS hash calculation on outer or inner header of tunneled packet.
 
-* ethdev:  Currently, if the  rte_eth_rx_burst() function returns a value less
-  than *nb_pkts*, the application will assume that no more packets are present.
-  Some of the hw queue based hardware can only support smaller burst for RX
-  and TX and thus break the expectation of the rx_burst API. Similar is the
-  case for TX burst as well as ring sizes. ``rte_eth_dev_info`` will be added
-  with following new parameters so as to support semantics for drivers to
-  define a preferred size for Rx/Tx burst and rings.
-
-  - Member ``struct preferred_size`` would be added to enclose all preferred
-    size to be fetched from driver/implementation.
-  - Members ``uint16_t rx_burst``,  ``uint16_t tx_burst``, ``uint16_t rx_ring``,
-    and ``uint16_t tx_ring`` would be added to ``struct preferred_size``.
-
 * ethdev: A work is being planned for 18.05 to expose VF port representors
   as a mean to perform control and data path operation on the different VFs.
   As VF representor is an ethdev port, new fields are needed in order to map
diff --git a/doc/guides/rel_notes/release_18_05.rst b/doc/guides/rel_notes/release_18_05.rst
index abc1c17..0d57a63 100644
--- a/doc/guides/rel_notes/release_18_05.rst
+++ b/doc/guides/rel_notes/release_18_05.rst
@@ -58,6 +58,11 @@ New Features
   * Added support for NVGRE, VXLAN and GENEVE filters in flow API.
   * Added support for DROP action in flow API.
 
+* **Added PMD-recommended Tx and Rx parameters**
+
+  Applications can now query drivers for device-tuned values of
+  ring sizes, burst sizes, and number of queues.
+
 
 API Changes
 -----------
@@ -83,6 +88,29 @@ API Changes
   memory footprint which helps in better cache utilization when large number
   of meter objects are used.
 
+* **Changes to semantics of rte_eth_dev_configure() parameters.**
+
+   If both the ``nb_rx_q`` and ``nb_tx_q`` parameters are zero,
+   ``rte_eth_dev_configure`` will now use PMD-recommended queue sizes, or if
+   recommendations are not provided by the PMD the function will use ethdev
+   fall-back values. Previously setting both of the parameters to zero would
+   have resulted in ``-EINVAL`` being returned.
+
+* **Changes to semantics of rte_eth_rx_queue_setup() parameters.**
+
+   If the ``nb_rx_desc`` parameter is zero, ``rte_eth_rx_queue_setup`` will
+   now use the PMD-recommended Rx ring size, or in the case where the PMD
+   does not provide a recommendation, will use an ethdev-provided
+   fall-back value. Previously, setting ``nb_rx_desc`` to zero would have
+   resulted in an error.
+
+* **Changes to semantics of rte_eth_tx_queue_setup() parameters.**
+
+   If the ``nb_tx_desc`` parameter is zero, ``rte_eth_tx_queue_setup`` will
+   now use the PMD-recommended Tx ring size, or in the case where the PMD
+   does not provide a recoomendation, will use an ethdev-provided
+   fall-back value. Previously, setting ``nb_tx_desc`` to zero would have
+   resulted in an error.
 
 ABI Changes
 -----------
@@ -97,6 +125,13 @@ ABI Changes
    Also, make sure to start the actual text at the margin.
    =========================================================
 
+* **Additional fields in rte_eth_dev_info.**
+
+  The ``rte_eth_dev_info`` structure has had two extra entries appended to the
+  end of it: ``default_rxportconf`` and ``default_txportconf``. Each of these
+  in turn are ``rte_eth_dev_portconf`` structures containing three fields of
+  type ``uint16_t``: ``burst_size``, ``ring_size``, and ``nb_queues``. These
+  are parameter values recommended for use by the PMD.
 
 Removed Items
 -------------
diff --git a/lib/librte_ether/rte_ethdev.c b/lib/librte_ether/rte_ethdev.c
index 2c74f7e..209796d 100644
--- a/lib/librte_ether/rte_ethdev.c
+++ b/lib/librte_ether/rte_ethdev.c
@@ -1061,6 +1061,26 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 
 	RTE_ETH_VALID_PORTID_OR_ERR_RET(port_id, -EINVAL);
 
+	dev = &rte_eth_devices[port_id];
+
+	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
+	(*dev->dev_ops->dev_infos_get)(dev, &dev_info);
+
+	/* If number of queues specified by application for both Rx and Tx is
+	 * zero, use driver preferred values. This cannot be done individually
+	 * as it is valid for either Tx or Rx (but not both) to be zero.
+	 * If driver does not provide any preferred valued, fall back on
+	 * EAL defaults.
+	 */
+	if (nb_rx_q == 0 && nb_tx_q == 0) {
+		nb_rx_q = dev_info.default_rxportconf.nb_queues;
+		if (nb_rx_q == 0)
+			nb_rx_q = RTE_ETH_DEV_FALLBACK_RX_NBQUEUES;
+		nb_tx_q = dev_info.default_txportconf.nb_queues;
+		if (nb_tx_q == 0)
+			nb_tx_q = RTE_ETH_DEV_FALLBACK_TX_NBQUEUES;
+	}
+
 	if (nb_rx_q > RTE_MAX_QUEUES_PER_PORT) {
 		RTE_PMD_DEBUG_TRACE(
 			"Number of RX queues requested (%u) is greater than max supported(%d)\n",
@@ -1075,8 +1095,6 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 		return -EINVAL;
 	}
 
-	dev = &rte_eth_devices[port_id];
-
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_infos_get, -ENOTSUP);
 	RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->dev_configure, -ENOTSUP);
 
@@ -1106,13 +1124,6 @@ rte_eth_dev_configure(uint16_t port_id, uint16_t nb_rx_q, uint16_t nb_tx_q,
 	 * than the maximum number of RX and TX queues supported by the
 	 * configured device.
 	 */
-	(*dev->dev_ops->dev_infos_get)(dev, &dev_info);
-
-	if (nb_rx_q == 0 && nb_tx_q == 0) {
-		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d both rx and tx queue cannot be 0\n", port_id);
-		return -EINVAL;
-	}
-
 	if (nb_rx_q > dev_info.max_rx_queues) {
 		RTE_PMD_DEBUG_TRACE("ethdev port_id=%d nb_rx_queues=%d > %d\n",
 				port_id, nb_rx_q, dev_info.max_rx_queues);
@@ -1477,6 +1488,14 @@ rte_eth_rx_queue_setup(uint16_t port_id, uint16_t rx_queue_id,
 		return -EINVAL;
 	}
 
+	/* Use default specified by driver, if nb_rx_desc is zero */
+	if (nb_rx_desc == 0) {
+		nb_rx_desc = dev_info.default_rxportconf.ring_size;
+		/* If driver default is also zero, fall back on EAL default */
+		if (nb_rx_desc == 0)
+			nb_rx_desc = RTE_ETH_DEV_FALLBACK_RX_RINGSIZE;
+	}
+
 	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) {
@@ -1600,6 +1619,13 @@ 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.default_txportconf.ring_size;
+		/* If driver default is zero, fall back on EAL default */
+		if (nb_tx_desc == 0)
+			nb_tx_desc = RTE_ETH_DEV_FALLBACK_TX_RINGSIZE;
+	}
 	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) {
diff --git a/lib/librte_ether/rte_ethdev.h b/lib/librte_ether/rte_ethdev.h
index 5e13dca..685145f 100644
--- a/lib/librte_ether/rte_ethdev.h
+++ b/lib/librte_ether/rte_ethdev.h
@@ -988,6 +988,27 @@ struct rte_eth_conf {
 
 struct rte_pci_device;
 
+/*
+ * Fallback default preferred Rx/Tx port parameters.
+ * These are used if an application requests default parameters
+ * but the PMD does not provide preferred values.
+ */
+#define RTE_ETH_DEV_FALLBACK_RX_RINGSIZE 512
+#define RTE_ETH_DEV_FALLBACK_TX_RINGSIZE 512
+#define RTE_ETH_DEV_FALLBACK_RX_NBQUEUES 1
+#define RTE_ETH_DEV_FALLBACK_TX_NBQUEUES 1
+
+/**
+ * Preferred Rx/Tx port parameters.
+ * There are separate instances of this structure for transmission
+ * and reception respectively.
+ */
+struct rte_eth_dev_portconf {
+	uint16_t burst_size; /**< Device-preferred burst size */
+	uint16_t ring_size; /**< Device-preferred size of queue rings */
+	uint16_t nb_queues; /**< Device-preferred number of queues */
+};
+
 /**
  * Ethernet device information
  */
@@ -1029,6 +1050,10 @@ 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. */
+	/** Rx parameter recommendations */
+	struct rte_eth_dev_portconf default_rxportconf;
+	/** Tx parameter recommendations */
+	struct rte_eth_dev_portconf default_txportconf;
 };
 
 /**
-- 
2.9.5

[PATCH v6 2/4] net/e1000: add TxRx tuning parameters

[Remy Horton]

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>
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 drivers/net/e1000/em_ethdev.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/e1000/em_ethdev.c b/drivers/net/e1000/em_ethdev.c
index f727382..2035ea8 100644
--- a/drivers/net/e1000/em_ethdev.c
+++ b/drivers/net/e1000/em_ethdev.c
@@ -1100,6 +1100,12 @@ eth_em_infos_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 	dev_info->speed_capa = ETH_LINK_SPEED_10M_HD | ETH_LINK_SPEED_10M |
 			ETH_LINK_SPEED_100M_HD | ETH_LINK_SPEED_100M |
 			ETH_LINK_SPEED_1G;
+
+	/* Preferred queue parameters */
+	dev_info->default_rxportconf.nb_queues = 1;
+	dev_info->default_txportconf.nb_queues = 1;
+	dev_info->default_txportconf.ring_size = 256;
+	dev_info->default_rxportconf.ring_size = 256;
 }
 
 /* return 0 means link status changed, -1 means not changed */
-- 
2.9.5

[PATCH v6 3/4] net/i40e: add TxRx tuning parameters

[Remy Horton]

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>
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
---
 drivers/net/i40e/i40e_ethdev.c | 33 ++++++++++++++++++++++++++++++---
 1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/drivers/net/i40e/i40e_ethdev.c b/drivers/net/i40e/i40e_ethdev.c
index d0bf4e3..acf1f99 100644
--- a/drivers/net/i40e/i40e_ethdev.c
+++ b/drivers/net/i40e/i40e_ethdev.c
@@ -3291,15 +3291,42 @@ i40e_dev_info_get(struct rte_eth_dev *dev, struct rte_eth_dev_info *dev_info)
 		dev_info->max_tx_queues += dev_info->vmdq_queue_num;
 	}
 
-	if (I40E_PHY_TYPE_SUPPORT_40G(hw->phy.phy_types))
+	if (I40E_PHY_TYPE_SUPPORT_40G(hw->phy.phy_types)) {
 		/* For XL710 */
 		dev_info->speed_capa = ETH_LINK_SPEED_40G;
-	else if (I40E_PHY_TYPE_SUPPORT_25G(hw->phy.phy_types))
+		dev_info->default_rxportconf.nb_queues = 2;
+		dev_info->default_txportconf.nb_queues = 2;
+		if (dev->data->nb_rx_queues == 1)
+			dev_info->default_rxportconf.ring_size = 2048;
+		else
+			dev_info->default_rxportconf.ring_size = 1024;
+		if (dev->data->nb_tx_queues == 1)
+			dev_info->default_txportconf.ring_size = 1024;
+		else
+			dev_info->default_txportconf.ring_size = 512;
+
+	} else if (I40E_PHY_TYPE_SUPPORT_25G(hw->phy.phy_types)) {
 		/* For XXV710 */
 		dev_info->speed_capa = ETH_LINK_SPEED_25G;
-	else
+		dev_info->default_rxportconf.nb_queues = 1;
+		dev_info->default_txportconf.nb_queues = 1;
+		dev_info->default_rxportconf.ring_size = 256;
+		dev_info->default_txportconf.ring_size = 256;
+	} else {
 		/* For X710 */
 		dev_info->speed_capa = ETH_LINK_SPEED_1G | ETH_LINK_SPEED_10G;
+		dev_info->default_rxportconf.nb_queues = 1;
+		dev_info->default_txportconf.nb_queues = 1;
+		if (dev->data->dev_conf.link_speeds & ETH_LINK_SPEED_10G) {
+			dev_info->default_rxportconf.ring_size = 512;
+			dev_info->default_txportconf.ring_size = 256;
+		} else {
+			dev_info->default_rxportconf.ring_size = 256;
+			dev_info->default_txportconf.ring_size = 256;
+		}
+	}
+	dev_info->default_rxportconf.burst_size = 32;
+	dev_info->default_txportconf.burst_size = 32;
 }
 
 static int
-- 
2.9.5

[PATCH v6 4/4] testpmd: make use of per-PMD TxRx parameters

[Remy Horton]

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 testpmd to make use of per-PMD tuned parameter values.

Signed-off-by: Remy Horton <remy.horton@intel.com>
Acked-by: Shreyansh Jain <shreyansh.jain@nxp.com>
---
 app/test-pmd/cmdline.c                | 31 +++++++++++++++++++++++++---
 app/test-pmd/parameters.c             | 38 ++++++++++++++++++++++++++++++-----
 app/test-pmd/testpmd.c                |  5 +++--
 doc/guides/testpmd_app_ug/run_app.rst |  4 +++-
 4 files changed, 67 insertions(+), 11 deletions(-)

diff --git a/app/test-pmd/cmdline.c b/app/test-pmd/cmdline.c
index 40b31ad..0914425 100644
--- a/app/test-pmd/cmdline.c
+++ b/app/test-pmd/cmdline.c
@@ -2599,6 +2599,8 @@ cmd_config_burst_parsed(void *parsed_result,
 			__attribute__((unused)) void *data)
 {
 	struct cmd_config_burst *res = parsed_result;
+	struct rte_eth_dev_info dev_info;
+	uint16_t rec_nb_pkts;
 
 	if (!all_ports_stopped()) {
 		printf("Please stop all ports first\n");
@@ -2606,11 +2608,34 @@ cmd_config_burst_parsed(void *parsed_result,
 	}
 
 	if (!strcmp(res->name, "burst")) {
-		if (res->value < 1 || res->value > MAX_PKT_BURST) {
+		if (res->value == 0) {
+			/* If user gives a value of zero, query the PMD for
+			 * its recommended Rx burst size. Testpmd uses a single
+			 * size for all ports, so assume all ports are the same
+			 * NIC model and use the values from Port 0.
+			 */
+			rte_eth_dev_info_get(0, &dev_info);
+			rec_nb_pkts = dev_info.default_rxportconf.burst_size;
+
+			if (rec_nb_pkts == 0) {
+				printf("PMD does not recommend a burst size.\n"
+					"User provided value must be between"
+					" 1 and %d\n", MAX_PKT_BURST);
+				return;
+			} else if (rec_nb_pkts > MAX_PKT_BURST) {
+				printf("PMD recommended burst size of %d"
+					" exceeds maximum value of %d\n",
+					rec_nb_pkts, MAX_PKT_BURST);
+				return;
+			}
+			printf("Using PMD-provided burst value of %d\n",
+				rec_nb_pkts);
+			nb_pkt_per_burst = rec_nb_pkts;
+		} else if (res->value > MAX_PKT_BURST) {
 			printf("burst must be >= 1 && <= %d\n", MAX_PKT_BURST);
 			return;
-		}
-		nb_pkt_per_burst = res->value;
+		} else
+			nb_pkt_per_burst = res->value;
 	} else {
 		printf("Unknown parameter\n");
 		return;
diff --git a/app/test-pmd/parameters.c b/app/test-pmd/parameters.c
index 2192bdc..cb6a229 100644
--- a/app/test-pmd/parameters.c
+++ b/app/test-pmd/parameters.c
@@ -544,6 +544,8 @@ launch_args_parse(int argc, char** argv)
 	/* Default offloads for all ports. */
 	uint64_t rx_offloads = rx_mode.offloads;
 	uint64_t tx_offloads = tx_mode.offloads;
+	struct rte_eth_dev_info dev_info;
+	uint16_t rec_nb_pkts;
 
 	static struct option lgopts[] = {
 		{ "help",			0, 0, 0 },
@@ -947,12 +949,38 @@ launch_args_parse(int argc, char** argv)
 			}
 			if (!strcmp(lgopts[opt_idx].name, "burst")) {
 				n = atoi(optarg);
-				if ((n >= 1) && (n <= MAX_PKT_BURST))
-					nb_pkt_per_burst = (uint16_t) n;
-				else
+				if (n == 0) {
+					/* A burst size of zero means that the
+					 * PMD should be queried for
+					 * recommended Rx burst size. Since
+					 * testpmd uses a single size for all
+					 * ports, port 0 is queried for the
+					 * value, on the assumption that all
+					 * ports are of the same NIC model.
+					 */
+					rte_eth_dev_info_get(0, &dev_info);
+					rec_nb_pkts = dev_info
+						.default_rxportconf.burst_size;
+
+					if (rec_nb_pkts == 0)
+						rte_exit(EXIT_FAILURE,
+							"PMD does not recommend a burst size. "
+							"Provided value must be between "
+							"1 and %d\n", MAX_PKT_BURST);
+					else if (rec_nb_pkts > MAX_PKT_BURST)
+						rte_exit(EXIT_FAILURE,
+							"PMD recommended burst size of %d"
+							" exceeds maximum value of %d\n",
+							rec_nb_pkts, MAX_PKT_BURST);
+					printf("Using PMD-provided burst value of %d\n",
+						rec_nb_pkts);
+					nb_pkt_per_burst = rec_nb_pkts;
+				} else if (n > MAX_PKT_BURST)
 					rte_exit(EXIT_FAILURE,
-						 "burst must >= 1 and <= %d]",
-						 MAX_PKT_BURST);
+						"burst must be between1 and %d\n",
+						MAX_PKT_BURST);
+				else
+					nb_pkt_per_burst = (uint16_t) n;
 			}
 			if (!strcmp(lgopts[opt_idx].name, "mbcache")) {
 				n = atoi(optarg);
diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 4c0e258..82eb197 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -210,9 +210,10 @@ queueid_t nb_txq = 1; /**< Number of TX queues per port. */
 
 /*
  * Configurable number of RX/TX ring descriptors.
+ * Defaults are supplied by drivers via ethdev.
  */
-#define RTE_TEST_RX_DESC_DEFAULT 1024
-#define RTE_TEST_TX_DESC_DEFAULT 1024
+#define RTE_TEST_RX_DESC_DEFAULT 0
+#define RTE_TEST_TX_DESC_DEFAULT 0
 uint16_t nb_rxd = RTE_TEST_RX_DESC_DEFAULT; /**< Number of RX descriptors. */
 uint16_t nb_txd = RTE_TEST_TX_DESC_DEFAULT; /**< Number of TX descriptors. */
 
diff --git a/doc/guides/testpmd_app_ug/run_app.rst b/doc/guides/testpmd_app_ug/run_app.rst
index 1fd5395..f876f95 100644
--- a/doc/guides/testpmd_app_ug/run_app.rst
+++ b/doc/guides/testpmd_app_ug/run_app.rst
@@ -372,7 +372,9 @@ The commandline options are:
 *   ``--burst=N``
 
     Set the number of packets per burst to N, where 1 <= N <= 512.
-    The default value is 16.
+    The default value is 32.
+    If set to 0, driver default is used if defined. Else, if driver
+    default is not defined, default of 32 is used.
 
 *   ``--mbcache=N``
 
-- 
2.9.5

Re: [PATCH v6 0/4] ethdev: add per-PMD tuning of RxTx parmeters

[Thomas Monjalon]

10/04/2018 11:43, Remy Horton:
> Changes in v6:
> * Updated/corrected testpmd documentation
> * Carried forward acks/review

You forgot my ack on ethdev patch.

Re: [PATCH v6 0/4] ethdev: add per-PMD tuning of RxTx parmeters

[Ferruh Yigit]

On 4/10/2018 10:43 AM, 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 patchset allows individual
> PMDs to specify their preferred parameter values, and if so indicated by an
> application, for them to be used automatically by the ethdev layer.
> 
> rte_eth_dev_configure() has been changed so that specifying zero for both
> nb_rx_q AND nb_tx_q causes it to use driver preferred values, and if these
> are not available, falls back to EAL defaults. Setting one (but not both)
> to zero does not cause the use of defaults, as having one of them zeroed is
> a valid setup.
> 
> This patchset includes per-PMD values for e1000 and i40e but it is expected
> that subsequent patchsets will cover other PMDs. A deprecation notice
> covering the API/ABI change is in place.
> 
> Changes in v6:
> * Updated/corrected testpmd documentation
> * Carried forward acks/review
> * Rebased to d218a4d060de
> 
> Changes in v5:
> * uint_16_t corrected to uint16_t
> 
> Changes in v4:
> * Added API/ABI change documentation
> * Rebased to 78f5a2e93d74
> 
> Changes in v3:
> * Changed formatting around new rte_eth_dev_info fields
> * Added Doxygen documentation to struct rte_eth_dev_portconf
> * Testpmd "port config all burst 0" and --burst=0 uses PMD 
>   Rx burst recommendations.
> * Added to release notes
> * Rebased to 8ea081f38161
> 
> Changes in v2:
> * Rebased to master
> * Removed fallback values from rte_eth_dev_info_get()
> * Added fallback values to rte_rte_[rt]x_queue_setup()
> * Added fallback values to rte_eth_dev_configure()
> * Corrected comment
> * Removed deprecation notice
> * Split RX and Tx into seperate structures
> * Changed parameter names
> 
> 
> Remy Horton (4):
>   ethdev: add support for PMD-tuned Tx/Rx parameters
>   net/e1000: add TxRx tuning parameters
>   net/i40e: add TxRx tuning parameters
>   testpmd: make use of per-PMD TxRx parameters

Series applied to dpdk-next-net/master, thanks.

(Thomas' ack added into ethdev patch)