| Message ID | 20250122104307.138659-1-dheeraj.linuxdev@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [net] net: fec: remove unnecessary DMA mapping of TSO header | expand |
On Wed, Jan 22, 2025 at 04:13:07PM +0530, Dheeraj Reddy Jonnalagadda wrote: > The TSO header buffer is pre-allocated DMA memory, so there's no need to > map it again with dma_map_single() in fec_enet_txq_put_hdr_tso(). Remove > this redundant mapping operation. > > Fixes: 79f339125ea3 ("net: fec: Add software TSO support") > Signed-off-by: Dheeraj Reddy Jonnalagadda <dheeraj.linuxdev@gmail.com> > --- > drivers/net/ethernet/freescale/fec_main.c | 9 --------- > 1 file changed, 9 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c > index 68725506a095..039de4c5044e 100644 > --- a/drivers/net/ethernet/freescale/fec_main.c > +++ b/drivers/net/ethernet/freescale/fec_main.c > @@ -805,15 +805,6 @@ fec_enet_txq_put_hdr_tso(struct fec_enet_priv_tx_q *txq, > > if (fep->quirks & FEC_QUIRK_SWAP_FRAME) > swap_buffer(bufaddr, hdr_len); > - > - dmabuf = dma_map_single(&fep->pdev->dev, bufaddr, > - hdr_len, DMA_TO_DEVICE); > - if (dma_mapping_error(&fep->pdev->dev, dmabuf)) { > - dev_kfree_skb_any(skb); > - if (net_ratelimit()) > - netdev_err(ndev, "Tx DMA memory map failed\n"); > - return NETDEV_TX_OK; > - } > } > > bdp->cbd_bufaddr = cpu_to_fec32(dmabuf); Hi Dheeraj, It seems to me that at this point of execution, without your patch, dmabuf can be one of two values: 1. txq->tso_hdrs_dma + index * TSO_HEADER_SIZE This is the default case. 2. txq->tx_bounce[index] This is the special case, which is handed in the conditional block that your patch updates. In case 1, I agree that tso_hdrs_dma is already mapped. But this case is not affected by your patch as the conditional block that includes dma_map_single() is not executed. In case 2, I do not see where txq->tx_bounce[index] is already mapped. Am I missing something?
> The TSO header buffer is pre-allocated DMA memory, so there's no need to > map it again with dma_map_single() in fec_enet_txq_put_hdr_tso(). Remove > this redundant mapping operation. > > Fixes: 79f339125ea3 ("net: fec: Add software TSO support") > Signed-off-by: Dheeraj Reddy Jonnalagadda <dheeraj.linuxdev@gmail.com> > --- > drivers/net/ethernet/freescale/fec_main.c | 9 --------- > 1 file changed, 9 deletions(-) > > diff --git a/drivers/net/ethernet/freescale/fec_main.c > b/drivers/net/ethernet/freescale/fec_main.c > index 68725506a095..039de4c5044e 100644 > --- a/drivers/net/ethernet/freescale/fec_main.c > +++ b/drivers/net/ethernet/freescale/fec_main.c > @@ -805,15 +805,6 @@ fec_enet_txq_put_hdr_tso(struct fec_enet_priv_tx_q > *txq, > > if (fep->quirks & FEC_QUIRK_SWAP_FRAME) > swap_buffer(bufaddr, hdr_len); > - > - dmabuf = dma_map_single(&fep->pdev->dev, bufaddr, > - hdr_len, DMA_TO_DEVICE); > - if (dma_mapping_error(&fep->pdev->dev, dmabuf)) { > - dev_kfree_skb_any(skb); > - if (net_ratelimit()) > - netdev_err(ndev, "Tx DMA memory map failed\n"); > - return NETDEV_TX_OK; > - } > } > > bdp->cbd_bufaddr = cpu_to_fec32(dmabuf); > -- > 2.34.1 Hi Dheeraj, I must admit that I misread it too. There is another case in the TSO header where txq->tx_bounce may be used in some cases. I think the most correct fix is to make txq->tso_hdrs aligned to 32/64 bytes when allocating tso_hdrs, then we do not need to use txq->tx_bounce in fec_enet_txq_put_hdr_tso(), because (bufaddr) & fep->tx_align) will not be true. This way we can safely remove dma_map_single() from fec_enet_txq_put_hdr_tso().
On Thu, Jan 23, 2025 at 02:58:32AM +0000, Wei Fang wrote: > > Hi Dheeraj, > > I must admit that I misread it too. There is another case in the TSO > header where txq->tx_bounce may be used in some cases. I think > the most correct fix is to make txq->tso_hdrs aligned to 32/64 bytes > when allocating tso_hdrs, then we do not need to use txq->tx_bounce > in fec_enet_txq_put_hdr_tso(), because (bufaddr) & fep->tx_align) > will not be true. This way we can safely remove dma_map_single() > from fec_enet_txq_put_hdr_tso(). Hi Fang, Simon, Thank you for the feedback. I have a clarification question regarding the alignment of txq->tso_hdrs. In the current code, txq->tso_hdrs is allocated using fec_dma_alloc(), which internally calls dma_alloc_coherent(). As I understand it, dma_alloc_coherent() guarantees that the allocated buffer is properly aligned. Given this, should we remove the alignment check ((unsigned long)bufaddr) & fep->tx_align and the associated dma_map_single() logic entirely from fec_enet_txq_put_hdr_tso() as you have suggested? -Dheeraj
> > Hi Dheeraj, > > > > I must admit that I misread it too. There is another case in the TSO > > header where txq->tx_bounce may be used in some cases. I think > > the most correct fix is to make txq->tso_hdrs aligned to 32/64 bytes > > when allocating tso_hdrs, then we do not need to use txq->tx_bounce > > in fec_enet_txq_put_hdr_tso(), because (bufaddr) & fep->tx_align) > > will not be true. This way we can safely remove dma_map_single() > > from fec_enet_txq_put_hdr_tso(). > > Hi Fang, Simon, > > Thank you for the feedback. I have a clarification question regarding > the alignment of txq->tso_hdrs. > > In the current code, txq->tso_hdrs is allocated using fec_dma_alloc(), > which internally calls dma_alloc_coherent(). As I understand it, > dma_alloc_coherent() guarantees that the allocated buffer is properly aligned. > > Given this, should we remove the alignment check > ((unsigned long)bufaddr) & fep->tx_align and the associated dma_map_single() > logic entirely from fec_enet_txq_put_hdr_tso() as you have suggested? > The maximum of fep->tx_align is 0xf, so we need to ensure the buffer allocated by dma_alloc_coherent() is at least 16-byte aligned, but I'm not sure whether dma_alloc_coherent() guarantees this alignment. A safe approach is to apply for a memory block longer than expected, for example, if we want the alignment is 32-byte, we can apply for a memory block that is 32 bytes longer than the expected length. Then use ALGIN and PTR_ALIGN to get the aligned addr. txq->tso_hdrs_base = fec_dma_alloc(&fep->pdev->dev, txq->bd.ring_size * TSO_HEADER_SIZE + 32, &txq->tso_hdrs_dma_base, GFP_KERNEL); txq->tso_hdrs = PTR_ALIGN(txq->tso_hdrs_base, 32); txq->tso_hdrs_dma = ALIGN(txq->tso_hdrs_dma_base, 32); This is just my personal experience, if you have a better way that would be great.
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index 68725506a095..039de4c5044e 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -805,15 +805,6 @@ fec_enet_txq_put_hdr_tso(struct fec_enet_priv_tx_q *txq, if (fep->quirks & FEC_QUIRK_SWAP_FRAME) swap_buffer(bufaddr, hdr_len); - - dmabuf = dma_map_single(&fep->pdev->dev, bufaddr, - hdr_len, DMA_TO_DEVICE); - if (dma_mapping_error(&fep->pdev->dev, dmabuf)) { - dev_kfree_skb_any(skb); - if (net_ratelimit()) - netdev_err(ndev, "Tx DMA memory map failed\n"); - return NETDEV_TX_OK; - } } bdp->cbd_bufaddr = cpu_to_fec32(dmabuf);
The TSO header buffer is pre-allocated DMA memory, so there's no need to map it again with dma_map_single() in fec_enet_txq_put_hdr_tso(). Remove this redundant mapping operation. Fixes: 79f339125ea3 ("net: fec: Add software TSO support") Signed-off-by: Dheeraj Reddy Jonnalagadda <dheeraj.linuxdev@gmail.com> --- drivers/net/ethernet/freescale/fec_main.c | 9 --------- 1 file changed, 9 deletions(-)