From: Jesper Dangaard Brouer <hawk@kernel.org>
To: Wei Fang <wei.fang@nxp.com>,
davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
pabeni@redhat.com, shenwei.wang@nxp.com, xiaoning.wang@nxp.com,
ast@kernel.org, daniel@iogearbox.net, hawk@kernel.org,
john.fastabend@gmail.com, larysa.zaremba@intel.com,
aleksander.lobakin@intel.com, jbrouer@redhat.com,
netdev@vger.kernel.org
Cc: linux-imx@nxp.com, linux-kernel@vger.kernel.org, bpf@vger.kernel.org
Subject: Re: [PATCH V5 net-next 2/2] net: fec: improve XDP_TX performance
Date: Thu, 10 Aug 2023 16:11:41 +0200 [thread overview]
Message-ID: <831968f6-3abb-6427-bb1a-e6fd40beb633@kernel.org> (raw)
In-Reply-To: <20230810064514.104470-3-wei.fang@nxp.com>
On 10/08/2023 08.45, Wei Fang wrote:
> As suggested by Jesper and Alexander, we can avoid converting xdp_buff
> to xdp_frame in case of XDP_TX to save a bunch of CPU cycles, so that
> we can further improve the XDP_TX performance.
>
> Before this patch on i.MX8MP-EVK board, the performance shows as follows.
> root@imx8mpevk:~# ./xdp2 eth0
> proto 17: 353918 pkt/s
> proto 17: 352923 pkt/s
> proto 17: 353900 pkt/s
> proto 17: 352672 pkt/s
> proto 17: 353912 pkt/s
> proto 17: 354219 pkt/s
>
> After applying this patch, the performance is improved.
> root@imx8mpevk:~# ./xdp2 eth0
> proto 17: 369261 pkt/s
> proto 17: 369267 pkt/s
> proto 17: 369206 pkt/s
> proto 17: 369214 pkt/s
> proto 17: 369126 pkt/s
> proto 17: 369272 pkt/s
>
So approx 4.3% improvement.
> Signed-off-by: Wei Fang <wei.fang@nxp.com>
> Suggested-by: Alexander Lobakin <aleksander.lobakin@intel.com>
> Suggested-by: Jesper Dangaard Brouer <hawk@kernel.org>
> ---
> V5 changes:
> New patch. Separated from the first patch, to keep track of the changes
> and improvements (suggested by Jesper).
Thanks
> ---
> drivers/net/ethernet/freescale/fec.h | 5 +-
> drivers/net/ethernet/freescale/fec_main.c | 134 ++++++++++++----------
> 2 files changed, 73 insertions(+), 66 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec.h b/drivers/net/ethernet/freescale/fec.h
> index 7bb02a9da2a6..a8fbcada6b01 100644
> --- a/drivers/net/ethernet/freescale/fec.h
> +++ b/drivers/net/ethernet/freescale/fec.h
> @@ -552,10 +552,7 @@ enum fec_txbuf_type {
> };
>
> struct fec_tx_buffer {
> - union {
> - struct sk_buff *skb;
> - struct xdp_frame *xdp;
> - };
> + void *buf_p;
I want to hear Olek's (Alexander) oppinion on this void pointer circus.
The rest of the patch looks correct to me so:
Reviewed-by: Jesper Dangaard Brouer <hawk@kernel.org>
--Jesper
> enum fec_txbuf_type type;
> };
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
> index 30b01985be7c..ae6e41ad71b8 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -399,7 +399,7 @@ static void fec_dump(struct net_device *ndev)
> fec16_to_cpu(bdp->cbd_sc),
> fec32_to_cpu(bdp->cbd_bufaddr),
> fec16_to_cpu(bdp->cbd_datlen),
> - txq->tx_buf[index].skb);
> + txq->tx_buf[index].buf_p);
> bdp = fec_enet_get_nextdesc(bdp, &txq->bd);
> index++;
> } while (bdp != txq->bd.base);
> @@ -656,7 +656,7 @@ static int fec_enet_txq_submit_skb(struct fec_enet_priv_tx_q *txq,
>
> index = fec_enet_get_bd_index(last_bdp, &txq->bd);
> /* Save skb pointer */
> - txq->tx_buf[index].skb = skb;
> + txq->tx_buf[index].buf_p = skb;
>
> /* Make sure the updates to rest of the descriptor are performed before
> * transferring ownership.
> @@ -862,7 +862,7 @@ static int fec_enet_txq_submit_tso(struct fec_enet_priv_tx_q *txq,
> }
>
> /* Save skb pointer */
> - txq->tx_buf[index].skb = skb;
> + txq->tx_buf[index].buf_p = skb;
>
> skb_tx_timestamp(skb);
> txq->bd.cur = bdp;
> @@ -959,27 +959,27 @@ static void fec_enet_bd_init(struct net_device *dev)
> fec32_to_cpu(bdp->cbd_bufaddr),
> fec16_to_cpu(bdp->cbd_datlen),
> DMA_TO_DEVICE);
> - if (txq->tx_buf[i].skb) {
> - dev_kfree_skb_any(txq->tx_buf[i].skb);
> - txq->tx_buf[i].skb = NULL;
> - }
> - } else {
> - if (bdp->cbd_bufaddr &&
> - txq->tx_buf[i].type == FEC_TXBUF_T_XDP_NDO)
> + if (txq->tx_buf[i].buf_p)
> + dev_kfree_skb_any(txq->tx_buf[i].buf_p);
> + } else if (txq->tx_buf[i].type == FEC_TXBUF_T_XDP_NDO) {
> + if (bdp->cbd_bufaddr)
> dma_unmap_single(&fep->pdev->dev,
> fec32_to_cpu(bdp->cbd_bufaddr),
> fec16_to_cpu(bdp->cbd_datlen),
> DMA_TO_DEVICE);
>
> - if (txq->tx_buf[i].xdp) {
> - xdp_return_frame(txq->tx_buf[i].xdp);
> - txq->tx_buf[i].xdp = NULL;
> - }
> + if (txq->tx_buf[i].buf_p)
> + xdp_return_frame(txq->tx_buf[i].buf_p);
> + } else {
> + struct page *page = txq->tx_buf[i].buf_p;
>
> - /* restore default tx buffer type: FEC_TXBUF_T_SKB */
> - txq->tx_buf[i].type = FEC_TXBUF_T_SKB;
> + if (page)
> + page_pool_put_page(page->pp, page, 0, false);
> }
>
> + txq->tx_buf[i].buf_p = NULL;
> + /* restore default tx buffer type: FEC_TXBUF_T_SKB */
> + txq->tx_buf[i].type = FEC_TXBUF_T_SKB;
> bdp->cbd_bufaddr = cpu_to_fec32(0);
> bdp = fec_enet_get_nextdesc(bdp, &txq->bd);
> }
> @@ -1386,6 +1386,8 @@ fec_enet_tx_queue(struct net_device *ndev, u16 queue_id, int budget)
> struct netdev_queue *nq;
> int index = 0;
> int entries_free;
> + struct page *page;
> + int frame_len;
>
> fep = netdev_priv(ndev);
>
> @@ -1407,8 +1409,7 @@ fec_enet_tx_queue(struct net_device *ndev, u16 queue_id, int budget)
> index = fec_enet_get_bd_index(bdp, &txq->bd);
>
> if (txq->tx_buf[index].type == FEC_TXBUF_T_SKB) {
> - skb = txq->tx_buf[index].skb;
> - txq->tx_buf[index].skb = NULL;
> + skb = txq->tx_buf[index].buf_p;
> if (bdp->cbd_bufaddr &&
> !IS_TSO_HEADER(txq, fec32_to_cpu(bdp->cbd_bufaddr)))
> dma_unmap_single(&fep->pdev->dev,
> @@ -1427,18 +1428,24 @@ fec_enet_tx_queue(struct net_device *ndev, u16 queue_id, int budget)
> if (unlikely(!budget))
> break;
>
> - xdpf = txq->tx_buf[index].xdp;
> - if (bdp->cbd_bufaddr &&
> - txq->tx_buf[index].type == FEC_TXBUF_T_XDP_NDO)
> - dma_unmap_single(&fep->pdev->dev,
> - fec32_to_cpu(bdp->cbd_bufaddr),
> - fec16_to_cpu(bdp->cbd_datlen),
> - DMA_TO_DEVICE);
> + if (txq->tx_buf[index].type == FEC_TXBUF_T_XDP_NDO) {
> + xdpf = txq->tx_buf[index].buf_p;
> + if (bdp->cbd_bufaddr)
> + dma_unmap_single(&fep->pdev->dev,
> + fec32_to_cpu(bdp->cbd_bufaddr),
> + fec16_to_cpu(bdp->cbd_datlen),
> + DMA_TO_DEVICE);
> + } else {
> + page = txq->tx_buf[index].buf_p;
> + }
> +
> bdp->cbd_bufaddr = cpu_to_fec32(0);
> - if (unlikely(!xdpf)) {
> + if (unlikely(!txq->tx_buf[index].buf_p)) {
> txq->tx_buf[index].type = FEC_TXBUF_T_SKB;
> goto tx_buf_done;
> }
> +
> + frame_len = fec16_to_cpu(bdp->cbd_datlen);
> }
>
> /* Check for errors. */
> @@ -1462,7 +1469,7 @@ fec_enet_tx_queue(struct net_device *ndev, u16 queue_id, int budget)
> if (txq->tx_buf[index].type == FEC_TXBUF_T_SKB)
> ndev->stats.tx_bytes += skb->len;
> else
> - ndev->stats.tx_bytes += xdpf->len;
> + ndev->stats.tx_bytes += frame_len;
> }
>
> /* Deferred means some collisions occurred during transmit,
> @@ -1487,20 +1494,16 @@ fec_enet_tx_queue(struct net_device *ndev, u16 queue_id, int budget)
>
> /* Free the sk buffer associated with this last transmit */
> dev_kfree_skb_any(skb);
> + } else if (txq->tx_buf[index].type == FEC_TXBUF_T_XDP_NDO) {
> + xdp_return_frame_rx_napi(xdpf);
> } else {
> - if (txq->tx_buf[index].type == FEC_TXBUF_T_XDP_NDO) {
> - xdp_return_frame_rx_napi(xdpf);
> - } else {
> - struct page *page = virt_to_head_page(xdpf->data);
> -
> - page_pool_put_page(page->pp, page, 0, true);
> - }
> -
> - txq->tx_buf[index].xdp = NULL;
> - /* restore default tx buffer type: FEC_TXBUF_T_SKB */
> - txq->tx_buf[index].type = FEC_TXBUF_T_SKB;
> + page_pool_put_page(page->pp, page, 0, true);
> }
>
> + txq->tx_buf[index].buf_p = NULL;
> + /* restore default tx buffer type: FEC_TXBUF_T_SKB */
> + txq->tx_buf[index].type = FEC_TXBUF_T_SKB;
> +
> tx_buf_done:
> /* Make sure the update to bdp and tx_buf are performed
> * before dirty_tx
> @@ -3230,7 +3233,6 @@ static void fec_enet_free_buffers(struct net_device *ndev)
> {
> struct fec_enet_private *fep = netdev_priv(ndev);
> unsigned int i;
> - struct sk_buff *skb;
> struct fec_enet_priv_tx_q *txq;
> struct fec_enet_priv_rx_q *rxq;
> unsigned int q;
> @@ -3255,18 +3257,23 @@ static void fec_enet_free_buffers(struct net_device *ndev)
> kfree(txq->tx_bounce[i]);
> txq->tx_bounce[i] = NULL;
>
> + if (!txq->tx_buf[i].buf_p) {
> + txq->tx_buf[i].type = FEC_TXBUF_T_SKB;
> + continue;
> + }
> +
> if (txq->tx_buf[i].type == FEC_TXBUF_T_SKB) {
> - skb = txq->tx_buf[i].skb;
> - txq->tx_buf[i].skb = NULL;
> - dev_kfree_skb(skb);
> + dev_kfree_skb(txq->tx_buf[i].buf_p);
> + } else if (txq->tx_buf[i].type == FEC_TXBUF_T_XDP_NDO) {
> + xdp_return_frame(txq->tx_buf[i].buf_p);
> } else {
> - if (txq->tx_buf[i].xdp) {
> - xdp_return_frame(txq->tx_buf[i].xdp);
> - txq->tx_buf[i].xdp = NULL;
> - }
> + struct page *page = txq->tx_buf[i].buf_p;
>
> - txq->tx_buf[i].type = FEC_TXBUF_T_SKB;
> + page_pool_put_page(page->pp, page, 0, false);
> }
> +
> + txq->tx_buf[i].buf_p = NULL;
> + txq->tx_buf[i].type = FEC_TXBUF_T_SKB;
> }
> }
> }
> @@ -3789,13 +3796,14 @@ fec_enet_xdp_get_tx_queue(struct fec_enet_private *fep, int index)
>
> static int fec_enet_txq_xmit_frame(struct fec_enet_private *fep,
> struct fec_enet_priv_tx_q *txq,
> - struct xdp_frame *frame,
> - u32 dma_sync_len, bool ndo_xmit)
> + void *frame, u32 dma_sync_len,
> + bool ndo_xmit)
> {
> unsigned int index, status, estatus;
> struct bufdesc *bdp;
> dma_addr_t dma_addr;
> int entries_free;
> + u16 frame_len;
>
> entries_free = fec_enet_get_free_txdesc_num(txq);
> if (entries_free < MAX_SKB_FRAGS + 1) {
> @@ -3811,30 +3819,36 @@ static int fec_enet_txq_xmit_frame(struct fec_enet_private *fep,
> index = fec_enet_get_bd_index(bdp, &txq->bd);
>
> if (ndo_xmit) {
> - dma_addr = dma_map_single(&fep->pdev->dev, frame->data,
> - frame->len, DMA_TO_DEVICE);
> + struct xdp_frame *xdpf = frame;
> +
> + dma_addr = dma_map_single(&fep->pdev->dev, xdpf->data,
> + xdpf->len, DMA_TO_DEVICE);
> if (dma_mapping_error(&fep->pdev->dev, dma_addr))
> return -ENOMEM;
>
> + frame_len = xdpf->len;
> + txq->tx_buf[index].buf_p = xdpf;
> txq->tx_buf[index].type = FEC_TXBUF_T_XDP_NDO;
> } else {
> - struct page *page = virt_to_page(frame->data);
> + struct xdp_buff *xdpb = frame;
> + struct page *page;
>
> - dma_addr = page_pool_get_dma_addr(page) + sizeof(*frame) +
> - frame->headroom;
> + page = virt_to_page(xdpb->data);
> + dma_addr = page_pool_get_dma_addr(page) +
> + (xdpb->data - xdpb->data_hard_start);
> dma_sync_single_for_device(&fep->pdev->dev, dma_addr,
> dma_sync_len, DMA_BIDIRECTIONAL);
> + frame_len = xdpb->data_end - xdpb->data;
> + txq->tx_buf[index].buf_p = page;
> txq->tx_buf[index].type = FEC_TXBUF_T_XDP_TX;
> }
>
> - txq->tx_buf[index].xdp = frame;
> -
> status |= (BD_ENET_TX_INTR | BD_ENET_TX_LAST);
> if (fep->bufdesc_ex)
> estatus = BD_ENET_TX_INT;
>
> bdp->cbd_bufaddr = cpu_to_fec32(dma_addr);
> - bdp->cbd_datlen = cpu_to_fec16(frame->len);
> + bdp->cbd_datlen = cpu_to_fec16(frame_len);
>
> if (fep->bufdesc_ex) {
> struct bufdesc_ex *ebdp = (struct bufdesc_ex *)bdp;
> @@ -3875,14 +3889,10 @@ static int fec_enet_xdp_tx_xmit(struct fec_enet_private *fep,
> int cpu, struct xdp_buff *xdp,
> u32 dma_sync_len)
> {
> - struct xdp_frame *xdpf = xdp_convert_buff_to_frame(xdp);
> struct fec_enet_priv_tx_q *txq;
> struct netdev_queue *nq;
> int queue, ret;
>
> - if (unlikely(!xdpf))
> - return -EFAULT;
> -
> queue = fec_enet_xdp_get_tx_queue(fep, cpu);
> txq = fep->tx_queue[queue];
> nq = netdev_get_tx_queue(fep->netdev, queue);
> @@ -3891,7 +3901,7 @@ static int fec_enet_xdp_tx_xmit(struct fec_enet_private *fep,
>
> /* Avoid tx timeout as XDP shares the queue with kernel stack */
> txq_trans_cond_update(nq);
> - ret = fec_enet_txq_xmit_frame(fep, txq, xdpf, dma_sync_len, false);
> + ret = fec_enet_txq_xmit_frame(fep, txq, xdp, dma_sync_len, false);
>
> __netif_tx_unlock(nq);
>
prev parent reply other threads:[~2023-08-10 14:11 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-08-10 6:45 [PATCH V5 net-next 0/2] net: fec: add XDP_TX feature support Wei Fang
2023-08-10 6:45 ` [PATCH V5 net-next 1/2] " Wei Fang
2023-08-10 13:58 ` Jesper Dangaard Brouer
2023-08-11 1:26 ` Wei Fang
2023-08-11 15:08 ` Jesper Dangaard Brouer
2023-08-11 11:58 ` Simon Horman
2023-08-12 0:06 ` Wei Fang
2023-08-10 6:45 ` [PATCH V5 net-next 2/2] net: fec: improve XDP_TX performance Wei Fang
2023-08-10 14:11 ` Jesper Dangaard Brouer [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=831968f6-3abb-6427-bb1a-e6fd40beb633@kernel.org \
--to=hawk@kernel.org \
--cc=aleksander.lobakin@intel.com \
--cc=ast@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=jbrouer@redhat.com \
--cc=john.fastabend@gmail.com \
--cc=kuba@kernel.org \
--cc=larysa.zaremba@intel.com \
--cc=linux-imx@nxp.com \
--cc=linux-kernel@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=shenwei.wang@nxp.com \
--cc=wei.fang@nxp.com \
--cc=xiaoning.wang@nxp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.