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