| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Fri, 6 Oct 2017 13:17:48 +0200
From: Jesper Dangaard Brouer <brouer@...hat.com>
To: Daniel Borkmann <daniel@...earbox.net>
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,
Daniel Borkmann <borkmann@...earbox.net>,
Alexei Starovoitov <alexei.starovoitov@...il.com>,
Andy Gospodarek <andy@...yhouse.net>, brouer@...hat.com
Subject: Re: [net-next V4 PATCH 2/5] bpf: XDP_REDIRECT enable use of cpumap
On Thu, 05 Oct 2017 12:10:13 +0200 Daniel Borkmann <daniel@...earbox.net> wrote:
> On 10/04/2017 02:03 PM, Jesper Dangaard Brouer wrote:
> > This patch connects cpumap to the xdp_do_redirect_map infrastructure.
> >
> > Still no SKB allocation are done yet. The XDP frames are transferred
> > to the other CPU, but they are simply refcnt decremented on the remote
> > CPU. This served as a good benchmark for measuring the overhead of
> > remote refcnt decrement. If driver page recycle cache is not
> > efficient then this, exposes a bottleneck in the page allocator.
> >
> > A shout-out to MST's ptr_ring, which is the secret behind is being so
> > efficient to transfer memory pointers between CPUs, without constantly
> > bouncing cache-lines between CPUs.
> >
> > V3: Handle !CONFIG_BPF_SYSCALL pointed out by kbuild test robot.
> >
> > V4: Make Generic-XDP aware of cpumap type, but don't allow redirect yet,
> > as implementation require a separate upstream discussion.
> >
> > Signed-off-by: Jesper Dangaard Brouer <brouer@...hat.com>
> [...]
> > diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c
> > index ae8e29352261..4926a9971f90 100644
> > --- a/kernel/bpf/cpumap.c
> > +++ b/kernel/bpf/cpumap.c
> > @@ -493,7 +493,8 @@ static int bq_enqueue(struct bpf_cpu_map_entry *rcpu, struct xdp_pkt *xdp_pkt)
> > return 0;
> > }
> >
> > -int cpu_map_enqueue(struct bpf_cpu_map_entry *rcpu, struct xdp_buff *xdp)
> > +int cpu_map_enqueue(struct bpf_cpu_map_entry *rcpu, struct xdp_buff *xdp,
> > + struct net_device *dev_rx)
> > {
> > struct xdp_pkt *xdp_pkt;
> > int headroom;
> > @@ -505,7 +506,7 @@ int cpu_map_enqueue(struct bpf_cpu_map_entry *rcpu, struct xdp_buff *xdp)
> > xdp_pkt = xdp->data_hard_start;
> > xdp_pkt->data = xdp->data;
> > xdp_pkt->len = xdp->data_end - xdp->data;
> > - xdp_pkt->headroom = headroom;
> > + xdp_pkt->headroom = headroom - sizeof(*xdp_pkt);
>
> (Just a note, bit confusing that first two patches add and extend
> this, and only in the third you add the xdp->data_meta handling,
> makes it harder to review at least.)
Sorry. This is a left-overs from rebasing and measuring the cost of
transferring only the pointer to the page, and remote put_page().
And your xdp->data_meta, happen basically while my patches was in-flight.
I'll move this one-line back to patch 2, to spreading over too many
patches.
> [...]
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 9b6e7e84aafd..dbf2ae071108 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -2521,10 +2521,36 @@ static int __bpf_tx_xdp(struct net_device *dev,
> > err = dev->netdev_ops->ndo_xdp_xmit(dev, xdp);
> > if (err)
> > return err;
> > - if (map)
> > + dev->netdev_ops->ndo_xdp_flush(dev);
> > + return 0;
> > +}
> > +
> > +static int __bpf_tx_xdp_map(struct net_device *dev_rx, void *fwd,
> > + struct bpf_map *map,
> > + struct xdp_buff *xdp,
> > + u32 index)
> > +{
> > + int err;
> > +
> > + if (map->map_type == BPF_MAP_TYPE_DEVMAP) {
> > + struct net_device *dev = fwd;
> > +
> > + if (!dev->netdev_ops->ndo_xdp_xmit)
> > + return -EOPNOTSUPP;
> > +
> > + err = dev->netdev_ops->ndo_xdp_xmit(dev, xdp);
> > + if (err)
> > + return err;
> > __dev_map_insert_ctx(map, index);
> > - else
> > - dev->netdev_ops->ndo_xdp_flush(dev);
> > +
> > + } else if (map->map_type == BPF_MAP_TYPE_CPUMAP) {
> > + struct bpf_cpu_map_entry *rcpu = fwd;
> > +
> > + err = cpu_map_enqueue(rcpu, xdp, dev_rx);
> > + if (err)
> > + return err;
> > + __cpu_map_insert_ctx(map, index);
> > + }
> > return 0;
> > }
> >
> > @@ -2534,11 +2560,33 @@ void xdp_do_flush_map(void)
> > struct bpf_map *map = ri->map_to_flush;
> >
> > ri->map_to_flush = NULL;
> > - if (map)
> > - __dev_map_flush(map);
> > + if (map) {
> > + switch (map->map_type) {
> > + case BPF_MAP_TYPE_DEVMAP:
> > + __dev_map_flush(map);
> > + break;
> > + case BPF_MAP_TYPE_CPUMAP:
> > + __cpu_map_flush(map);
> > + break;
> > + default:
> > + break;
> > + }
> > + }
> > }
> > EXPORT_SYMBOL_GPL(xdp_do_flush_map);
> >
> > +static void *__xdp_map_lookup_elem(struct bpf_map *map, u32 index)
> > +{
> > + switch (map->map_type) {
> > + case BPF_MAP_TYPE_DEVMAP:
> > + return __dev_map_lookup_elem(map, index);
> > + case BPF_MAP_TYPE_CPUMAP:
> > + return __cpu_map_lookup_elem(map, index);
> > + default:
> > + return NULL;
> > + }
>
> Should we just have a callback and instead of the above use
> map->ptr_lookup_elem() (or however we name it) ... lot of it
> is pretty much the same logic as with devmap.
We could extend struct bpf_map *map with such a callback, I was just
afraid that this would be too invasive.
Performance wise, I don't thinks will hurt too much.
http://www.cipht.net/2017/10/03/are-jump-tables-always-fastest.html
> > +}
> > +
> > static inline bool xdp_map_invalid(const struct bpf_prog *xdp_prog,
> > unsigned long aux)
> > {
> > @@ -2551,8 +2599,8 @@ static int xdp_do_redirect_map(struct net_device *dev, struct xdp_buff *xdp,
> > struct redirect_info *ri = this_cpu_ptr(&redirect_info);
> > unsigned long map_owner = ri->map_owner;
> > struct bpf_map *map = ri->map;
> > - struct net_device *fwd = NULL;
> > u32 index = ri->ifindex;
> > + void *fwd = NULL;
> > int err;
> >
> > ri->ifindex = 0;
>
--
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