[dpdk-dev,v2,3/5] testpmd: Change rxfreet default to 32

To improve performance by using bulk alloc or vectored RX routines, we
need to set rx free threshold (rxfreet) value to 32, so make this the
testpmd default.

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

The difference in IO performance for testpmd when called without any
optional parameters, and using 10G NICs using the ixgbe driver, can be
significant - approx 25% or more.

Updates in V2:
* Updated commit message with additional details

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test-pmd/testpmd.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
  

On Tue, Sep 23, 2014 at 12:08:15PM +0100, Bruce Richardson wrote:
> To improve performance by using bulk alloc or vectored RX routines, we
> need to set rx free threshold (rxfreet) value to 32, so make this the
> testpmd default.
> 
> Thirty-two 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.
> 
> The difference in IO performance for testpmd when called without any
> optional parameters, and using 10G NICs using the ixgbe driver, can be
> significant - approx 25% or more.
> 
> Updates in V2:
> * Updated commit message with additional details
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  app/test-pmd/testpmd.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 9f6cdc4..f76406f 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -225,7 +225,9 @@ 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,
> +		This setting is needed for ixgbe to enable bulk alloc or vector
> +		receive functionality. */

I thought we were talking about making this a pmd private selectable item, or
allowing a reserved "let the pmd decide" setting.  Or are we saving that for a
later time?

Neil

>  
>  /*
>   * Configurable value of RX drop enable.
> -- 
> 1.9.3
> 
>
  

> -----Original Message-----
> From: Neil Horman [mailto:nhorman@tuxdriver.com]
> Sent: Tuesday, September 23, 2014 6:03 PM
> To: Richardson, Bruce
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v2 3/5] testpmd: Change rxfreet default to 32
> 
> On Tue, Sep 23, 2014 at 12:08:15PM +0100, Bruce Richardson wrote:
> > To improve performance by using bulk alloc or vectored RX routines, we
> > need to set rx free threshold (rxfreet) value to 32, so make this the
> > testpmd default.
> >
> > Thirty-two 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.
> >
> > The difference in IO performance for testpmd when called without any
> > optional parameters, and using 10G NICs using the ixgbe driver, can be
> > significant - approx 25% or more.
> >
> > Updates in V2:
> > * Updated commit message with additional details
> >
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> >  app/test-pmd/testpmd.c | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> > index 9f6cdc4..f76406f 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -225,7 +225,9 @@ 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,
> > +		This setting is needed for ixgbe to enable bulk alloc or vector
> > +		receive functionality. */
> 
> I thought we were talking about making this a pmd private selectable item, or
> allowing a reserved "let the pmd decide" setting.  Or are we saving that for a
> later time?
> 

Yes, we are looking at that - and hopefully we can also get a patch for that in for our next release. However, I've left this patch in just in case that doesn't actually happen, as the performance improvements for 10G are just too good to leave aside for the sake of a 1-line change. Ideally, I'd like this go to in, and then be replaced by a "proper" fix.

/Bruce

> Neil
> 
> >
> >  /*
> >   * Configurable value of RX drop enable.
> > --
> > 1.9.3
> >
> >
  

On Wed, Sep 24, 2014 at 09:03:20AM +0000, Richardson, Bruce wrote:
> > -----Original Message-----
> > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > Sent: Tuesday, September 23, 2014 6:03 PM
> > To: Richardson, Bruce
> > Cc: dev@dpdk.org
> > Subject: Re: [dpdk-dev] [PATCH v2 3/5] testpmd: Change rxfreet default to 32
> > 
> > On Tue, Sep 23, 2014 at 12:08:15PM +0100, Bruce Richardson wrote:
> > > To improve performance by using bulk alloc or vectored RX routines, we
> > > need to set rx free threshold (rxfreet) value to 32, so make this the
> > > testpmd default.
> > >
> > > Thirty-two 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.
> > >
> > > The difference in IO performance for testpmd when called without any
> > > optional parameters, and using 10G NICs using the ixgbe driver, can be
> > > significant - approx 25% or more.
> > >
> > > Updates in V2:
> > > * Updated commit message with additional details
> > >
> > > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > > ---
> > >  app/test-pmd/testpmd.c | 4 +++-
> > >  1 file changed, 3 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> > > index 9f6cdc4..f76406f 100644
> > > --- a/app/test-pmd/testpmd.c
> > > +++ b/app/test-pmd/testpmd.c
> > > @@ -225,7 +225,9 @@ 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,
> > > +		This setting is needed for ixgbe to enable bulk alloc or vector
> > > +		receive functionality. */
> > 
> > I thought we were talking about making this a pmd private selectable item, or
> > allowing a reserved "let the pmd decide" setting.  Or are we saving that for a
> > later time?
> > 
> 
> Yes, we are looking at that - and hopefully we can also get a patch for that in for our next release. However, I've left this patch in just in case that doesn't actually happen, as the performance improvements for 10G are just too good to leave aside for the sake of a 1-line change. Ideally, I'd like this go to in, and then be replaced by a "proper" fix.
> 
Ok, thanks
Neil

> /Bruce
> 
> > Neil
> > 
> > >
> > >  /*
> > >   * Configurable value of RX drop enable.
> > > --
> > > 1.9.3
> > >
> > >
>
  

Hi Bruce,

2014-09-24 09:03, Richardson, Bruce:
> From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > On Tue, Sep 23, 2014 at 12:08:15PM +0100, Bruce Richardson wrote:
> > > --- a/app/test-pmd/testpmd.c
> > > +++ b/app/test-pmd/testpmd.c
> > > @@ -225,7 +225,9 @@ 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,
> > > +		This setting is needed for ixgbe to enable bulk alloc or vector
> > > +		receive functionality. */
> > 
> > I thought we were talking about making this a pmd private selectable item,
> > or allowing a reserved "let the pmd decide" setting.  Or are we saving
> > that for a later time?
> 
> Yes, we are looking at that - and hopefully we can also get a patch for that
> in for our next release. However, I've left this patch in just in case that
> doesn't actually happen, as the performance improvements for 10G are just
> too good to leave aside for the sake of a 1-line change. Ideally, I'd like
> this go to in, and then be replaced by a "proper" fix.

Now the patch for PMD defaults is integrated:
	http://dpdk.org/ml/archives/dev/2014-October/006511.html
Are you working on getting these defaults in testpmd?

Thanks
  

On Fri, Nov 07, 2014 at 01:30:53PM +0100, Thomas Monjalon wrote:
> Hi Bruce,
> 
> 2014-09-24 09:03, Richardson, Bruce:
> > From: Neil Horman [mailto:nhorman@tuxdriver.com]
> > > On Tue, Sep 23, 2014 at 12:08:15PM +0100, Bruce Richardson wrote:
> > > > --- a/app/test-pmd/testpmd.c
> > > > +++ b/app/test-pmd/testpmd.c
> > > > @@ -225,7 +225,9 @@ 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,
> > > > +		This setting is needed for ixgbe to enable bulk alloc or vector
> > > > +		receive functionality. */
> > > 
> > > I thought we were talking about making this a pmd private selectable item,
> > > or allowing a reserved "let the pmd decide" setting.  Or are we saving
> > > that for a later time?
> > 
> > Yes, we are looking at that - and hopefully we can also get a patch for that
> > in for our next release. However, I've left this patch in just in case that
> > doesn't actually happen, as the performance improvements for 10G are just
> > too good to leave aside for the sake of a 1-line change. Ideally, I'd like
> > this go to in, and then be replaced by a "proper" fix.
> 
> Now the patch for PMD defaults is integrated:
> 	http://dpdk.org/ml/archives/dev/2014-October/006511.html
> Are you working on getting these defaults in testpmd?
> 

Not at the minute, as I'm busy on other things. If I find time I can look at it,
though, if nobody else volunteers to do so first.

/Bruce


> Thanks
> -- 
> Thomas