lkml.org 
[lkml]   [2022]   [Sep]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRE: [EXT] Re: [PATCH 1/1] net: fec: add initial XDP support
    Date


    > -----Original Message-----
    > From: Jesper Dangaard Brouer <jbrouer@redhat.com>
    > Sent: Thursday, September 29, 2022 5:17 AM
    > To: Shenwei Wang <shenwei.wang@nxp.com>; Joakim Zhang
    > <qiangqing.zhang@nxp.com>; David S. Miller <davem@davemloft.net>; Eric
    > Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Paolo
    > Abeni <pabeni@redhat.com>
    > > diff --git a/drivers/net/ethernet/freescale/fec.h
    > > b/drivers/net/ethernet/freescale/fec.h
    > > index b0100fe3c9e4..f7531503aa95 100644
    > > --- a/drivers/net/ethernet/freescale/fec.h
    > > +++ b/drivers/net/ethernet/freescale/fec.h
    > > @@ -346,8 +346,10 @@ struct bufdesc_ex {
    > > * the skbuffer directly.
    > > */
    > >
    > > +#define FEC_ENET_XDP_HEADROOM (512) /* XDP_PACKET_HEADROOM
    > + NET_IP_ALIGN) */
    >
    > Why the large headroom?
    >

    The accurate value here should be "XDP_PACKET_HEADROOM (256) + NET_IP_ALIGN" which then aligns with 64 bytes. So 256 + 64 should be enough here.

    > > +
    > > #define FEC_ENET_RX_PAGES 256
    > > -#define FEC_ENET_RX_FRSIZE 2048
    > > +#define FEC_ENET_RX_FRSIZE (PAGE_SIZE - FEC_ENET_XDP_HEADROOM)
    >
    > This FEC_ENET_RX_FRSIZE is likely wrong, because you also need to reserve 320
    > bytes at the end for struct skb_shared_info.
    > (320 calculated as SKB_DATA_ALIGN(sizeof(struct skb_shared_info)))
    >
    > > #define FEC_ENET_RX_FRPPG (PAGE_SIZE / FEC_ENET_RX_FRSIZE)
    > > #define RX_RING_SIZE (FEC_ENET_RX_FRPPG *
    > FEC_ENET_RX_PAGES)
    > > #define FEC_ENET_TX_FRSIZE 2048
    > > @@ -517,6 +519,22 @@ struct bufdesc_prop {
    > [...]
    >
    > > diff --git a/drivers/net/ethernet/freescale/fec_main.c
    > > b/drivers/net/ethernet/freescale/fec_main.c
    > > index 59921218a8a4..2e30182ed770 100644
    > > --- a/drivers/net/ethernet/freescale/fec_main.c
    > > +++ b/drivers/net/ethernet/freescale/fec_main.c
    > > @@ -66,6 +66,8 @@
    > > #include <linux/mfd/syscon.h>
    > > #include <linux/regmap.h>
    > > #include <soc/imx/cpuidle.h>
    > > +#include <linux/filter.h>
    > > +#include <linux/bpf.h>
    > >
    > > #include <asm/cacheflush.h>
    > >
    > > @@ -87,6 +89,11 @@ static const u16 fec_enet_vlan_pri_to_queue[8] = {0, 0,
    > 1, 1, 1, 2, 2, 2};
    > > #define FEC_ENET_OPD_V 0xFFF0
    > > #define FEC_MDIO_PM_TIMEOUT 100 /* ms */
    > >
    > > +#define FEC_ENET_XDP_PASS 0
    > > +#define FEC_ENET_XDP_CONSUMED BIT(0)
    > > +#define FEC_ENET_XDP_TX BIT(1)
    > > +#define FEC_ENET_XDP_REDIR BIT(2)
    > > +
    > > struct fec_devinfo {
    > > u32 quirks;
    > > };
    > > @@ -422,6 +429,49 @@ fec_enet_clear_csum(struct sk_buff *skb, struct
    > net_device *ndev)
    > > return 0;
    > > }
    > >
    > > +static int
    > > +fec_enet_create_page_pool(struct fec_enet_private *fep,
    > > + struct fec_enet_priv_rx_q *rxq, int size) {
    > > + struct bpf_prog *xdp_prog = READ_ONCE(fep->xdp_prog);
    > > + struct page_pool_params pp_params = {
    > > + .order = 0,
    > > + .flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV,
    > > + .pool_size = size,
    > > + .nid = dev_to_node(&fep->pdev->dev),
    > > + .dev = &fep->pdev->dev,
    > > + .dma_dir = xdp_prog ? DMA_BIDIRECTIONAL : DMA_FROM_DEVICE,
    > > + .offset = FEC_ENET_XDP_HEADROOM,
    > > + .max_len = FEC_ENET_RX_FRSIZE,
    >
    > XDP BPF-prog cannot access last 320 bytes, so FEC_ENET_RX_FRSIZE is wrong
    > here.
    >

    So the FEC_ENET_RX_FRSIZE should subtract the sizeof(struct skb_shared_info) in the definition, right?

    > > + };
    > > + int err;
    > > +
    > > + rxq->page_pool = page_pool_create(&pp_params);
    > > + if (IS_ERR(rxq->page_pool)) {
    > > + err = PTR_ERR(rxq->page_pool);
    > > + rxq->page_pool = NULL;
    > > + return err;
    > > + }
    > > +
    > > + err = xdp_rxq_info_reg(&rxq->xdp_rxq, fep->netdev, rxq->id, 0);
    > > + if (err < 0)
    > > + goto err_free_pp;
    > > +
    > > + err = xdp_rxq_info_reg_mem_model(&rxq->xdp_rxq,
    > MEM_TYPE_PAGE_POOL,
    > > + rxq->page_pool);
    > > + if (err)
    > > + goto err_unregister_rxq;
    > > +
    > > + return 0;
    > > +
    > > +err_unregister_rxq:
    > > + xdp_rxq_info_unreg(&rxq->xdp_rxq);
    > > +err_free_pp:
    > > + page_pool_destroy(rxq->page_pool);
    > > + rxq->page_pool = NULL;
    > > + return err;
    > > +}
    > > +
    > > static struct bufdesc *
    > > fec_enet_txq_submit_frag_skb(struct fec_enet_priv_tx_q *txq,
    > > struct sk_buff *skb, @@ -1285,7 +1335,6 @@
    > > fec_stop(struct net_device *ndev)
    > > }
    > > }
    > >
    > > -
    > > static void
    > > fec_timeout(struct net_device *ndev, unsigned int txqueue)
    > > {
    > > @@ -1450,7 +1499,7 @@ static void fec_enet_tx(struct net_device *ndev)
    > > fec_enet_tx_queue(ndev, i);
    > > }
    > >
    > > -static int
    > > +static int __maybe_unused
    > > fec_enet_new_rxbdp(struct net_device *ndev, struct bufdesc *bdp, struct
    > sk_buff *skb)
    > > {
    > > struct fec_enet_private *fep = netdev_priv(ndev); @@ -1470,8
    > > +1519,9 @@ fec_enet_new_rxbdp(struct net_device *ndev, struct bufdesc
    > *bdp, struct sk_buff
    > > return 0;
    > > }
    > >
    > > -static bool fec_enet_copybreak(struct net_device *ndev, struct sk_buff **skb,
    > > - struct bufdesc *bdp, u32 length, bool swap)
    > > +static bool __maybe_unused
    > > +fec_enet_copybreak(struct net_device *ndev, struct sk_buff **skb,
    > > + struct bufdesc *bdp, u32 length, bool swap)
    > > {
    > > struct fec_enet_private *fep = netdev_priv(ndev);
    > > struct sk_buff *new_skb;
    > > @@ -1496,6 +1546,78 @@ static bool fec_enet_copybreak(struct net_device
    > *ndev, struct sk_buff **skb,
    > > return true;
    > > }
    > >
    > > +static void fec_enet_update_cbd(struct fec_enet_priv_rx_q *rxq,
    > > + struct bufdesc *bdp, int index) {
    > > + struct page *new_page;
    > > + dma_addr_t phys_addr;
    > > +
    > > + new_page = page_pool_dev_alloc_pages(rxq->page_pool);
    > > + WARN_ON(!new_page);
    > > + rxq->rx_skb_info[index].page = new_page;
    > > +
    > > + rxq->rx_skb_info[index].offset = FEC_ENET_XDP_HEADROOM;
    > > + phys_addr = page_pool_get_dma_addr(new_page) +
    > FEC_ENET_XDP_HEADROOM;
    > > + bdp->cbd_bufaddr = cpu_to_fec32(phys_addr); }
    > > +
    > > +static u32
    > > +fec_enet_run_xdp(struct fec_enet_private *fep, struct bpf_prog *prog,
    > > + struct xdp_buff *xdp, struct fec_enet_priv_rx_q *rxq,
    > > +int index) {
    > > + unsigned int sync, len = xdp->data_end - xdp->data;
    > > + u32 ret = FEC_ENET_XDP_PASS;
    > > + struct page *page;
    > > + int err;
    > > + u32 act;
    > > +
    > > + act = bpf_prog_run_xdp(prog, xdp);
    > > +
    > > + /* Due xdp_adjust_tail: DMA sync for_device cover max len CPU touch */
    > > + sync = xdp->data_end - xdp->data_hard_start -
    > FEC_ENET_XDP_HEADROOM;
    > > + sync = max(sync, len);
    > > +
    > > + switch (act) {
    > > + case XDP_PASS:
    > > + rxq->stats.xdp_pass++;
    > > + ret = FEC_ENET_XDP_PASS;
    > > + break;
    > > +
    > > + case XDP_TX:
    > > + rxq->stats.xdp_tx++;
    > > + bpf_warn_invalid_xdp_action(fep->netdev, prog, act);
    > > + fallthrough;
    >
    > This fallthrough looks wrong. The next xdp_do_redirect() call will pickup left-
    > overs in per CPU bpf_redirect_info.
    >

    So before the XDP_TX is implemented, this part of codes should reside below the XDP_REDIRECT case?

    > > +
    > > + case XDP_REDIRECT:
    > > + err = xdp_do_redirect(fep->netdev, xdp, prog);
    > > + rxq->stats.xdp_redirect++;
    > > - dma_unmap_single(&fep->pdev->dev,
    ...

    > > - fec32_to_cpu(bdp->cbd_bufaddr),
    > > - FEC_ENET_RX_FRSIZE - fep->rx_align,
    > > - DMA_FROM_DEVICE);
    > > - }
    > > -
    > > - prefetch(skb->data - NET_IP_ALIGN);
    > > + skb = build_skb(page_address(page), FEC_ENET_RX_FRSIZE);
    >
    > This looks wrong, I think FEC_ENET_RX_FRSIZE should be replaced by PAGE_SIZE.
    > As build_skb() does:
    >
    > size -= SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
    >

    Agree. As the current FRSIZE definition did not subtract the sizeof(struct skb_shared_info), I happened to not see the problem during the testing.

    Thanks,
    Shenwei

    > > + skb_reserve(skb, FEC_ENET_XDP_HEADROOM);
    >
    > The skb_reserve looks correct.
    >
    > > skb_put(skb, pkt_len - 4);
    > > data = skb->data;
    > > + page_pool_release_page(rxq->page_pool, page);
    >
    > Today page_pool have SKB recycle support (you might have looked at drivers
    > that didn't utilize this yet), thus you don't need to release the page
    > (page_pool_release_page) here. Instead you could simply mark the SKB for
    > recycling, unless driver does some page refcnt tricks I didn't notice.
    >
    > skb_mark_for_recycle(skb);
    >
    >
    > > - if (!is_copybreak && need_swap)
    > > + if (need_swap)
    > > swap_buffer(data, pkt_len);
    > >
    > > #if !defined(CONFIG_M5272)
    > > @@ -1649,16 +1781,6 @@ fec_enet_rx_queue(struct net_device *ndev, int
    > > budget, u16 queue_id)
    > [...]

    \
     
     \ /
      Last update: 2022-09-29 15:12    [W:2.661 / U:0.516 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site