[3/3] net/ixgbe: reduce redundant store operation
Checks
Commit Message
For free buffer in ixgbe driver, 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>
---
drivers/net/ixgbe/ixgbe_rxtx.c | 1 -
1 file changed, 1 deletion(-)
Comments
> -----Original Message-----
> From: Feifei Wang <feifei.wang2@arm.com>
> Sent: Monday, December 20, 2021 13:51
> To: Wang, Haiyue <haiyue.wang@intel.com>
> Cc: dev@dpdk.org; nd@arm.com; Feifei Wang <feifei.wang2@arm.com>; Ruifeng Wang <ruifeng.wang@arm.com>
> Subject: [PATCH 3/3] net/ixgbe: reduce redundant store operation
>
> For free buffer in ixgbe driver, 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>
> ---
> drivers/net/ixgbe/ixgbe_rxtx.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c b/drivers/net/ixgbe/ixgbe_rxtx.c
> index d7c80d4242..9f3f2e9b50 100644
> --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> @@ -120,7 +120,6 @@ ixgbe_tx_free_bufs(struct ixgbe_tx_queue *txq)
> for (i = 0; i < txq->tx_rs_thresh; ++i, ++txep) {
> /* free buffers one at a time */
> m = rte_pktmbuf_prefree_seg(txep->mbuf);
> - txep->mbuf = NULL;
Not sure, but at least found:
static void __rte_cold
ixgbe_tx_queue_release_mbufs(struct ixgbe_tx_queue *txq)
{
unsigned i;
if (txq->sw_ring != NULL) {
for (i = 0; i < txq->nb_tx_desc; i++) {
if (txq->sw_ring[i].mbuf != NULL) { <---------------------------- ?
rte_pktmbuf_free_seg(txq->sw_ring[i].mbuf);
txq->sw_ring[i].mbuf = NULL;
}
}
}
}
>
> if (unlikely(m == NULL))
> continue;
> --
> 2.25.1
> -----邮件原件-----
> 发件人: Wang, Haiyue <haiyue.wang@intel.com>
> 发送时间: Monday, December 20, 2021 3:25 PM
> 收件人: Feifei Wang <Feifei.Wang2@arm.com>
> 抄送: dev@dpdk.org; nd <nd@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>
> 主题: RE: [PATCH 3/3] net/ixgbe: reduce redundant store operation
>
> > -----Original Message-----
> > From: Feifei Wang <feifei.wang2@arm.com>
> > Sent: Monday, December 20, 2021 13:51
> > To: Wang, Haiyue <haiyue.wang@intel.com>
> > Cc: dev@dpdk.org; nd@arm.com; Feifei Wang <feifei.wang2@arm.com>;
> > Ruifeng Wang <ruifeng.wang@arm.com>
> > Subject: [PATCH 3/3] net/ixgbe: reduce redundant store operation
> >
> > For free buffer in ixgbe driver, 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>
> > ---
> > drivers/net/ixgbe/ixgbe_rxtx.c | 1 -
> > 1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/net/ixgbe/ixgbe_rxtx.c
> > b/drivers/net/ixgbe/ixgbe_rxtx.c index d7c80d4242..9f3f2e9b50 100644
> > --- a/drivers/net/ixgbe/ixgbe_rxtx.c
> > +++ b/drivers/net/ixgbe/ixgbe_rxtx.c
> > @@ -120,7 +120,6 @@ ixgbe_tx_free_bufs(struct ixgbe_tx_queue *txq)
> > for (i = 0; i < txq->tx_rs_thresh; ++i, ++txep) {
> > /* free buffers one at a time */
> > m = rte_pktmbuf_prefree_seg(txep->mbuf);
> > - txep->mbuf = NULL;
>
> Not sure, but at least found:
>
> static void __rte_cold
> ixgbe_tx_queue_release_mbufs(struct ixgbe_tx_queue *txq) {
> unsigned i;
>
> if (txq->sw_ring != NULL) {
> for (i = 0; i < txq->nb_tx_desc; i++) {
> if (txq->sw_ring[i].mbuf != NULL) { <--------------------
> -------- ?
> rte_pktmbuf_free_seg(txq-
> >sw_ring[i].mbuf);
> txq->sw_ring[i].mbuf = NULL;
> }
> }
> }
> }
>
Thanks for your remind. I check the function"xx_tx_queue_release_mbufs" and "xx_tx_done_cleanup_full" which
have the check for 'sw_ring->buf == NULL'. I find the scheme of free buffers in scalar path and vector path are different:
For scalar, it should support jumbo frame, so it cannot use 'tx_next_dd' to find free buffer index
For vector, free a packet means free a buffer, just use tx_next_dd can find the start index of free buffer.
At last, store operation of NULL for freed buffer is necessary for scalar path. And I will just keep the vector path path.
> >
> > if (unlikely(m == NULL))
> > continue;
> > --
> > 2.25.1
@@ -120,7 +120,6 @@ ixgbe_tx_free_bufs(struct ixgbe_tx_queue *txq)
for (i = 0; i < txq->tx_rs_thresh; ++i, ++txep) {
/* free buffers one at a time */
m = rte_pktmbuf_prefree_seg(txep->mbuf);
- txep->mbuf = NULL;
if (unlikely(m == NULL))
continue;