Date:   Fri, 13 Oct 2017 10:17:57 +0200
From:   Jesper Dangaard Brouer <brouer@...hat.com>
To:     Edward Cree <ecree@...arflare.com>
Cc:     <netdev@...r.kernel.org>, <jakub.kicinski@...ronome.com>,
        "Michael S. Tsirkin" <mst@...hat.com>, <pavel.odintsov@...il.com>,
        Jason Wang <jasowang@...hat.com>, <mchan@...adcom.com>,
        John Fastabend <john.fastabend@...il.com>,
        <peter.waskiewicz.jr@...el.com>, <ast@...erby.dk>,
        Daniel Borkmann <borkmann@...earbox.net>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Andy Gospodarek <andy@...yhouse.net>, brouer@...hat.com
Subject: Re: [net-next V7 PATCH 1/5] bpf: introduce new bpf cpu map type
 BPF_MAP_TYPE_CPUMAP


On Thu, 12 Oct 2017 21:35:05 +0100 Edward Cree <ecree@...arflare.com> wrote:

> On 12/10/17 13:26, Jesper Dangaard Brouer wrote:
> > The 'cpumap' is primary used as a backend map for XDP BPF helper  
> s/primary/primarily.
>  [...]  
> Again, s/primary/primarily.
> > + * call bpf_redirect_map() and XDP_REDIRECT action, like 'devmap'.
> > + *
> > + * Unlike devmap which redirect XDP frames out another NIC device,
> > + * this map type redirect raw XDP frames to another CPU.  The remote  
> Also I think both of these 'redirect' should be 'redirects', just a
>  grammatical nit pick ;)
> > + * CPU will do SKB-allocation and call the normal network stack.
> > + *
> > + * This is a scalability and isolation mechanism, that allow
> > + * separating the early driver network XDP layer, from the rest of the
> > + * netstack, and assigning dedicated CPUs for this stage.  This
> > + * basically allows for 10G wirespeed pre-filtering via bpf.
> > + */
> > +#include <linux/bpf.h>
> > +#include <linux/filter.h>
> > +#include <linux/ptr_ring.h>
> > +
> > +#include <linux/sched.h>
> > +#include <linux/workqueue.h>
> > +#include <linux/kthread.h>
> > +#include <linux/capability.h>
> > +
> > +/* General idea: XDP packets getting XDP redirected to another CPU,
> > + * will maximum be stored/queued for one driver ->poll() call.  It is
> > + * guaranteed that setting flush bit and flush operation happen on
> > + * same CPU.  Thus, cpu_map_flush operation can deduct via this_cpu_ptr()
> > + * which queue in bpf_cpu_map_entry contains packets.
> > + */
> > +
> > +#define CPU_MAP_BULK_SIZE 8  /* 8 == one cacheline on 64-bit archs */
> > +struct xdp_bulk_queue {
> > +	void *q[CPU_MAP_BULK_SIZE];
> > +	unsigned int count;
> > +};  
>
> I realise it's a bit late to say this on a v7, but it might be better to
>  use a linked-list (list_heads) here instead of an array.  Then, the
>  struct xdp_pkt you store in the packet headroom could contain the
>  list_head, there's no arbitrary bulking limit, and the flush just has
>  to link the newly-created elements into the receiving CPU's list.
> Is there an obvious reason why this wouldn't work / can't perform as
>  well, or should I try it and benchmark it?

No, I've tried to explain this before.  I do want a bulking limit for
several reasons. (1) This is connected to how ptr_ring works. I do want
to have a full cache-line to transfer/enqueue into the ptr_ring.  The
ptr_ring is the key to making the transfer between CPUs work so
efficiently (I even reject my own alf_queue in favor of ptr_ring).
(2) Due to latency concerns, I don't want to "wait" for 64 packets before
the remote CPU get a chance to see these. I want to transfer/enqueue
packets to the remote CPU as soon as possible, and due to cacheline
constraints this is 8 packets.

The ptr_ring goes to great lengths to avoid cache-line bouncing. Like
fb9de9704775 ("ptr_ring: batch ring zeroing") which helps avoid cache
line bouncing when queue is full. When queue is almost empty,
cache-line bouncing still occurs.  Which is what I'm trying to minimize
here by transfering/enqueueing a full cacheline.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Powered by blists - more mailing lists