Re: [PATCH 3/5] testpmd: Change rxfreet default to 32

On Fri, Sep 19, 2014 at 09:18:26AM +0000, Richardson, Bruce wrote:

quoted

-----Original Message-----
From: Neil Horman [mailto:nhorman-2XuSBdqkA4R54TAoqtyWWQ@public.gmane.org]
Sent: Thursday, September 18, 2014 7:09 PM
To: Thomas Monjalon
Cc: Richardson, Bruce; dev-VfR2kkLFssw@public.gmane.org
Subject: Re: [dpdk-dev] [PATCH 3/5] testpmd: Change rxfreet default to 32

On Thu, Sep 18, 2014 at 07:13:52PM +0200, Thomas Monjalon wrote:

quoted

2014-09-18 15:53, Richardson, Bruce:

quoted

quoted

quoted

--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c

@@ -225,7 +225,7 @@ struct rte_eth_thresh tx_thresh = {
 /*
  * Configurable value of RX free threshold.
  */
-uint16_t rx_free_thresh = 0; /* Immediately free RX descriptors by

default. */

quoted

quoted

quoted

quoted

+uint16_t rx_free_thresh = 32; /* Refill RX descriptors once every 32

packets

quoted

quoted

quoted

*/

quoted

Why 32?  Was that an experimentally determined value?
Does it hold true for all PMD's?

This is primarily for the ixgbe PMD, which is right now the most
highly tuned driver, but it works fine for all other ones too,
as far as I'm aware.

Yes, you are changing this value for all PMDs but you're targetting
only one.
These thresholds are dependent of the PMD implementation. There's
something wrong here.

I agree. Its fine to do this, but it does seem like the sample application
should document why it does this and make note of the fact that other PMDs
may
have a separate optimal value.

quoted

quoted

Basically, this is the minimum setting needed to enable either the
bulk alloc or vector RX routines inside the ixgbe driver, so it's
best made the default for that reason. Please see
"check_rx_burst_bulk_alloc_preconditions()" in ixgbe_rxtx.c, and
RX function assignment logic in "ixgbe_dev_rx_queue_setup()" in
the same file.

Since this parameter is so important, it could be a default value somewhere.

I think we should split generic tuning parameters and tuning parameters
related to driver implementation or specific hardware.
Then we should provide some good default values for each of them.
At last, if needed, applications should be able to easily tune the
pmd-specific parameters.

I like this idea.  I've not got an idea of how much work it is to do so, but in
principle it makes sense.

Perhaps for the immediate need, since rte_eth_rx_queue_setup allows the
config
struct to get passed directly to PMDs, we can create a reserved value
RTE_ETH_RX_FREE_THRESH_OPTIMAL, that instructs the pmd to select
whatever
threshold is optimal for its own hardware?

Neil

Actually, looking at the code, I would suggest a couple of options, some of which may be used together.
	1) we make NULL a valid value for the rxconf structure parameter to rte_eth_rx_queue_setup. There is little information in it that should really need to be passed in by applications to the drivers, and that would allow the drivers to be completely free to select the best options for their own operation. 
	2) As a companion to that (or as an alternative), we could also allow each driver to provide its own functions for rte_eth_get_rxconf_default, and rte_eth_get_txconf_default, to be used by applications that want to use known-good values for thresholds but also want to tweak one of the other values e.g. for rx, set the drop_en bit, and for tx set the txqflags to disable offloads.
	3) Lastly, we could also consider removing the threshold and other not-generally-used values from the rxconf and txconf structures and make those removed fields completely driver-set values. Optionally, we could provide an alternate API to tune them, but I don't really see this being useful in most cases, and I'd probably omit it unless someone can prove a need for such APIs.

These all sound fairly reasonable to me.
Neil

Regards,
/Bruce