Message-ID: <5eb44eb03f8e1_22a22b23544285b87a@john-XPS-13-9370.notmuch>
Date:   Thu, 07 May 2020 11:08:48 -0700
From:   John Fastabend <john.fastabend@...il.com>
To:     Toke Høiland-Jørgensen <toke@...hat.com>,
        Björn Töpel <bjorn.topel@...il.com>
Cc:     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?

Toke Høiland-Jørgensen wrote:
> Björn Töpel <bjorn.topel@...il.com> writes:
> 
> > On Thu, 7 May 2020 at 15:44, Toke Høiland-Jørgensen <toke@...hat.com> wrote:
> >>
> >> Björn Töpel <bjorn.topel@...il.com> writes:
> >>
> >> > Before I start hacking on this, I might as well check with the XDP
> >> > folks if this considered a crappy idea or not. :-)
> >> >
> >> > The XDP redirect flow for a packet is typical a dance of
> >> > bpf_redirect_map() that updates the bpf_redirect_info structure with
> >> > maps type/items, which is then followed by an xdp_do_redirect(). That
> >> > function takes an action based on the bpf_redirect_info content.
> >> >
> >> > I'd like to get rid of the xdp_do_redirect() call, and the
> >> > bpf_redirect_info (per-cpu) lookup. The idea is to introduce a new
> >> > (oh-no!) XDP action, say, XDP_CONSUMED and a built-in helper with
> >> > tail-call semantics.
> >> >
> >> > Something across the lines of:
> >> >
> >> > --8<--
> >> >
> >> > struct {
> >> >         __uint(type, BPF_MAP_TYPE_XSKMAP);
> >> >         __uint(max_entries, MAX_SOCKS);
> >> >         __uint(key_size, sizeof(int));
> >> >         __uint(value_size, sizeof(int));
> >> > } xsks_map SEC(".maps");
> >> >
> >> > SEC("xdp1")
> >> > int xdp_prog1(struct xdp_md *ctx)
> >> > {
> >> >         bpf_tail_call_redirect(ctx, &xsks_map, 0);
> >> >         // Redirect the packet to an AF_XDP socket at entry 0 of the
> >> >         // map.
> >> >         //
> >> >         // After a successful call, ctx is said to be
> >> >         // consumed. XDP_CONSUMED will be returned by the program.
> >> >         // Note that if the call is not successful, the buffer is
> >> >         // still valid.
> >> >         //
> >> >         // XDP_CONSUMED in the driver means that the driver should not
> >> >         // issue an xdp_do_direct() call, but only xdp_flush().
> >> >         //
> >> >         // The verifier need to be taught that XDP_CONSUMED can only
> >> >         // be returned "indirectly", meaning a bpf_tail_call_XXX()
> >> >         // call. An explicit "return XDP_CONSUMED" should be
> >> >         // rejected. Can that be implemented?
> >> >         return XDP_PASS; // or any other valid action.
> >> > }

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.

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.

> >> >
> >> > -->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.

> 
> 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.

> 
> -Toke
> 

Powered by blists - more mailing lists