Re: [RFC PATCH bpf-next 2/4] bpf: introduce BPF dispatcher

["Björn Töpel" <bjorn.topel@gmail.com>]

On Wed, 13 Nov 2019 at 22:41, Edward Cree <ecree@solarflare.com> wrote:
>
> On 13/11/2019 20:47, Björn Töpel wrote:
> > From: Björn Töpel <bjorn.topel@intel.com>
> >
> > The BPF dispatcher builds on top of the BPF trampoline ideas;
> > Introduce bpf_arch_text_poke() and (re-)use the BPF JIT generate
> > code. The dispatcher builds a dispatch table for XDP programs, for
> > retpoline avoidance. The table is a simple binary search model, so
> > lookup is O(log n). Here, the dispatch table is limited to four
> > entries (for laziness reason -- only 1B relative jumps :-P). If the
> > dispatch table is full, it will fallback to the retpoline path.
> >
> > An example: A module/driver allocates a dispatcher. The dispatcher is
> > shared for all netdevs. Each netdev allocate a slot in the dispatcher
> > and a BPF program. The netdev then uses the dispatcher to call the
> > correct program with a direct call (actually a tail-call).
> >
> > Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
> The first-come-first-served model for dispatcher slots might mean that
>  a low-traffic user ends up getting priority while a higher-traffic
>  user is stuck with the retpoline fallback.  Have you considered using
>  a learning mechanism, like in my dynamic call RFC [1] earlier this
>  year?  (Though I'm sure a better learning mechanism than the one I
>  used there could be devised.)
>

My rationale was that this mechanism would almost exclusively be used
by physical HW NICs using XDP. My hunch was that the number of netdevs
would be ~4, and typically less using XDP, so a more sophisticated
mechanism didn't really make sense IMO. However, your approach is more
generic and doesn't require any arch specific work. What was the push
back for your work? I'll read up on the thread. I'm intrigued to take
your series for a spin!

> > +static int bpf_dispatcher_add_prog(struct bpf_dispatcher *d,
> > +                                struct bpf_prog *prog)
> > +{
> > +     struct bpf_prog **entry = NULL;
> > +     int i, err = 0;
> > +
> > +     if (d->num_progs == BPF_DISPATCHER_MAX)
> > +             return err;
> > +
> > +     for (i = 0; i < BPF_DISPATCHER_MAX; i++) {
> > +             if (!entry && !d->progs[i])
> > +                     entry = &d->progs[i];
> > +             if (d->progs[i] == prog)
> > +                     return err;
> > +     }
> > +
> > +     prog = bpf_prog_inc(prog);
> > +     if (IS_ERR(prog))
> > +             return err;
> > +
> > +     *entry = prog;
> > +     d->num_progs++;
> > +     return err;
> > +}
> If I'm reading this function right, it always returns zero; was that
>  the intention, and if so why isn't it void?
>

Ugh, yeah. In general the error handling should be improved in this
RFC. If it makes sense to move forward with this series, I'll make
sure to address that. Thanks for taking a look, and for the pointers
to your work!


Cheers,
Björn

> -Ed
>
> [1] https://lkml.org/lkml/2019/2/1/948