bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Daniel Borkmann <daniel@iogearbox.net>
Cc: "John Fastabend" <john.fastabend@gmail.com>,
	"Toke Høiland-Jørgensen" <toke@redhat.com>,
	"Alexei Starovoitov" <ast@kernel.org>,
	"Martin KaFai Lau" <kafai@fb.com>,
	"Song Liu" <songliubraving@fb.com>, "Yonghong Song" <yhs@fb.com>,
	"Marek Majkowski" <marek@cloudflare.com>,
	"Lorenz Bauer" <lmb@cloudflare.com>,
	"David Miller" <davem@davemloft.net>,
	"Jesper Dangaard Brouer" <brouer@redhat.com>,
	netdev@vger.kernel.org, bpf@vger.kernel.org
Subject: Re: [PATCH bpf-next 0/9] xdp: Support multiple programs on a single interface through chain calls
Date: Wed, 2 Oct 2019 12:46:44 -0700	[thread overview]
Message-ID: <20191002194642.e77o45odwth5gil7@ast-mbp.dhcp.thefacebook.com> (raw)
In-Reply-To: <20191002191522.GA9196@pc-66.home>

On Wed, Oct 02, 2019 at 09:15:22PM +0200, Daniel Borkmann wrote:
> On Wed, Oct 02, 2019 at 09:43:49AM -0700, John Fastabend wrote:
> > Toke Høiland-Jørgensen wrote:
> > > This series adds support for executing multiple XDP programs on a single
> > > interface in sequence, through the use of chain calls, as discussed at the Linux
> > > Plumbers Conference last month:
> > > 
> > > https://linuxplumbersconf.org/event/4/contributions/460/
> > > 
> > > # HIGH-LEVEL IDEA
> > > 
> > > The basic idea is to express the chain call sequence through a special map type,
> > > which contains a mapping from a (program, return code) tuple to another program
> > > to run in next in the sequence. Userspace can populate this map to express
> > > arbitrary call sequences, and update the sequence by updating or replacing the
> > > map.
> > > 
> > > The actual execution of the program sequence is done in bpf_prog_run_xdp(),
> > > which will lookup the chain sequence map, and if found, will loop through calls
> > > to BPF_PROG_RUN, looking up the next XDP program in the sequence based on the
> > > previous program ID and return code.
> > > 
> > > An XDP chain call map can be installed on an interface by means of a new netlink
> > > attribute containing an fd pointing to a chain call map. This can be supplied
> > > along with the XDP prog fd, so that a chain map is always installed together
> > > with an XDP program.
> > > 
> > > # PERFORMANCE
> > > 
> > > I performed a simple performance test to get an initial feel for the overhead of
> > > the chain call mechanism. This test consists of running only two programs in
> > > sequence: One that returns XDP_PASS and another that returns XDP_DROP. I then
> > > measure the drop PPS performance and compare it to a baseline of just a single
> > > program that only returns XDP_DROP.
> > > 
> > > For comparison, a test case that uses regular eBPF tail calls to sequence two
> > > programs together is also included. Finally, because 'perf' showed that the
> > > hashmap lookup was the largest single source of overhead, I also added a test
> > > case where I removed the jhash() call from the hashmap code, and just use the
> > > u32 key directly as an index into the hash bucket structure.
> > > 
> > > The performance for these different cases is as follows (with retpolines disabled):
> > 
> > retpolines enabled would also be interesting.
> > 
> > > 
> > > | Test case                       | Perf      | Add. overhead | Total overhead |
> > > |---------------------------------+-----------+---------------+----------------|
> > > | Before patch (XDP DROP program) | 31.0 Mpps |               |                |
> > > | After patch (XDP DROP program)  | 28.9 Mpps |        2.3 ns |         2.3 ns |
> > 
> > IMO even 1 Mpps overhead is too much for a feature that is primarily about
> > ease of use. Sacrificing performance to make userland a bit easier is hard
> > to justify for me when XDP _is_ singularly about performance. Also that is
> > nearly 10% overhead which is fairly large. So I think going forward the
> > performance gab needs to be removed.
> 
> Fully agree, for the case where this facility is not used, it must have
> *zero* overhead. This is /one/ map flavor, in future there will be other
> facilities with different use-cases, but we cannot place them all into
> the critical fast-path. Given this is BPF, we have the flexibility that
> this can be hidden behind the scenes by rewriting and therefore only add
> overhead when used.
> 
> What I also see as a red flag with this proposal is the fact that it's
> tied to XDP only because you need to go and hack bpf_prog_run_xdp() all
> the way to fetch xdp->rxq->dev->xdp_chain_map even though the map/concept
> itself is rather generic and could be used in various other program types
> as well. I'm very sure that once there, people would request it. Therefore,
> better to explore a way where this has no changes to BPF_PROG_RUN() similar
> to the original tail call work.

two +1s.

1. new features have to have zero overhead when not used. this is not negotiable.
2. prog chaining is not xdp specific.

two years ago I was thinking about extending tail_call mechanism like this:
https://git.kernel.org/pub/scm/linux/kernel/git/ast/bpf.git/commit/?h=prog_chain&id=f54f45d00f91e083f6aec2abe35b6f0be52ae85b

and the program would call the new helper 'bpf_tail_call_next()' to jump
into the next program.
Sample code is here:
https://git.kernel.org/pub/scm/linux/kernel/git/ast/bpf.git/commit/?h=prog_chain&id=933a93208f1bd60a9707dc3f51a9aa457c86be87

In my benchmarks it was faster than existing bpf_tail_call via prog_array.
(And fit the rule of zero overhead when not used).

While coding it I figured that we can do proper indirect calls instead,
which would be even cleaner solution.
It would support arbitrary program chaining and calling.

The verifier back then didn't have enough infra to support indirect calls.
I suggest to look into implementing indirect calls instead of hacking
custom prog chaining logic via maps.


  parent reply	other threads:[~2019-10-02 19:46 UTC|newest]

Thread overview: 53+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-02 13:30 [PATCH bpf-next 0/9] xdp: Support multiple programs on a single interface through chain calls Toke Høiland-Jørgensen
2019-10-02 13:30 ` [PATCH bpf-next 1/9] hashtab: Add new bpf_map_fd_put_value op Toke Høiland-Jørgensen
2019-10-02 13:30 ` [PATCH bpf-next 2/9] xdp: Add new xdp_chain_map type for specifying XDP call sequences Toke Høiland-Jørgensen
2019-10-02 15:50   ` Lorenz Bauer
2019-10-02 18:25     ` Toke Høiland-Jørgensen
2019-10-02 13:30 ` [PATCH bpf-next 3/9] xdp: Support setting and getting device chain map Toke Høiland-Jørgensen
2019-10-02 15:50   ` Lorenz Bauer
2019-10-02 18:32     ` Toke Høiland-Jørgensen
2019-10-02 18:07   ` kbuild test robot
2019-10-02 18:29   ` kbuild test robot
2019-10-02 13:30 ` [PATCH bpf-next 4/9] xdp: Implement chain call logic to support multiple programs on one interface Toke Høiland-Jørgensen
2019-10-02 17:33   ` kbuild test robot
2019-10-02 17:53   ` kbuild test robot
2019-10-02 13:30 ` [PATCH bpf-next 5/9] tools/include/uapi: Add XDP chain map definitions Toke Høiland-Jørgensen
2019-10-02 13:30 ` [PATCH bpf-next 6/9] tools/libbpf_probes: Add support for xdp_chain map type Toke Høiland-Jørgensen
2019-10-02 13:30 ` [PATCH bpf-next 7/9] bpftool: Add definitions " Toke Høiland-Jørgensen
2019-10-02 13:30 ` [PATCH bpf-next 8/9] libbpf: Add support for setting and getting XDP chain maps Toke Høiland-Jørgensen
2019-10-02 13:30 ` [PATCH bpf-next 9/9] selftests: Add tests for XDP chain calls Toke Høiland-Jørgensen
2019-10-02 15:10 ` [PATCH bpf-next 0/9] xdp: Support multiple programs on a single interface through " Alan Maguire
2019-10-02 15:33   ` Toke Høiland-Jørgensen
2019-10-02 16:34     ` John Fastabend
2019-10-02 18:33       ` Toke Høiland-Jørgensen
2019-10-02 20:34         ` John Fastabend
2019-10-03  7:48           ` Toke Høiland-Jørgensen
2019-10-03 10:09             ` Jesper Dangaard Brouer
2019-10-03 19:45               ` John Fastabend
2019-10-02 16:35 ` Lorenz Bauer
2019-10-02 18:54   ` Toke Høiland-Jørgensen
2019-10-02 16:43 ` John Fastabend
2019-10-02 19:09   ` Toke Høiland-Jørgensen
2019-10-02 19:15   ` Daniel Borkmann
2019-10-02 19:29     ` Toke Høiland-Jørgensen
2019-10-02 19:46     ` Alexei Starovoitov [this message]
2019-10-03  7:58       ` Toke Høiland-Jørgensen
2019-10-02 18:38 ` Song Liu
2019-10-02 18:54   ` Song Liu
2019-10-02 19:25     ` Toke Høiland-Jørgensen
2019-10-03  8:53       ` Jesper Dangaard Brouer
2019-10-03 14:03         ` Alexei Starovoitov
2019-10-03 14:33           ` Toke Høiland-Jørgensen
2019-10-03 14:53             ` Edward Cree
2019-10-03 18:49               ` Jesper Dangaard Brouer
2019-10-03 19:35               ` John Fastabend
2019-10-04  8:09                 ` Toke Høiland-Jørgensen
2019-10-04 10:34                   ` Edward Cree
2019-10-04 15:58                     ` Lorenz Bauer
2019-10-07 16:43                       ` Edward Cree
2019-10-07 17:12                         ` Lorenz Bauer
2019-10-07 19:21                           ` Edward Cree
2019-10-07 21:01                         ` Alexei Starovoitov
2019-10-02 19:23   ` Toke Høiland-Jørgensen
2019-10-02 19:49     ` Song Liu
2019-10-03  7:59       ` Toke Høiland-Jørgensen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20191002194642.e77o45odwth5gil7@ast-mbp.dhcp.thefacebook.com \
    --to=alexei.starovoitov@gmail.com \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=brouer@redhat.com \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=john.fastabend@gmail.com \
    --cc=kafai@fb.com \
    --cc=lmb@cloudflare.com \
    --cc=marek@cloudflare.com \
    --cc=netdev@vger.kernel.org \
    --cc=songliubraving@fb.com \
    --cc=toke@redhat.com \
    --cc=yhs@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).