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

[Neil Horman <nhorman@tuxdriver.com>]

On Fri, Sep 19, 2014 at 09:18:26AM +0000, Richardson, Bruce wrote:
> > -----Original Message-----
> > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > Sent: Thursday, September 18, 2014 7:09 PM
> > To: Thomas Monjalon
> > Cc: Richardson, Bruce; dev@dpdk.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:
> > > 2014-09-18 15:53, Richardson, Bruce:
> > > > > > --- 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. */
> > > > > > +uint16_t rx_free_thresh = 32; /* Refill RX descriptors once every 32
> > packets
> > > > > */
> > > > > >
> > > > >
> > > > > 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.
> > 
> > > > 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
>