From mboxrd@z Thu Jan 1 00:00:00 1970 From: John Fastabend Subject: Re: [PATCH v5 bpf-next 2/9] veth: Add driver XDP Date: Thu, 26 Jul 2018 20:02:20 -0700 Message-ID: <2dd56ee3-08ff-49ef-8019-09e53ac86395@gmail.com> References: <20180726144032.2116-1-toshiaki.makita1@gmail.com> <20180726144032.2116-3-toshiaki.makita1@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: Toshiaki Makita , Jesper Dangaard Brouer , Jakub Kicinski To: Toshiaki Makita , netdev@vger.kernel.org, Alexei Starovoitov , Daniel Borkmann Return-path: Received: from mail-yb0-f193.google.com ([209.85.213.193]:36298 "EHLO mail-yb0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725829AbeG0EWF (ORCPT ); Fri, 27 Jul 2018 00:22:05 -0400 Received: by mail-yb0-f193.google.com with SMTP id s1-v6so1491622ybk.3 for ; Thu, 26 Jul 2018 20:02:23 -0700 (PDT) In-Reply-To: <20180726144032.2116-3-toshiaki.makita1@gmail.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 07/26/2018 07:40 AM, Toshiaki Makita wrote: > From: Toshiaki Makita > > This is the basic implementation of veth driver XDP. > > Incoming packets are sent from the peer veth device in the form of skb, > so this is generally doing the same thing as generic XDP. > > This itself is not so useful, but a starting point to implement other > useful veth XDP features like TX and REDIRECT. > > This introduces NAPI when XDP is enabled, because XDP is now heavily > relies on NAPI context. Use ptr_ring to emulate NIC ring. Tx function > enqueues packets to the ring and peer NAPI handler drains the ring. > > Currently only one ring is allocated for each veth device, so it does > not scale on multiqueue env. This can be resolved by allocating rings > on the per-queue basis later. > > Note that NAPI is not used but netif_rx is used when XDP is not loaded, > so this does not change the default behaviour. > > v3: > - Fix race on closing the device. > - Add extack messages in ndo_bpf. > > v2: > - Squashed with the patch adding NAPI. > - Implement adjust_tail. > - Don't acquire consumer lock because it is guarded by NAPI. > - Make poll_controller noop since it is unnecessary. > - Register rxq_info on enabling XDP rather than on opening the device. > > Signed-off-by: Toshiaki Makita > --- [...] One nit and one question. > + > +static struct sk_buff *veth_xdp_rcv_skb(struct veth_priv *priv, > + struct sk_buff *skb) > +{ > + u32 pktlen, headroom, act, metalen; > + void *orig_data, *orig_data_end; > + int size, mac_len, delta, off; > + struct bpf_prog *xdp_prog; > + struct xdp_buff xdp; > + > + rcu_read_lock(); > + xdp_prog = rcu_dereference(priv->xdp_prog); > + if (unlikely(!xdp_prog)) { > + rcu_read_unlock(); > + goto out; > + } > + > + mac_len = skb->data - skb_mac_header(skb); > + pktlen = skb->len + mac_len; > + size = SKB_DATA_ALIGN(VETH_XDP_HEADROOM + pktlen) + > + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)); > + if (size > PAGE_SIZE) > + goto drop; I'm not sure why it matters if size > PAGE_SIZE here. Why not just consume it and use the correct page order in alloc_page if its not linear. > + > + headroom = skb_headroom(skb) - mac_len; > + if (skb_shared(skb) || skb_head_is_locked(skb) || > + skb_is_nonlinear(skb) || headroom < XDP_PACKET_HEADROOM) { > + struct sk_buff *nskb; > + void *head, *start; > + struct page *page; > + int head_off; > + > + page = alloc_page(GFP_ATOMIC); Should also have __NO_WARN here as well this can be triggered by external events so we don't want DDOS here to flood system logs. > + if (!page) > + goto drop; > + > + head = page_address(page); > + start = head + VETH_XDP_HEADROOM; > + if (skb_copy_bits(skb, -mac_len, start, pktlen)) { > + page_frag_free(head); > + goto drop; > + } > + > + nskb = veth_build_skb(head, > + VETH_XDP_HEADROOM + mac_len, skb->len, > + PAGE_SIZE); > + if (!nskb) { > + page_frag_free(head); > + goto drop; > + } > + > + skb_copy_header(nskb, skb); > + head_off = skb_headroom(nskb) - skb_headroom(skb); > + skb_headers_offset_update(nskb, head_off); > + if (skb->sk) > + skb_set_owner_w(nskb, skb->sk); > + consume_skb(skb); > + skb = nskb; > + } > + > + xdp.data_hard_start = skb->head; > + xdp.data = skb_mac_header(skb); > + xdp.data_end = xdp.data + pktlen; > + xdp.data_meta = xdp.data; > + xdp.rxq = &priv->xdp_rxq; > + orig_data = xdp.data; > + orig_data_end = xdp.data_end; > + > + act = bpf_prog_run_xdp(xdp_prog, &xdp); > + > + switch (act) { > + case XDP_PASS: > + break; > + default: > + bpf_warn_invalid_xdp_action(act); > + case XDP_ABORTED: > + trace_xdp_exception(priv->dev, xdp_prog, act); > + case XDP_DROP: > + goto drop; > + } > + rcu_read_unlock(); > + > + delta = orig_data - xdp.data; > + off = mac_len + delta; > + if (off > 0) > + __skb_push(skb, off); > + else if (off < 0) > + __skb_pull(skb, -off); > + skb->mac_header -= delta; > + off = xdp.data_end - orig_data_end; > + if (off != 0) > + __skb_put(skb, off); > + skb->protocol = eth_type_trans(skb, priv->dev); > + > + metalen = xdp.data - xdp.data_meta; > + if (metalen) > + skb_metadata_set(skb, metalen); > +out: > + return skb; > +drop: > + rcu_read_unlock(); > + kfree_skb(skb); > + return NULL; > +} > + Thanks, John