diff mbox

[dpdk-dev] net/i40e: update tx_free_threshold to improve zero copy performance

Message ID 20180412053226.295226-1-junjie.j.chen@intel.com (mailing list archive)
State Superseded, archived
Delegated to: Helin Zhang
Headers

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

junjie.j.chen@intel.com April 12, 2018, 5:32 a.m. UTC 
  From: "Chen, Junjie" <junjie.j.chen@intel.com>

When vhost backend works in dequeue zero copy mode, nic locks virtio's
buffer until there is less or equal than tx_free_threshold buffer remain
and then free number of tx burst buffer. This causes packets drop in
virtio side and impacts zero copy performance. So we need to increase
the tx_free_threshold to let nic free virtio's buffer as soon as possible.
Also we keep the upper limit to tx max burst size to ensure least
performance impact on non zero copy.

Signed-off-by: Chen, Junjie <junjie.j.chen@intel.com>
---
 drivers/net/i40e/i40e_rxtx.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Ananyev, Konstantin April 12, 2018, 11:51 a.m. UTC | #1 
> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Junjie Chen
> Sent: Thursday, April 12, 2018 6:32 AM
> To: Xing, Beilei <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Chen, Junjie J <junjie.j.chen@intel.com>; Chen@dpdk.org
> Subject: [dpdk-dev] [PATCH] net/i40e: update tx_free_threshold to improve zero copy performance
> 
> From: "Chen, Junjie" <junjie.j.chen@intel.com>
> 
> When vhost backend works in dequeue zero copy mode, nic locks virtio's
> buffer until there is less or equal than tx_free_threshold buffer remain
> and then free number of tx burst buffer. This causes packets drop in
> virtio side and impacts zero copy performance. So we need to increase
> the tx_free_threshold to let nic free virtio's buffer as soon as possible.
> Also we keep the upper limit to tx max burst size to ensure least
> performance impact on non zero copy.

Ok but why vhost app can't just use tx_queue_setup() to specify desired value for
tx_free_thresh?
Why instead we have to modify PMD to satisfy needs of one app?
Konstantin

> 
> Signed-off-by: Chen, Junjie <junjie.j.chen@intel.com>
> ---
>  drivers/net/i40e/i40e_rxtx.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
> index 56a854cec..d9569bdc9 100644
> --- a/drivers/net/i40e/i40e_rxtx.c
> +++ b/drivers/net/i40e/i40e_rxtx.c
> @@ -2039,6 +2039,8 @@ i40e_dev_tx_queue_setup(struct rte_eth_dev *dev,
>  		tx_conf->tx_rs_thresh : DEFAULT_TX_RS_THRESH);
>  	tx_free_thresh = (uint16_t)((tx_conf->tx_free_thresh) ?
>  		tx_conf->tx_free_thresh : DEFAULT_TX_FREE_THRESH);
> +	if (tx_free_thresh < nb_desc - I40E_TX_MAX_BURST)
> +		tx_free_thresh = nb_desc - I40E_TX_MAX_BURST;
>  	if (tx_rs_thresh >= (nb_desc - 2)) {
>  		PMD_INIT_LOG(ERR, "tx_rs_thresh must be less than the "
>  			     "number of TX descriptors minus 2. "
> --
> 2.16.0
Qi Zhang April 12, 2018, 12:20 p.m. UTC | #2 
Hi Junjie:

> -----Original Message-----
> From: Ananyev, Konstantin
> Sent: Thursday, April 12, 2018 7:52 PM
> To: Chen, Junjie J <junjie.j.chen@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; Chen, Junjie J <junjie.j.chen@intel.com>; Chen@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH] net/i40e: update tx_free_threshold to
> improve zero copy performance
> 
> 
> 
> > -----Original Message-----
> > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Junjie Chen
> > Sent: Thursday, April 12, 2018 6:32 AM
> > To: Xing, Beilei <beilei.xing@intel.com>; Zhang, Qi Z
> > <qi.z.zhang@intel.com>
> > Cc: dev@dpdk.org; Chen, Junjie J <junjie.j.chen@intel.com>;
> > Chen@dpdk.org
> > Subject: [dpdk-dev] [PATCH] net/i40e: update tx_free_threshold to
> > improve zero copy performance
> >
> > From: "Chen, Junjie" <junjie.j.chen@intel.com>
> >
> > When vhost backend works in dequeue zero copy mode, nic locks virtio's
> > buffer until there is less or equal than tx_free_threshold buffer
> > remain and then free number of tx burst buffer. This causes packets
> > drop in virtio side and impacts zero copy performance. So we need to
> > increase the tx_free_threshold to let nic free virtio's buffer as soon as
> possible.
> > Also we keep the upper limit to tx max burst size to ensure least
> > performance impact on non zero copy.
> 
> Ok but why vhost app can't just use tx_queue_setup() to specify desired value
> for tx_free_thresh?
> Why instead we have to modify PMD to satisfy needs of one app?
> Konstantin

I think the commit log could include the explanation that this change is proved not impact 
driver's performance and it reduce total memory be locked by PMD Tx, so basically it benefit
application that share the same mem pool overall, vhost dequeue zero copy is one of the example.

> 
> >
> > Signed-off-by: Chen, Junjie <junjie.j.chen@intel.com>
> > ---
> >  drivers/net/i40e/i40e_rxtx.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/net/i40e/i40e_rxtx.c
> > b/drivers/net/i40e/i40e_rxtx.c index 56a854cec..d9569bdc9 100644
> > --- a/drivers/net/i40e/i40e_rxtx.c
> > +++ b/drivers/net/i40e/i40e_rxtx.c
> > @@ -2039,6 +2039,8 @@ i40e_dev_tx_queue_setup(struct rte_eth_dev
> *dev,
> >  		tx_conf->tx_rs_thresh : DEFAULT_TX_RS_THRESH);
> >  	tx_free_thresh = (uint16_t)((tx_conf->tx_free_thresh) ?
> >  		tx_conf->tx_free_thresh : DEFAULT_TX_FREE_THRESH);
> > +	if (tx_free_thresh < nb_desc - I40E_TX_MAX_BURST)
> > +		tx_free_thresh = nb_desc - I40E_TX_MAX_BURST;

I think we'd better still allow application to set tx_free_thresh, since a small tx_free_thresh may still have benefit to let driver handle the first strike after device restarted
So, nb_desc - I40E_TX_MAX_BURST can only be set when tx_conf->tx_rs_thresh = 0

Regards
Qi

> >  	if (tx_rs_thresh >= (nb_desc - 2)) {
> >  		PMD_INIT_LOG(ERR, "tx_rs_thresh must be less than the "
> >  			     "number of TX descriptors minus 2. "
> > --
> > 2.16.0
Bruce Richardson April 12, 2018, 1:12 p.m. UTC | #3 
On Thu, Apr 12, 2018 at 12:20:07PM +0000, Zhang, Qi Z wrote:
> Hi Junjie:
> 
> > -----Original Message-----
> > From: Ananyev, Konstantin
> > Sent: Thursday, April 12, 2018 7:52 PM
> > To: Chen, Junjie J <junjie.j.chen@intel.com>; Xing, Beilei
> > <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> > Cc: dev@dpdk.org; Chen, Junjie J <junjie.j.chen@intel.com>; Chen@dpdk.org
> > Subject: RE: [dpdk-dev] [PATCH] net/i40e: update tx_free_threshold to
> > improve zero copy performance
> > 
> > 
> > 
> > > -----Original Message-----
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Junjie Chen
> > > Sent: Thursday, April 12, 2018 6:32 AM
> > > To: Xing, Beilei <beilei.xing@intel.com>; Zhang, Qi Z
> > > <qi.z.zhang@intel.com>
> > > Cc: dev@dpdk.org; Chen, Junjie J <junjie.j.chen@intel.com>;
> > > Chen@dpdk.org
> > > Subject: [dpdk-dev] [PATCH] net/i40e: update tx_free_threshold to
> > > improve zero copy performance
> > >
> > > From: "Chen, Junjie" <junjie.j.chen@intel.com>
> > >
> > > When vhost backend works in dequeue zero copy mode, nic locks virtio's
> > > buffer until there is less or equal than tx_free_threshold buffer
> > > remain and then free number of tx burst buffer. This causes packets
> > > drop in virtio side and impacts zero copy performance. So we need to
> > > increase the tx_free_threshold to let nic free virtio's buffer as soon as
> > possible.
> > > Also we keep the upper limit to tx max burst size to ensure least
> > > performance impact on non zero copy.
> > 
> > Ok but why vhost app can't just use tx_queue_setup() to specify desired value
> > for tx_free_thresh?
> > Why instead we have to modify PMD to satisfy needs of one app?
> > Konstantin
> 
> I think the commit log could include the explanation that this change is proved not impact 
> driver's performance and it reduce total memory be locked by PMD Tx, so basically it benefit
> application that share the same mem pool overall, vhost dequeue zero copy is one of the example.
> 
> > 
> > >
> > > Signed-off-by: Chen, Junjie <junjie.j.chen@intel.com>
> > > ---
> > >  drivers/net/i40e/i40e_rxtx.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/net/i40e/i40e_rxtx.c
> > > b/drivers/net/i40e/i40e_rxtx.c index 56a854cec..d9569bdc9 100644
> > > --- a/drivers/net/i40e/i40e_rxtx.c
> > > +++ b/drivers/net/i40e/i40e_rxtx.c
> > > @@ -2039,6 +2039,8 @@ i40e_dev_tx_queue_setup(struct rte_eth_dev
> > *dev,
> > >  		tx_conf->tx_rs_thresh : DEFAULT_TX_RS_THRESH);
> > >  	tx_free_thresh = (uint16_t)((tx_conf->tx_free_thresh) ?
> > >  		tx_conf->tx_free_thresh : DEFAULT_TX_FREE_THRESH);
> > > +	if (tx_free_thresh < nb_desc - I40E_TX_MAX_BURST)
> > > +		tx_free_thresh = nb_desc - I40E_TX_MAX_BURST;
> 
> I think we'd better still allow application to set tx_free_thresh, since a small tx_free_thresh may still have benefit to let driver handle the first strike after device restarted
> So, nb_desc - I40E_TX_MAX_BURST can only be set when tx_conf->tx_rs_thresh = 0
> 
> Regards
> Qi
> 
+1 for just changing in this case.

/Bruce
Ananyev, Konstantin April 12, 2018, 1:56 p.m. UTC | #4 
> -----Original Message-----
> From: Richardson, Bruce
> Sent: Thursday, April 12, 2018 2:12 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Chen, Junjie J <junjie.j.chen@intel.com>; Xing, Beilei <beilei.xing@intel.com>;
> dev@dpdk.org; Chen@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] net/i40e: update tx_free_threshold to improve zero copy performance
> 
> On Thu, Apr 12, 2018 at 12:20:07PM +0000, Zhang, Qi Z wrote:
> > Hi Junjie:
> >
> > > -----Original Message-----
> > > From: Ananyev, Konstantin
> > > Sent: Thursday, April 12, 2018 7:52 PM
> > > To: Chen, Junjie J <junjie.j.chen@intel.com>; Xing, Beilei
> > > <beilei.xing@intel.com>; Zhang, Qi Z <qi.z.zhang@intel.com>
> > > Cc: dev@dpdk.org; Chen, Junjie J <junjie.j.chen@intel.com>; Chen@dpdk.org
> > > Subject: RE: [dpdk-dev] [PATCH] net/i40e: update tx_free_threshold to
> > > improve zero copy performance
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Junjie Chen
> > > > Sent: Thursday, April 12, 2018 6:32 AM
> > > > To: Xing, Beilei <beilei.xing@intel.com>; Zhang, Qi Z
> > > > <qi.z.zhang@intel.com>
> > > > Cc: dev@dpdk.org; Chen, Junjie J <junjie.j.chen@intel.com>;
> > > > Chen@dpdk.org
> > > > Subject: [dpdk-dev] [PATCH] net/i40e: update tx_free_threshold to
> > > > improve zero copy performance
> > > >
> > > > From: "Chen, Junjie" <junjie.j.chen@intel.com>
> > > >
> > > > When vhost backend works in dequeue zero copy mode, nic locks virtio's
> > > > buffer until there is less or equal than tx_free_threshold buffer
> > > > remain and then free number of tx burst buffer. This causes packets
> > > > drop in virtio side and impacts zero copy performance. So we need to
> > > > increase the tx_free_threshold to let nic free virtio's buffer as soon as
> > > possible.
> > > > Also we keep the upper limit to tx max burst size to ensure least
> > > > performance impact on non zero copy.
> > >
> > > Ok but why vhost app can't just use tx_queue_setup() to specify desired value
> > > for tx_free_thresh?
> > > Why instead we have to modify PMD to satisfy needs of one app?
> > > Konstantin
> >
> > I think the commit log could include the explanation that this change is proved not impact
> > driver's performance and it reduce total memory be locked by PMD Tx, so basically it benefit
> > application that share the same mem pool overall, vhost dequeue zero copy is one of the example.
> >
> > >
> > > >
> > > > Signed-off-by: Chen, Junjie <junjie.j.chen@intel.com>
> > > > ---
> > > >  drivers/net/i40e/i40e_rxtx.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > >
> > > > diff --git a/drivers/net/i40e/i40e_rxtx.c
> > > > b/drivers/net/i40e/i40e_rxtx.c index 56a854cec..d9569bdc9 100644
> > > > --- a/drivers/net/i40e/i40e_rxtx.c
> > > > +++ b/drivers/net/i40e/i40e_rxtx.c
> > > > @@ -2039,6 +2039,8 @@ i40e_dev_tx_queue_setup(struct rte_eth_dev
> > > *dev,
> > > >  		tx_conf->tx_rs_thresh : DEFAULT_TX_RS_THRESH);
> > > >  	tx_free_thresh = (uint16_t)((tx_conf->tx_free_thresh) ?
> > > >  		tx_conf->tx_free_thresh : DEFAULT_TX_FREE_THRESH);
> > > > +	if (tx_free_thresh < nb_desc - I40E_TX_MAX_BURST)
> > > > +		tx_free_thresh = nb_desc - I40E_TX_MAX_BURST;
> >
> > I think we'd better still allow application to set tx_free_thresh, since a small tx_free_thresh may still have benefit to let driver handle the
> first strike after device restarted
> > So, nb_desc - I40E_TX_MAX_BURST can only be set when tx_conf->tx_rs_thresh = 0
> >
> > Regards
> > Qi
> >
> +1 for just changing in this case.
> 
Basically you suggest to change DEFAULT_TX_FREE_THRESH.
Are you sure that it wouldn't impact any application on any platform (IA, arm, etc.)?
As I remember we already had similar conversation few years ago.
Again if memory serves me right - one of the contr-arguments about setting that value too high
was that PMD might start to check DD bit inside TXD too often - and will collide with HW updating it more often.
As I remember it was suggested to use 1/2 or 3/4 of nb_desc as default one.
Though I still don't see what is wrong with setting tx_free_thresh vi queue_setup() for that particular case.
In that case we can be sure that no other stuff will be affected.
After all - that's why it is configurable.
Konstantin
diff mbox

Patch

diff --git a/drivers/net/i40e/i40e_rxtx.c b/drivers/net/i40e/i40e_rxtx.c
index 56a854cec..d9569bdc9 100644
--- a/drivers/net/i40e/i40e_rxtx.c
+++ b/drivers/net/i40e/i40e_rxtx.c
@@ -2039,6 +2039,8 @@  i40e_dev_tx_queue_setup(struct rte_eth_dev *dev,
 		tx_conf->tx_rs_thresh : DEFAULT_TX_RS_THRESH);
 	tx_free_thresh = (uint16_t)((tx_conf->tx_free_thresh) ?
 		tx_conf->tx_free_thresh : DEFAULT_TX_FREE_THRESH);
+	if (tx_free_thresh < nb_desc - I40E_TX_MAX_BURST)
+		tx_free_thresh = nb_desc - I40E_TX_MAX_BURST;
 	if (tx_rs_thresh >= (nb_desc - 2)) {
 		PMD_INIT_LOG(ERR, "tx_rs_thresh must be less than the "
 			     "number of TX descriptors minus 2. "