Message-ID: <CAJ+HfNhXq=17650ztPcnTSP4ztj8K1zwbC-GojYkZviPBdOGxA@mail.gmail.com>
Date:   Fri, 8 May 2020 11:09:33 +0200
From:   Björn Töpel <bjorn.topel@...il.com>
To:     John Fastabend <john.fastabend@...il.com>
Cc:     Toke Høiland-Jørgensen <toke@...hat.com>,
        bpf <bpf@...r.kernel.org>, Netdev <netdev@...r.kernel.org>,
        Jesper Dangaard Brouer <brouer@...hat.com>
Subject: Re: XDP bpf_tail_call_redirect(): yea or nay?

On Thu, 7 May 2020 at 20:08, John Fastabend <john.fastabend@...il.com> wrote:
>
[]
>
> I'm wondering if we can teach the verifier to recognize tail calls,
>
> int xdp_prog1(struct xdp_md *ctx)
> {
>         return xdp_do_redirect(ctx, &xsks_map, 0);
> }
>
> This would be useful for normal calls as well. I guess the question here
> is would a tail call be sufficient for above case or do you need the
> 'return XDP_PASS' at the end? If so maybe we could fold it into the
> helper somehow.
>

No, that was just for handling the "failed call", bpf_tail_call() style.

> I think it would also address Toke's concerns, no new action so
> bpf developers can just develope like normal but "smart" developers
> will try do calls as tail calls. Not sure it can be done without
> driver changes though.
>

Take me though this. So, the new xdp_do_redirect() would return
XDP_REDIRECT? If the call is a tail call, we can "consume" (perform
the REDIRECT action) in the helper, set a "we're done/tail call
performed" flag in bpf_redirect_info and the xdp_do_redirect() checks
this flag and returns directly. If the call is *not* a tail call, the
regular REDIRECT path is performed. Am I following that correctly? So
we would be able to detect if the optimization has been performed, so
the "consume" semantics can be done.

Or do you mean that xdp_do_redirect() is only allowed if it can be
tailcall optimized?

int xdp_prog1(struct xdp_md *ctx)
{
        int ret;

        ret = xdp_do_redirect(ctx, &xsks_map, 0);
        // If xdp_do_redirect() consumes the context, the ctx is stale
        // here.
        ...
        return ret;
}

Let me clarify what I'm trying to do. bpf_redirect_map() performs a
lookup into the map. It would make sense to perform the action here as
well, since everything (maptype/item)is known. Today,
bpf_redirect_info is populated, and then the maptype is looked up
again from xdp_do_redirect(), and bpf_redirect_info() is cleared. I'd
like to get rid of bpf_redirect_info and xdp_do_redirect(), when
possible.

> > >> >
> > >> > -->8--
> > >> >
> > >> > The bpf_tail_call_redirect() would work with all redirectable maps.
> > >> >
> > >> > Thoughts? Tomatoes? Pitchforks?
> > >>
> > >> The above answers the 'what'. Might be easier to evaluate if you also
> > >> included the 'why'? :)
> > >>
> > >
> > > Ah! Sorry! Performance, performance, performance. Getting rid of a
> > > bunch of calls/instructions per packet, which helps my (AF_XDP) case.
> > > This would be faster than the regular REDIRECT path. Today, in
> > > bpf_redirect_map(), instead of actually performing the action, we
> > > populate the bpf_redirect_info structure, just to look up the action
> > > again in xdp_do_redirect().
> > >
> > > I'm pretty certain this would be a gain for AF_XDP (quite easy to do a
> > > quick hack, and measure). It would also shave off the same amount of
> > > instructions for "vanilla" XDP_REDIRECT cases. The bigger issue; Is
> > > this new semantic something people would be comfortable being added to
> > > XDP.
> >
> > Well, my immediate thought would be that the added complexity would not
> > be worth it, because:
> >
> > - A new action would mean either you'd need to patch all drivers or
> >   (more likely) we'd end up with yet another difference between drivers'
> >   XDP support.
> >
> > - BPF developers would suddenly have to choose - do this new faster
> >   thing, or be compatible? And manage the choice based on drivers they
> >   expect to run on, etc. This was already confusing with
> >   bpf_redirect()/bpf_redirect_map(), and this would introduce a third
> >   option!
> >
> > So in light of this, I'd say the performance benefit would have to be
> > quite substantial for this to be worth it. Which we won't know until you
> > try it, I guess :)
>
> Knowing the number would be useful. But if it can be done in general
> way it may not need to be as high because its not a special xdp thing.
>

Yeah, I need to do some experimentation here!

> >
> > Thinking of alternatives - couldn't you shoe-horn this into the existing
> > helper and return code? Say, introduce an IMMEDIATE_RETURN flag to the
> > existing helpers, which would change the behaviour to the tail call
> > semantics. When used, xdp_do_redirect() would then return immediately
> > (or you could even turn xdp_do_redirect() into an inlined wrapper that
> > checks the flag before issuing a CALL to the existing function). Any
> > reason why that wouldn't work?
>
> I think it would work but I it would be even nicer if clang, verifier
> and jit caught the tail call pattern and did it automatically.
>

Yes. :-)


Thanks for the input, and the ideas!
Björn

> >
> > -Toke
> >

Powered by blists - more mailing lists