Message-ID: <CAEf4BzaqUsefJMehPdU1GmM2=S2NZfM9_=hG=RexFqah2n_WiA@mail.gmail.com>
Date:   Mon, 13 Apr 2020 13:48:49 -0700
From:   Andrii Nakryiko <andrii.nakryiko@...il.com>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Yonghong Song <yhs@...com>, Andrii Nakryiko <andriin@...com>,
        bpf <bpf@...r.kernel.org>, Martin KaFai Lau <kafai@...com>,
        Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [RFC PATCH bpf-next 05/16] bpf: create file or anonymous dumpers

On Sat, Apr 11, 2020 at 4:11 PM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> On Fri, Apr 10, 2020 at 04:47:36PM -0700, Yonghong Song wrote:
> > >
> > > Instead of special-casing dumper_name, can we require specifying full
> > > path, and then check whether it is in BPF FS vs BPFDUMP FS? If the
> > > latter, additionally check that it is in the right sub-directory
> > > matching its intended target type.
> >
> > We could. I just think specifying full path for bpfdump is not necessary
> > since it is a single user mount...
> >
> > >
> > > But honestly, just doing everything within BPF FS starts to seem
> > > cleaner at this point...
> >
> > bpffs is multi mount, which is not a perfect fit for bpfdump,
> > considering mounting inside namespace, etc, all dumpers are gone.
>
> As Yonghong pointed out reusing bpffs for dumpers doesn't look possible
> from implementation perspective.
> Even if it was possible the files in such mix-and-match file system
> would be of different kinds with different semantics. I think that
> will lead to mediocre user experience when file 'foo' is cat-able
> with nice human output, but file 'bar' isn't cat-able at all because
> it's just a pinned map. imo having all dumpers in one fixed location
> in /sys/kernel/bpfdump makes it easy to discover for folks who might
> not even know what bpf is.

I agree about importance of discoverability, but bpffs will typically
be mounted as /sys/fs/bpf/ as well, so it's just as discoverable at
/sys/fs/bpf/bpfdump. But I'm not too fixated on unifying bpffs and
bpfdumpfs, it's just that bpfdumpfs feels a bit too single-purpose.

> For example when I'm trying to learn some new area of the kernel I might go
> poke around /proc and /sys directory looking for a file name that could be
> interesting to 'cat'. This is how I discovered /sys/kernel/slab/ :)
> I think keeping all dumpers in /sys/kernel/bpfdump/ will make them
> similarly discoverable.
>
> re: f_dump flag...
> May be it's a sign that pinning is not the right name for such operation?
> If kernel cannot distinguish pinning dumper prog into bpffs as a vanilla
> pinning operation vs pinning into bpfdumpfs to make it cat-able then something
> isn't right about api. Either it needs to be a new bpf syscall command (like
> install_dumper_in_dumpfs) or reuse pinning command, but make libbpf specify the
> full path. From bpf prog point of view it may still specify only the final
> name, but libbpf can prepend the /sys/kernel/bpfdump/.../. May be there is a
> third option. Extra flag for pinning just doesn't look right. What if we do
> another specialized file system later? It would need yet another flag to pin
> there?

I agree about specifying full path from libbpf side. But section
definition shouldn't include /sys/fs/bpfdump part, so program would be
defined as:

SEC("dump/task/file")
int prog(...) { }

And libbpf by default will concat that with /sys/fs/bpfdump, but
probably should also provide a way to override prefix with custom
value, provided by users.

Powered by blists - more mailing lists