netdev - Re: [PATCH bpf-next v3 2/6] bpf: introduce BPF dispatcher

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 10 Dec 2019 06:54:33 +0100
From:   Björn Töpel <bjorn.topel@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Netdev <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Björn Töpel <bjorn.topel@...el.com>,
        bpf <bpf@...r.kernel.org>,
        Magnus Karlsson <magnus.karlsson@...il.com>,
        "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        Jonathan Lemon <jonathan.lemon@...il.com>,
        Edward Cree <ecree@...arflare.com>,
        Toke Høiland-Jørgensen <thoiland@...hat.com>,
        Andrii Nakryiko <andrii.nakryiko@...il.com>
Subject: Re: [PATCH bpf-next v3 2/6] bpf: introduce BPF dispatcher

On Tue, 10 Dec 2019 at 06:50, Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> On Mon, Dec 09, 2019 at 02:55:18PM +0100, Björn Töpel wrote:
> > +
> > +struct bpf_disp_prog {
> > +     struct bpf_prog *prog;
> > +     refcount_t users;
> > +};
> > +
> > +struct bpf_dispatcher {
> > +     void *func;
> > +     struct bpf_disp_prog progs[BPF_DISPATCHER_MAX];
> > +     int num_progs;
> > +     void *image;
> > +     u32 image_off;
> > +};
> > +
> > +static struct bpf_dispatcher *bpf_disp;
> > +
> > +static DEFINE_MUTEX(dispatcher_mutex);
> > +
> > +static struct bpf_dispatcher *bpf_dispatcher_lookup(void *func)
> > +{
> > +     struct bpf_dispatcher *d;
> > +     void *image;
> > +
> > +     if (bpf_disp) {
> > +             if (bpf_disp->func != func)
> > +                     return NULL;
> > +             return bpf_disp;
> > +     }
> > +
> > +     d = kzalloc(sizeof(*d), GFP_KERNEL);
> > +     if (!d)
> > +             return NULL;
>
> The bpf_dispatcher_lookup() above makes this dispatch logic a bit difficult to
> extend, since it works for only one bpf_disp and additional dispatchers would
> need hash table. Yet your numbers show that even with retpoline=off there is a
> performance benefit. So dispatcher probably can be reused almost as-is to
> accelerate sched_cls programs.
> What I was trying to say in my previous feedback on this subject is that
> lookup() doesn't need to exist. That 'void *func' doesn't need to be a function
> that dispatcher uses. It can be 'struct bpf_dispatcher *' instead.
> And lookup() becomes init().
> Then bpf_dispatcher_change_prog() will be passing &bpf_dispatcher_xdp
> and bpf_dispatcher_xdp is defined via macro that supplies
> 'struct bpf_dispatcher' above and instantiated in particular .c file
> that used that macro. Then dispatcher can be used in more than one place.
> No need for hash table. Multiple dispatchers are instantiated in places
> that need them via macro.
> The code will look like:
> bpf_prog_change_xdp(struct bpf_prog *prev_prog, struct bpf_prog *prog)
> {
>    bpf_dispatcher_change_prog(&bpf_dispatcher_xdp, prev_prog, prog);
> }
> Similarly sched_cls dispatcher for skb progs will do:
>    bpf_dispatcher_change_prog(&bpf_dispatcher_tc, prev_prog, prog);
> wdyt?
>

Yes, much cleaner. I'll respin!