[v2] net/i40e: reduce redundant store operation
Checks
Commit Message
For free buffer operation in i40e vector path, it is unnecessary to
store 'NULL' into txep.mbuf. This is because when putting mbuf into Tx
queue, tx_tail is the sentinel. And when doing tx_free, tx_next_dd is
the sentinel. In all processes, mbuf==NULL is not a condition in check.
Thus reset of mbuf is unnecessary and can be omitted.
Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
---
v2: remove the change for scalar path due to scalar path needs to check
whether the mbuf is 'NULL' to release and clean up (Haiyue)
drivers/net/i40e/i40e_rxtx_vec_common.h | 1 -
1 file changed, 1 deletion(-)
Comments
> -----Original Message-----
> From: Feifei Wang <feifei.wang2@arm.com>
> Sent: Tuesday, December 21, 2021 4:11 PM
> To: Xing, Beilei <beilei.xing@intel.com>
> Cc: dev@dpdk.org; Wang, Haiyue <haiyue.wang@intel.com>; nd@arm.com;
> Feifei Wang <feifei.wang2@arm.com>; Ruifeng Wang
> <ruifeng.wang@arm.com>
> Subject: [PATCH v2] net/i40e: reduce redundant store operation
>
> For free buffer operation in i40e vector path, it is unnecessary to store 'NULL'
> into txep.mbuf. This is because when putting mbuf into Tx queue, tx_tail is
> the sentinel. And when doing tx_free, tx_next_dd is the sentinel. In all
> processes, mbuf==NULL is not a condition in check.
> Thus reset of mbuf is unnecessary and can be omitted.
>
> Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
> Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> ---
>
> v2: remove the change for scalar path due to scalar path needs to check
> whether the mbuf is 'NULL' to release and clean up (Haiyue)
>
> drivers/net/i40e/i40e_rxtx_vec_common.h | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/net/i40e/i40e_rxtx_vec_common.h
> b/drivers/net/i40e/i40e_rxtx_vec_common.h
> index f9a7f46550..26deb59fc4 100644
> --- a/drivers/net/i40e/i40e_rxtx_vec_common.h
> +++ b/drivers/net/i40e/i40e_rxtx_vec_common.h
> @@ -103,7 +103,6 @@ i40e_tx_free_bufs(struct i40e_tx_queue *txq)
> if (txq->offloads & RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE) {
> for (i = 0; i < n; i++) {
> free[i] = txep[i].mbuf;
> - txep[i].mbuf = NULL;
I will suggest to still add some comment here just for explaining, this may help to avoid unnecessary suspect when someone reading or debug on these code 😊
> }
> rte_mempool_put_bulk(free[0]->pool, (void **)free, n);
> goto done;
> --
> 2.25.1
> -----邮件原件-----
> 发件人: Zhang, Qi Z <qi.z.zhang@intel.com>
> 发送时间: Wednesday, January 26, 2022 10:28 PM
> 收件人: Feifei Wang <Feifei.Wang2@arm.com>; Xing, Beilei
> <beilei.xing@intel.com>
> 抄送: dev@dpdk.org; Wang, Haiyue <haiyue.wang@intel.com>; nd
> <nd@arm.com>; Ruifeng Wang <Ruifeng.Wang@arm.com>
> 主题: RE: [PATCH v2] net/i40e: reduce redundant store operation
>
>
>
> > -----Original Message-----
> > From: Feifei Wang <feifei.wang2@arm.com>
> > Sent: Tuesday, December 21, 2021 4:11 PM
> > To: Xing, Beilei <beilei.xing@intel.com>
> > Cc: dev@dpdk.org; Wang, Haiyue <haiyue.wang@intel.com>; nd@arm.com;
> > Feifei Wang <feifei.wang2@arm.com>; Ruifeng Wang
> > <ruifeng.wang@arm.com>
> > Subject: [PATCH v2] net/i40e: reduce redundant store operation
> >
> > For free buffer operation in i40e vector path, it is unnecessary to store
> 'NULL'
> > into txep.mbuf. This is because when putting mbuf into Tx queue,
> > tx_tail is the sentinel. And when doing tx_free, tx_next_dd is the
> > sentinel. In all processes, mbuf==NULL is not a condition in check.
> > Thus reset of mbuf is unnecessary and can be omitted.
> >
> > Signed-off-by: Feifei Wang <feifei.wang2@arm.com>
> > Reviewed-by: Ruifeng Wang <ruifeng.wang@arm.com>
> > ---
> >
> > v2: remove the change for scalar path due to scalar path needs to
> > check whether the mbuf is 'NULL' to release and clean up (Haiyue)
> >
> > drivers/net/i40e/i40e_rxtx_vec_common.h | 1 -
> > 1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/net/i40e/i40e_rxtx_vec_common.h
> > b/drivers/net/i40e/i40e_rxtx_vec_common.h
> > index f9a7f46550..26deb59fc4 100644
> > --- a/drivers/net/i40e/i40e_rxtx_vec_common.h
> > +++ b/drivers/net/i40e/i40e_rxtx_vec_common.h
> > @@ -103,7 +103,6 @@ i40e_tx_free_bufs(struct i40e_tx_queue *txq)
> > if (txq->offloads & RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE) {
> > for (i = 0; i < n; i++) {
> > free[i] = txep[i].mbuf;
> > - txep[i].mbuf = NULL;
>
> I will suggest to still add some comment here just for explaining, this may help
> to avoid unnecessary suspect when someone reading or debug on these code
> 😊
>
Thanks for your comments. Agree with this, and I will add the comment to
explain why this store operation is unnecessary here.
>
> > }
> > rte_mempool_put_bulk(free[0]->pool, (void **)free, n);
> > goto done;
> > --
> > 2.25.1
@@ -103,7 +103,6 @@ i40e_tx_free_bufs(struct i40e_tx_queue *txq)
if (txq->offloads & RTE_ETH_TX_OFFLOAD_MBUF_FAST_FREE) {
for (i = 0; i < n; i++) {
free[i] = txep[i].mbuf;
- txep[i].mbuf = NULL;
}
rte_mempool_put_bulk(free[0]->pool, (void **)free, n);
goto done;