From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Cc: Yonghong Song <yhs@fb.com>, Andrii Nakryiko <andriin@fb.com>,
bpf <bpf@vger.kernel.org>, Martin KaFai Lau <kafai@fb.com>,
Networking <netdev@vger.kernel.org>,
Alexei Starovoitov <ast@fb.com>,
Daniel Borkmann <daniel@iogearbox.net>,
Kernel Team <kernel-team@fb.com>
Subject: Re: [RFC PATCH bpf-next 05/16] bpf: create file or anonymous dumpers
Date: Thu, 16 Apr 2020 10:04:14 -0700 [thread overview]
Message-ID: <20200416170414.ds3hcb3bgfetjt4v@ast-mbp> (raw)
In-Reply-To: <CAEf4Bza2YkmFMZ_d6d6keLqeWNDr8dbBQj=42xSrONaULK1PXg@mail.gmail.com>
On Wed, Apr 15, 2020 at 06:48:13PM -0700, Andrii Nakryiko wrote:
> On Wed, Apr 15, 2020 at 9:46 AM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Tue, Apr 14, 2020 at 09:45:08PM -0700, Andrii Nakryiko wrote:
> > > >
> > > > > FD is closed, dumper program is detached and dumper is destroyed
> > > > > (unless pinned in bpffs, just like with any other bpf_link.
> > > > > 3. At this point bpf_dumper_link can be treated like a factory of
> > > > > seq_files. We can add a new BPF_DUMPER_OPEN_FILE (all names are for
> > > > > illustration purposes) command, that accepts dumper link FD and
> > > > > returns a new seq_file FD, which can be read() normally (or, e.g.,
> > > > > cat'ed from shell).
> > > >
> > > > In this case, link_query may not be accurate if a bpf_dumper_link
> > > > is created but no corresponding bpf_dumper_open_file. What we really
> > > > need to iterate through all dumper seq_file FDs.
> > >
> > > If the goal is to iterate all the open seq_files (i.e., bpfdump active
> > > sessions), then bpf_link is clearly not the right approach. But I
> > > thought we are talking about iterating all the bpfdump programs
> > > attachments, not **sessions**, in which case bpf_link is exactly the
> > > right approach.
> >
> > That's an important point. What is the pinned /sys/kernel/bpfdump/tasks/foo ?
>
> Assuming it's not a rhetorical question, foo is a pinned bpf_dumper
> link (in my interpretation of all this).
It wasn't rhetorical question and your answer is differrent from mine :)
It's not a link. It's a template of seq_file. It's the same as
$ stat /proc/net/ipv6_route
File: ‘/proc/net/ipv6_route’
Size: 0 Blocks: 0 IO Block: 1024 regular empty file
> > Every time 'cat' opens it a new seq_file is created with new FD, right ?
>
> yes
>
> > Reading of that file can take infinite amount of time, since 'cat' can be
> > paused in the middle.
>
> yep, correct (though most use case probably going to be very short-lived)
>
> > I think we're dealing with several different kinds of objects here.
> > 1. "template" of seq_file that is seen with 'ls' in /sys/kernel/bpfdump/
>
> Let's clarify here again, because this can be interpreted differently.
>
> Are you talking about, e.g., /sys/fs/bpfdump/task directory that
> defines what class of items should be iterated? Or you are talking
> about named dumper: /sys/fs/bpfdump/task/my_dumper?
the latter.
>
> If the former, I agree that it's not a link. If the latter, then
> that's what we've been so far calling "a named bpfdumper". Which is
> what I argue is a link, pinned in bpfdumpfs (*not bpffs*).
It cannot be a link, since link is only a connection between
kernel object and bpf prog.
Whereas seq_file is such kernel object.
>
> For named dumper:
> 1. load bpfdump prog
> 2. attach prog to bpfdump "provider" (/sys/fs/bpfdump/task), get
> bpf_link anon FD back
> 3. pin link in bpfdumpfs (e.g., /sys/fs/bpfdump/task/my_dumper)
> 4. each open() of /sys/fs/bpfdump/task/my_dumper produces new
> bpfdumper session/seq_file
>
> For anon dumper:
> 1. load bpfdump prog
> 2. attach prog to bpfdump "provider" (/sys/fs/bpfdump/task), get
> bpf_link anon FD back
> 3. give bpf_link FD to some new API (say, BPF_DUMP_NEW_SESSION or
> whatever name) to create seq_file/bpfdumper session, which will create
> FD that can be read(). One can do that many times, each time getting
> its own bpfdumper session.
I slept on it and still fundamentally disagree that seq_file + bpf_prog
is a derivative of link. Or in OoO terms it's not a child class of bpf_link.
seq_file is its own class that should contain bpf_link as one of its
members, but it shouldn't be derived from 'class bpf_link'.
In that sense Yonghong proposed api (raw_tp_open to create anon seq_file+prog
and obj_pin to create a template of named seq_file+prog) are the best fit.
Implementation wise his 'struct extra_priv_data' needs to include
'struct bpf_link' instead of 'struct bpf_prog *prog;' directly.
So evertime 'cat' opens named seq_file there is bpf_link registered in IDR.
Anon seq_file should have another bpf_link as well.
My earlier suggestion to disallow get_fd_from_id for such links is wrong.
It's fine to get an FD to such link, but it shouldn't prevent destruction
of seq_file. 'cat' will close named seq_file and 'struct extra_priv_data' class
should do link_put. If some other process did get_fd_from_id then such link will
become dangling. Just like removal of netdev will make dangling xdp links.
next prev parent reply other threads:[~2020-04-16 17:04 UTC|newest]
Thread overview: 71+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-04-08 23:25 [RFC PATCH bpf-next 00/16] bpf: implement bpf based dumping of kernel data structures Yonghong Song
2020-04-08 23:25 ` [RFC PATCH bpf-next 01/16] net: refactor net assignment for seq_net_private structure Yonghong Song
2020-04-08 23:25 ` [RFC PATCH bpf-next 02/16] bpf: create /sys/kernel/bpfdump mount file system Yonghong Song
2020-04-08 23:25 ` [RFC PATCH bpf-next 03/16] bpf: provide a way for targets to register themselves Yonghong Song
2020-04-10 22:18 ` Andrii Nakryiko
2020-04-10 23:24 ` Yonghong Song
2020-04-13 19:31 ` Andrii Nakryiko
2020-04-15 22:57 ` Yonghong Song
2020-04-10 22:25 ` Andrii Nakryiko
2020-04-10 23:25 ` Yonghong Song
2020-04-08 23:25 ` [RFC PATCH bpf-next 04/16] bpf: allow loading of a dumper program Yonghong Song
2020-04-10 22:36 ` Andrii Nakryiko
2020-04-10 23:28 ` Yonghong Song
2020-04-13 19:33 ` Andrii Nakryiko
2020-04-08 23:25 ` [RFC PATCH bpf-next 05/16] bpf: create file or anonymous dumpers Yonghong Song
2020-04-10 3:00 ` Alexei Starovoitov
2020-04-10 6:09 ` Yonghong Song
2020-04-10 22:42 ` Yonghong Song
2020-04-10 22:53 ` Andrii Nakryiko
2020-04-10 23:47 ` Yonghong Song
2020-04-11 23:11 ` Alexei Starovoitov
2020-04-12 6:51 ` Yonghong Song
2020-04-13 20:48 ` Andrii Nakryiko
2020-04-10 22:51 ` Andrii Nakryiko
2020-04-10 23:41 ` Yonghong Song
2020-04-13 19:45 ` Andrii Nakryiko
2020-04-10 23:25 ` Andrii Nakryiko
2020-04-11 0:23 ` Yonghong Song
2020-04-11 23:17 ` Alexei Starovoitov
2020-04-13 21:04 ` Andrii Nakryiko
2020-04-13 19:59 ` Andrii Nakryiko
2020-04-14 5:56 ` Andrii Nakryiko
2020-04-14 23:59 ` Yonghong Song
2020-04-15 4:45 ` Andrii Nakryiko
2020-04-15 16:46 ` Alexei Starovoitov
2020-04-16 1:48 ` Andrii Nakryiko
2020-04-16 7:15 ` Yonghong Song
2020-04-16 17:04 ` Alexei Starovoitov [this message]
2020-04-16 19:35 ` Andrii Nakryiko
2020-04-16 23:18 ` Alexei Starovoitov
2020-04-17 5:11 ` Andrii Nakryiko
2020-04-19 6:11 ` Yonghong Song
2020-04-08 23:25 ` [RFC PATCH bpf-next 06/16] bpf: add netlink and ipv6_route targets Yonghong Song
2020-04-10 23:13 ` Andrii Nakryiko
2020-04-10 23:52 ` Yonghong Song
2020-04-08 23:25 ` [RFC PATCH bpf-next 07/16] bpf: add bpf_map target Yonghong Song
2020-04-13 22:18 ` Andrii Nakryiko
2020-04-13 22:47 ` Andrii Nakryiko
2020-04-08 23:25 ` [RFC PATCH bpf-next 08/16] bpf: add task and task/file targets Yonghong Song
2020-04-10 3:22 ` Alexei Starovoitov
2020-04-10 6:19 ` Yonghong Song
2020-04-10 21:31 ` Alexei Starovoitov
2020-04-10 21:33 ` Alexei Starovoitov
2020-04-13 23:00 ` Andrii Nakryiko
2020-04-08 23:25 ` [RFC PATCH bpf-next 09/16] bpf: add bpf_seq_printf and bpf_seq_write helpers Yonghong Song
2020-04-10 3:26 ` Alexei Starovoitov
2020-04-10 6:12 ` Yonghong Song
2020-04-14 5:28 ` Andrii Nakryiko
2020-04-08 23:25 ` [RFC PATCH bpf-next 10/16] bpf: support variable length array in tracing programs Yonghong Song
2020-04-14 0:13 ` Andrii Nakryiko
2020-04-08 23:25 ` [RFC PATCH bpf-next 11/16] bpf: implement query for target_proto and file dumper prog_id Yonghong Song
2020-04-10 3:10 ` Alexei Starovoitov
2020-04-10 6:11 ` Yonghong Song
2020-04-08 23:25 ` [RFC PATCH bpf-next 12/16] tools/libbpf: libbpf support for bpfdump Yonghong Song
2020-04-08 23:25 ` [RFC PATCH bpf-next 13/16] tools/bpftool: add bpf dumper support Yonghong Song
2020-04-08 23:25 ` [RFC PATCH bpf-next 14/16] tools/bpf: selftests: add dumper programs for ipv6_route and netlink Yonghong Song
2020-04-14 5:39 ` Andrii Nakryiko
2020-04-08 23:25 ` [RFC PATCH bpf-next 15/16] tools/bpf: selftests: add dumper progs for bpf_map/task/task_file Yonghong Song
2020-04-10 3:33 ` Alexei Starovoitov
2020-04-10 6:41 ` Yonghong Song
2020-04-08 23:25 ` [RFC PATCH bpf-next 16/16] tools/bpf: selftests: add a selftest for anonymous dumper Yonghong Song
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=20200416170414.ds3hcb3bgfetjt4v@ast-mbp \
--to=alexei.starovoitov@gmail.com \
--cc=andrii.nakryiko@gmail.com \
--cc=andriin@fb.com \
--cc=ast@fb.com \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=kafai@fb.com \
--cc=kernel-team@fb.com \
--cc=netdev@vger.kernel.org \
--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).