> On Thu, 10 Oct 2019 01:18:35 +0200
> Lorenzo Bianconi <lore...@kernel.org> wrote:
>
> > Add basic XDP support to mvneta driver for devices that rely on software
> > buffer management. Currently supported verdicts are:
> > - XDP_DROP
> > - XDP_PASS
> > - XDP_REDIRECT
> > - XDP_ABORTED
> >
> > Signed-off-by: Lorenzo Bianconi <lore...@kernel.org>
> > ---
> > drivers/net/ethernet/marvell/mvneta.c | 144 ++++++++++++++++++++++++--
> > 1 file changed, 135 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/marvell/mvneta.c
> > b/drivers/net/ethernet/marvell/mvneta.c
> > index ba4aa9bbc798..e2795dddbcaf 100644
> > --- a/drivers/net/ethernet/marvell/mvneta.c
> > +++ b/drivers/net/ethernet/marvell/mvneta.c
> [...]
>
> > @@ -1950,16 +1960,60 @@ int mvneta_rx_refill_queue(struct mvneta_port *pp,
> > struct mvneta_rx_queue *rxq)
> > return i;
> > }
> >
> > +static int
> > +mvneta_run_xdp(struct mvneta_port *pp, struct bpf_prog *prog,
> > + struct xdp_buff *xdp)
> > +{
> > + u32 ret, act = bpf_prog_run_xdp(prog, xdp);
> > +
> > + switch (act) {
> > + case XDP_PASS:
> > + ret = MVNETA_XDP_PASS;
> > + break;
> > + case XDP_REDIRECT: {
> > + int err;
> > +
> > + err = xdp_do_redirect(pp->dev, xdp, prog);
> > + if (err) {
> > + ret = MVNETA_XDP_CONSUMED;
> > + xdp_return_buff(xdp);
>
> > + } else {
> > + ret = MVNETA_XDP_REDIR;
> > + }
> > + break;
> > + }
> > + default:
> > + bpf_warn_invalid_xdp_action(act);
> > + /* fall through */
> > + case XDP_ABORTED:
> > + trace_xdp_exception(pp->dev, prog, act);
> > + /* fall through */
> > + case XDP_DROP:
> > + ret = MVNETA_XDP_CONSUMED;
> > + xdp_return_buff(xdp);
>
> Using xdp_return_buff() here is actually not optimal for performance.
> I can see that others socionext/netsec.c and AF_XDP also use this
> xdp_return_buff().
>
> I do think code wise it looks a lot nice to use xdp_return_buff(), so
> maybe we should optimize xdp_return_buff(), instead of using
> page_pool_recycle_direct() here? (That would also help AF_XDP ?)
>
> The problem with xdp_return_buff() is that it does a "full" lookup from
> the mem.id (xdp_buff->xdp_rxq_info->mem.id) to find the "allocator"
> pointer in this case the page_pool pointer. Here in the driver we
> already have access to the stable allocator page_pool pointer via
> struct mvneta_rx_queue *rxq->page_pool.
ack, right. Thx for pointing it out. I will fix it in v3
>
>
> > + break;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > static int
> > mvneta_swbm_rx_frame(struct mvneta_port *pp,
> > struct mvneta_rx_desc *rx_desc,
> > struct mvneta_rx_queue *rxq,
> > - struct page *page)
> > + struct bpf_prog *xdp_prog,
> > + struct page *page, u32 *xdp_ret)
> > {
> > unsigned char *data = page_address(page);
> > int data_len = -MVNETA_MH_SIZE, len;
> > struct net_device *dev = pp->dev;
> > enum dma_data_direction dma_dir;
> > + struct xdp_buff xdp = {
> > + .data_hard_start = data,
> > + .data = data + MVNETA_SKB_HEADROOM + MVNETA_MH_SIZE,
> > + .rxq = &rxq->xdp_rxq,
> > + };
>
> Creating the struct xdp_buff (on call-stack) this way is not optimal
> for performance (IHMO it looks nicer code-wise, but too bad).
>
> This kind of initialization of only some of the members, cause GCC to
> zero out other members (I observed this on Intel, which use an
> expensive rep-sto operation). Thus, this cause extra unnecessary memory
> writes.
>
> A further optimization, is that you can avoid re-assigning:
> rxq = &rxq->xdp_rxq
> for each frame, as this actually stays the same for all the frames in
> this NAPI cycle. By instead allocating the xdp_buff on the callers
> stack, and parsing in xdp_buff as a pointer.
ack, will fix it in v3.
Regards,
Lorenzo
>
>
> > + xdp_set_data_meta_invalid(&xdp);
> >
> > if (MVNETA_SKB_SIZE(rx_desc->data_size) > PAGE_SIZE) {
> > len = MVNETA_MAX_RX_BUF_SIZE;
> > @@ -1968,13 +2022,27 @@ mvneta_swbm_rx_frame(struct mvneta_port *pp,
> > len = rx_desc->data_size;
> > data_len += len - ETH_FCS_LEN;
> > }
> > + xdp.data_end = xdp.data + data_len;
> >
> > dma_dir = page_pool_get_dma_dir(rxq->page_pool);
> > dma_sync_single_range_for_cpu(dev->dev.parent,
> > rx_desc->buf_phys_addr, 0,
> > len, dma_dir);
> >
> > - rxq->skb = build_skb(data, PAGE_SIZE);
> > + if (xdp_prog) {
> > + u32 ret;
> > +
> > + ret = mvneta_run_xdp(pp, xdp_prog, &xdp);
> > + if (ret != MVNETA_XDP_PASS) {
> > + mvneta_update_stats(pp, 1, xdp.data_end - xdp.data,
> > + false);
> > + rx_desc->buf_phys_addr = 0;
> > + *xdp_ret |= ret;
> > + return ret;
> > + }
> > + }
> > +
> > + rxq->skb = build_skb(xdp.data_hard_start, PAGE_SIZE);
> > if (unlikely(!rxq->skb)) {
> > netdev_err(dev,
> > "Can't allocate skb on queue %d\n",
> [...]
>
> --
> Best regards,
> Jesper Dangaard Brouer
> MSc.CS, Principal Kernel Engineer at Red Hat
> LinkedIn: http://www.linkedin.com/in/brouer
signature.asc
Description: PGP signature
