From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ilias Apalodimas Subject: Re: [net-next, PATCH 2/2, v1] net: socionext: add AF_XDP support Date: Mon, 10 Sep 2018 14:20:38 +0300 Message-ID: <20180910112038.GA27542@apalos> References: <1536567880-15097-1-git-send-email-ilias.apalodimas@linaro.org> <1536567880-15097-3-git-send-email-ilias.apalodimas@linaro.org> <8bfd8219-acea-8b63-b6be-d17a7e3b6e24@lab.ntt.co.jp> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: netdev@vger.kernel.org, jaswinder.singh@linaro.org, ard.biesheuvel@linaro.org, masami.hiramatsu@linaro.org, arnd@arndb.de, mykyta.iziumtsev@linaro.org, bjorn.topel@intel.com, magnus.karlsson@intel.com To: Toshiaki Makita Return-path: Received: from mail-wr1-f65.google.com ([209.85.221.65]:42331 "EHLO mail-wr1-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727646AbeIJQOR (ORCPT ); Mon, 10 Sep 2018 12:14:17 -0400 Received: by mail-wr1-f65.google.com with SMTP id v17-v6so21474264wrr.9 for ; Mon, 10 Sep 2018 04:20:42 -0700 (PDT) Content-Disposition: inline In-Reply-To: <8bfd8219-acea-8b63-b6be-d17a7e3b6e24@lab.ntt.co.jp> Sender: netdev-owner@vger.kernel.org List-ID: On Mon, Sep 10, 2018 at 07:56:49PM +0900, Toshiaki Makita wrote: > On 2018/09/10 17:24, Ilias Apalodimas wrote: > > Add basic AF_XDP support without zero-copy > > > > Signed-off-by: Ilias Apalodimas > > --- > ... > > @@ -707,6 +731,26 @@ static int netsec_process_rx(struct netsec_priv *priv, int budget) > > if (unlikely(!buf_addr)) > > break; > > > > + if (xdp_prog) { > > + xdp_result = netsec_run_xdp(desc, priv, xdp_prog, > > + pkt_len); > > + if (xdp_result != NETSEC_XDP_PASS) { > > + xdp_flush |= xdp_result & NETSEC_XDP_REDIR; > > + > > + dma_unmap_single_attrs(priv->dev, > > + desc->dma_addr, > > + desc->len, DMA_TO_DEVICE, > > + DMA_ATTR_SKIP_CPU_SYNC); > > + > > + desc->len = desc_len; > > + desc->dma_addr = dma_handle; > > + desc->addr = buf_addr; > > + netsec_rx_fill(priv, idx, 1); > > + nsetsec_adv_desc(&dring->tail); > > + } > > + continue; > > Continue even on XDP_PASS? Is this really correct? > > Also seems there is no handling of adjust_head/tail for XDP_PASS case. No continue is wrong there, thanks for the catch. This should return the packet to the network stack. I'll fix it on v2. > > > + } > > + > > skb = build_skb(desc->addr, desc->len); > > if (unlikely(!skb)) { > > dma_unmap_single(priv->dev, dma_handle, desc_len, > > @@ -740,6 +784,9 @@ static int netsec_process_rx(struct netsec_priv *priv, int budget) > > nsetsec_adv_desc(&dring->tail); > > } > > > > + if (xdp_flush & NETSEC_XDP_REDIR) > > + xdp_do_flush_map(); > > + > > return done; > > } > ... > > +static u32 netsec_run_xdp(struct netsec_desc *desc, struct netsec_priv *priv, > > + struct bpf_prog *prog, u16 len) > > + > > +{ > > + struct netsec_desc_ring *dring = &priv->desc_ring[NETSEC_RING_RX]; > > + struct xdp_buff xdp; > > + u32 ret = NETSEC_XDP_PASS; > > + int err; > > + u32 act; > > + > > + xdp.data_hard_start = desc->addr; > > + xdp.data = desc->addr; > > There is no headroom. REDIRECT using devmap/cpumap will fail due to > this. Generally we need XDP_PACKET_HEADROOM. > Ok, i'll modify the patch to include XDP_PACKET_HEADROOM > > + xdp_set_data_meta_invalid(&xdp); > > + xdp.data_end = xdp.data + len; > > + xdp.rxq = &dring->xdp_rxq; > > + > > + rcu_read_lock(); > > + act = bpf_prog_run_xdp(prog, &xdp); > > + > > + switch (act) { > > + case XDP_PASS: > > + ret = NETSEC_XDP_PASS; > > + break; > > + case XDP_TX: > > + ret = netsec_xmit_xdp(priv, &xdp, desc); > > + break; > > + case XDP_REDIRECT: > > + err = xdp_do_redirect(priv->ndev, &xdp, prog); > > + if (!err) { > > + ret = NETSEC_XDP_REDIR; > > + } else { > > + ret = NETSEC_XDP_CONSUMED; > > + xdp_return_buff(&xdp); > > + } > > + break; > > + default: > > + bpf_warn_invalid_xdp_action(act); > > + /* fall through */ > > + case XDP_ABORTED: > > + trace_xdp_exception(priv->ndev, prog, act); > > + /* fall through -- handle aborts by dropping packet */ > > + case XDP_DROP: > > + ret = NETSEC_XDP_CONSUMED; > > + break; > > + } > > + > > + rcu_read_unlock(); > > + > > + return ret; > > +} > > + > > +static int netsec_xdp_setup(struct netsec_priv *priv, struct bpf_prog *prog) > > +{ > > + struct net_device *dev = priv->ndev; > > + struct bpf_prog *old_prog; > > + > > + /* For now just support only the usual MTU sized frames */ > > + if (prog && dev->mtu > 1500) { > > + netdev_warn(dev, "Jumbo frames not yet supported with XDP\n"); > > Why not using extack? > Wasn't aware that this should use the extack for errors. I just saw the same check on an existing driver and thought that was the way to go > > + return -EOPNOTSUPP; > > + } > > + Thanks for having a look! /Ilias