[bpf-next,09/10] libbpf: simplify look up by name of internal maps

Commit Message

Andrii Nakryiko Oct. 8, 2021, 12:03 a.m. UTC

From: Andrii Nakryiko <[e-mail]>

Map name that's assigned to internal maps (.rodata, .data, .bss, etc)
consist of a small prefix of bpf_object's name and ELF section name as
a suffix. This makes it hard for users to "guess" the name to use for
looking up by name with bpf_object__find_map_by_name() API.

One proposal was to drop object name prefix from the map name and just
use ".rodata", ".data", etc, names. One downside called out was that
when multiple BPF applications are active on the host, it will be hard
to distinguish between multiple instances of .rodata and know which BPF
object (app) they belong to. Having few first characters, while quite
limiting, still can give a bit of a clue, in general.

Another downside of such approach is that it is not backwards compatible
and, among direct use of bpf_object__find_map_by_name() API, will break
any BPF skeleton generated using bpftool that was compiled with older
libbpf version.

Instead of causing all this pain, libbpf will still generate map name
using a combination of object name and ELF section name, but it will
allow looking such maps up by their natural names, which correspond to
their respective ELF section names. This means non-truncated ELF section
names longer than 15 characters are going to be expected and supported.

With such set up, we get the best of both worlds: leave small bits of
a clue about BPF application that instantiated such maps, as well as
making it easy for user apps to lookup such maps at runtime. In this
sense it closes corresponding libbpf 1.0 issue ([0]).

BPF skeletons will continue using full names for lookups.

  [0] Closes: https://github.com/libbpf/libbpf/issues/275

Cc: Toke Høiland-Jørgensen <[e-mail]>
Cc: Stanislav Fomichev <[e-mail]>
Signed-off-by: Andrii Nakryiko <[e-mail]>
---
 tools/lib/bpf/libbpf.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Toke Høiland-Jørgensen Oct. 8, 2021, 5:30 p.m. UTC | #1

[e-mail] writes:

> From: Andrii Nakryiko <[e-mail]>
>
> Map name that's assigned to internal maps (.rodata, .data, .bss, etc)
> consist of a small prefix of bpf_object's name and ELF section name as
> a suffix. This makes it hard for users to "guess" the name to use for
> looking up by name with bpf_object__find_map_by_name() API.
>
> One proposal was to drop object name prefix from the map name and just
> use ".rodata", ".data", etc, names. One downside called out was that
> when multiple BPF applications are active on the host, it will be hard
> to distinguish between multiple instances of .rodata and know which BPF
> object (app) they belong to. Having few first characters, while quite
> limiting, still can give a bit of a clue, in general.
>
> Another downside of such approach is that it is not backwards compatible
> and, among direct use of bpf_object__find_map_by_name() API, will break
> any BPF skeleton generated using bpftool that was compiled with older
> libbpf version.
>
> Instead of causing all this pain, libbpf will still generate map name
> using a combination of object name and ELF section name, but it will
> allow looking such maps up by their natural names, which correspond to
> their respective ELF section names. This means non-truncated ELF section
> names longer than 15 characters are going to be expected and supported.
>
> With such set up, we get the best of both worlds: leave small bits of
> a clue about BPF application that instantiated such maps, as well as
> making it easy for user apps to lookup such maps at runtime. In this
> sense it closes corresponding libbpf 1.0 issue ([0]).

I like this approach. Only possible problem I can see is that it might
be confusing that a map can be looked up with one name, but that it
disappears once it's loaded into the kernel (and the BPF object is
closed).

Hmm, couldn't we just extend the kernel to accept longer names? Kinda
like with the netdev name aliases: support a secondary label that can be
longer, and have bpftool display both?

-Toke

Andrii Nakryiko Oct. 8, 2021, 6:21 p.m. UTC | #2

On Fri, Oct 8, 2021 at 10:31 AM Toke Høiland-Jørgensen <[e-mail]> wrote:
>
> [e-mail] writes:
>
> > From: Andrii Nakryiko <[e-mail]>
> >
> > Map name that's assigned to internal maps (.rodata, .data, .bss, etc)
> > consist of a small prefix of bpf_object's name and ELF section name as
> > a suffix. This makes it hard for users to "guess" the name to use for
> > looking up by name with bpf_object__find_map_by_name() API.
> >
> > One proposal was to drop object name prefix from the map name and just
> > use ".rodata", ".data", etc, names. One downside called out was that
> > when multiple BPF applications are active on the host, it will be hard
> > to distinguish between multiple instances of .rodata and know which BPF
> > object (app) they belong to. Having few first characters, while quite
> > limiting, still can give a bit of a clue, in general.
> >
> > Another downside of such approach is that it is not backwards compatible
> > and, among direct use of bpf_object__find_map_by_name() API, will break
> > any BPF skeleton generated using bpftool that was compiled with older
> > libbpf version.
> >
> > Instead of causing all this pain, libbpf will still generate map name
> > using a combination of object name and ELF section name, but it will
> > allow looking such maps up by their natural names, which correspond to
> > their respective ELF section names. This means non-truncated ELF section
> > names longer than 15 characters are going to be expected and supported.
> >
> > With such set up, we get the best of both worlds: leave small bits of
> > a clue about BPF application that instantiated such maps, as well as
> > making it easy for user apps to lookup such maps at runtime. In this
> > sense it closes corresponding libbpf 1.0 issue ([0]).
>
> I like this approach. Only possible problem I can see is that it might
> be confusing that a map can be looked up with one name, but that it
> disappears once it's loaded into the kernel (and the BPF object is
> closed).
>
> Hmm, couldn't we just extend the kernel to accept longer names? Kinda
> like with the netdev name aliases: support a secondary label that can be
> longer, and have bpftool display both?

Yes, this discrepancy can be confusing. I'd like all those internal
maps to be named after their corresponding ELF sections, tbh. We have
a mechanism now to make this transition (libbpf_set_strict_mode()),
but people have complained before that just seeing ".data" won't give
them enough information.

But if we are going to extend the kernel with longer map names, then
I'd rather stick to clean ".data.custom" naming from the very
beginning, and then switch all existing .data/.rodata/.bss/.kconfig
map naming to the same convention as well (guarded by opt-in flag in
libbpf_set_strict_mode() until libbpf 1.0). In the kernel, though,
instead of having two names (i.e., one is alias), I'd just allow to
provide one long name and then all existing UAPIs that have char[16]
everywhere would just be a potentially truncated prefix of such a
longer name. All the tooling can be updated to use long name when
available, of course. WDYT?

>
> -Toke
>

Toke Høiland-Jørgensen Oct. 8, 2021, 9:44 p.m. UTC | #3

Andrii Nakryiko <[e-mail]> writes:

> On Fri, Oct 8, 2021 at 10:31 AM Toke Høiland-Jørgensen <[e-mail]> wrote:
>>
>> [e-mail] writes:
>>
>> > From: Andrii Nakryiko <[e-mail]>
>> >
>> > Map name that's assigned to internal maps (.rodata, .data, .bss, etc)
>> > consist of a small prefix of bpf_object's name and ELF section name as
>> > a suffix. This makes it hard for users to "guess" the name to use for
>> > looking up by name with bpf_object__find_map_by_name() API.
>> >
>> > One proposal was to drop object name prefix from the map name and just
>> > use ".rodata", ".data", etc, names. One downside called out was that
>> > when multiple BPF applications are active on the host, it will be hard
>> > to distinguish between multiple instances of .rodata and know which BPF
>> > object (app) they belong to. Having few first characters, while quite
>> > limiting, still can give a bit of a clue, in general.
>> >
>> > Another downside of such approach is that it is not backwards compatible
>> > and, among direct use of bpf_object__find_map_by_name() API, will break
>> > any BPF skeleton generated using bpftool that was compiled with older
>> > libbpf version.
>> >
>> > Instead of causing all this pain, libbpf will still generate map name
>> > using a combination of object name and ELF section name, but it will
>> > allow looking such maps up by their natural names, which correspond to
>> > their respective ELF section names. This means non-truncated ELF section
>> > names longer than 15 characters are going to be expected and supported.
>> >
>> > With such set up, we get the best of both worlds: leave small bits of
>> > a clue about BPF application that instantiated such maps, as well as
>> > making it easy for user apps to lookup such maps at runtime. In this
>> > sense it closes corresponding libbpf 1.0 issue ([0]).
>>
>> I like this approach. Only possible problem I can see is that it might
>> be confusing that a map can be looked up with one name, but that it
>> disappears once it's loaded into the kernel (and the BPF object is
>> closed).
>>
>> Hmm, couldn't we just extend the kernel to accept longer names? Kinda
>> like with the netdev name aliases: support a secondary label that can be
>> longer, and have bpftool display both?
>
> Yes, this discrepancy can be confusing. I'd like all those internal
> maps to be named after their corresponding ELF sections, tbh. We have
> a mechanism now to make this transition (libbpf_set_strict_mode()),
> but people have complained before that just seeing ".data" won't give
> them enough information.

Yeah, I do also sympathise with that complaint :)

> But if we are going to extend the kernel with longer map names, then
> I'd rather stick to clean ".data.custom" naming from the very
> beginning, and then switch all existing .data/.rodata/.bss/.kconfig
> map naming to the same convention as well (guarded by opt-in flag in
> libbpf_set_strict_mode() until libbpf 1.0). In the kernel, though,
> instead of having two names (i.e., one is alias), I'd just allow to
> provide one long name and then all existing UAPIs that have char[16]
> everywhere would just be a potentially truncated prefix of such a
> longer name. All the tooling can be updated to use long name when
> available, of course. WDYT?

Hmm, so introduce a new 'map_name_long' field, and on query the kernel
will fill in the old map_name with a truncated version, and put the full
name in the new field? Yeah, I guess that would work too!

-Toke

Song Liu Oct. 8, 2021, 10:16 p.m. UTC | #4

On Thu, Oct 7, 2021 at 5:05 PM <[e-mail]> wrote:
>
[...]
>
> With such set up, we get the best of both worlds: leave small bits of
> a clue about BPF application that instantiated such maps, as well as
> making it easy for user apps to lookup such maps at runtime. In this
> sense it closes corresponding libbpf 1.0 issue ([0]).
>
> BPF skeletons will continue using full names for lookups.
>
>   [0] Closes: https://github.com/libbpf/libbpf/issues/275
>
> Cc: Toke Høiland-Jørgensen <[e-mail]>
> Cc: Stanislav Fomichev <[e-mail]>
> Signed-off-by: Andrii Nakryiko <[e-mail]>

Acked-by: Song Liu <[e-mail]>

Alexei Starovoitov Oct. 11, 2021, 9:24 p.m. UTC | #5

On 10/8/21 2:44 PM, Toke Høiland-Jørgensen wrote:
> 
> Hmm, so introduce a new 'map_name_long' field, and on query the kernel
> will fill in the old map_name with a truncated version, and put the full
> name in the new field? Yeah, I guess that would work too!

Let's start storing full map names in BTF instead.
Like we do already for progs.
Some tools already fetch full prog names this way.

Andrii Nakryiko Oct. 12, 2021, 3:45 a.m. UTC | #6

On Mon, Oct 11, 2021 at 11:24 PM Alexei Starovoitov <[e-mail]> wrote:
>
> On 10/8/21 2:44 PM, Toke Høiland-Jørgensen wrote:
> >
> > Hmm, so introduce a new 'map_name_long' field, and on query the kernel
> > will fill in the old map_name with a truncated version, and put the full
> > name in the new field? Yeah, I guess that would work too!
>
> Let's start storing full map names in BTF instead.
> Like we do already for progs.
> Some tools already fetch full prog names this way.

We do have those names in BTF. Each map has either corresponding VAR
or DATASEC. The problem is that we don't know which.

Are you proposing to add some extra "btf_def_type_id" field to specify
BTF type ID of what "defines" the map (VAR or DATASEC)? That would
work. Would still require UAPI and kernel changes, of course.

The reason Toke and others were asking to preserve that object name
prefix for .rodata/.data maps was different though, and won't be
addressed by the above. Even if you know the BTF VAR/DATASEC, you
don't know the "object name" associated with the map. And the kernel
doesn't know because it's purely libbpf's abstraction. And sometimes
that abstraction doesn't make sense (e.g., if we create a map that's
pinned and reused from multiple BPF apps/objects).

We do have BPF metadata that Stanislav added a while ago, so maybe we
should just punt that problem to that? I'd love to have clean
".rodata" and ".data" names, of course.

Stanislav Fomichev Oct. 12, 2021, 3:29 p.m. UTC | #7

On Mon, Oct 11, 2021 at 8:45 PM Andrii Nakryiko
<[e-mail]> wrote:
>
> On Mon, Oct 11, 2021 at 11:24 PM Alexei Starovoitov <[e-mail]> wrote:
> >
> > On 10/8/21 2:44 PM, Toke Høiland-Jørgensen wrote:
> > >
> > > Hmm, so introduce a new 'map_name_long' field, and on query the kernel
> > > will fill in the old map_name with a truncated version, and put the full
> > > name in the new field? Yeah, I guess that would work too!
> >
> > Let's start storing full map names in BTF instead.
> > Like we do already for progs.
> > Some tools already fetch full prog names this way.
>
> We do have those names in BTF. Each map has either corresponding VAR
> or DATASEC. The problem is that we don't know which.
>
> Are you proposing to add some extra "btf_def_type_id" field to specify
> BTF type ID of what "defines" the map (VAR or DATASEC)? That would
> work. Would still require UAPI and kernel changes, of course.
>
> The reason Toke and others were asking to preserve that object name
> prefix for .rodata/.data maps was different though, and won't be
> addressed by the above. Even if you know the BTF VAR/DATASEC, you
> don't know the "object name" associated with the map. And the kernel
> doesn't know because it's purely libbpf's abstraction. And sometimes
> that abstraction doesn't make sense (e.g., if we create a map that's
> pinned and reused from multiple BPF apps/objects).

[..]

> We do have BPF metadata that Stanislav added a while ago, so maybe we
> should just punt that problem to that? I'd love to have clean
> ".rodata" and ".data" names, of course.

Are you suggesting we add some option to associate the metadata with
the maps (might be an option)? IIRC, the metadata can only be
associated with the progs right now.

Andrii Nakryiko Oct. 20, 2021, 5:59 p.m. UTC | #8

On Tue, Oct 12, 2021 at 8:29 AM Stanislav Fomichev <[e-mail]> wrote:
>
> On Mon, Oct 11, 2021 at 8:45 PM Andrii Nakryiko
> <[e-mail]> wrote:
> >
> > On Mon, Oct 11, 2021 at 11:24 PM Alexei Starovoitov <[e-mail]> wrote:
> > >
> > > On 10/8/21 2:44 PM, Toke Høiland-Jørgensen wrote:
> > > >
> > > > Hmm, so introduce a new 'map_name_long' field, and on query the kernel
> > > > will fill in the old map_name with a truncated version, and put the full
> > > > name in the new field? Yeah, I guess that would work too!
> > >
> > > Let's start storing full map names in BTF instead.
> > > Like we do already for progs.
> > > Some tools already fetch full prog names this way.
> >
> > We do have those names in BTF. Each map has either corresponding VAR
> > or DATASEC. The problem is that we don't know which.
> >
> > Are you proposing to add some extra "btf_def_type_id" field to specify
> > BTF type ID of what "defines" the map (VAR or DATASEC)? That would
> > work. Would still require UAPI and kernel changes, of course.
> >
> > The reason Toke and others were asking to preserve that object name
> > prefix for .rodata/.data maps was different though, and won't be
> > addressed by the above. Even if you know the BTF VAR/DATASEC, you
> > don't know the "object name" associated with the map. And the kernel
> > doesn't know because it's purely libbpf's abstraction. And sometimes
> > that abstraction doesn't make sense (e.g., if we create a map that's
> > pinned and reused from multiple BPF apps/objects).
>
> [..]
>
> > We do have BPF metadata that Stanislav added a while ago, so maybe we
> > should just punt that problem to that? I'd love to have clean
> > ".rodata" and ".data" names, of course.
>
> Are you suggesting we add some option to associate the metadata with
> the maps (might be an option)? IIRC, the metadata can only be
> associated with the progs right now.

Well, maps have associated BTF fd, when they are created, no? So you
can find all the same metadata for the map, no?

Stanislav Fomichev Oct. 20, 2021, 6:09 p.m. UTC | #9

On Wed, Oct 20, 2021 at 10:59 AM Andrii Nakryiko
<[e-mail]> wrote:
>
> On Tue, Oct 12, 2021 at 8:29 AM Stanislav Fomichev <[e-mail]> wrote:
> >
> > On Mon, Oct 11, 2021 at 8:45 PM Andrii Nakryiko
> > <[e-mail]> wrote:
> > >
> > > On Mon, Oct 11, 2021 at 11:24 PM Alexei Starovoitov <[e-mail]> wrote:
> > > >
> > > > On 10/8/21 2:44 PM, Toke Høiland-Jørgensen wrote:
> > > > >
> > > > > Hmm, so introduce a new 'map_name_long' field, and on query the kernel
> > > > > will fill in the old map_name with a truncated version, and put the full
> > > > > name in the new field? Yeah, I guess that would work too!
> > > >
> > > > Let's start storing full map names in BTF instead.
> > > > Like we do already for progs.
> > > > Some tools already fetch full prog names this way.
> > >
> > > We do have those names in BTF. Each map has either corresponding VAR
> > > or DATASEC. The problem is that we don't know which.
> > >
> > > Are you proposing to add some extra "btf_def_type_id" field to specify
> > > BTF type ID of what "defines" the map (VAR or DATASEC)? That would
> > > work. Would still require UAPI and kernel changes, of course.
> > >
> > > The reason Toke and others were asking to preserve that object name
> > > prefix for .rodata/.data maps was different though, and won't be
> > > addressed by the above. Even if you know the BTF VAR/DATASEC, you
> > > don't know the "object name" associated with the map. And the kernel
> > > doesn't know because it's purely libbpf's abstraction. And sometimes
> > > that abstraction doesn't make sense (e.g., if we create a map that's
> > > pinned and reused from multiple BPF apps/objects).
> >
> > [..]
> >
> > > We do have BPF metadata that Stanislav added a while ago, so maybe we
> > > should just punt that problem to that? I'd love to have clean
> > > ".rodata" and ".data" names, of course.
> >
> > Are you suggesting we add some option to associate the metadata with
> > the maps (might be an option)? IIRC, the metadata can only be
> > associated with the progs right now.
>
> Well, maps have associated BTF fd, when they are created, no? So you
> can find all the same metadata for the map, no?

I guess that's true, we can store this metadata in the map itself
using something like existing bpf_metadata_ prefix.

Andrii Nakryiko Oct. 20, 2021, 6:20 p.m. UTC | #10

On Wed, Oct 20, 2021 at 11:09 AM Stanislav Fomichev <[e-mail]> wrote:
>
> On Wed, Oct 20, 2021 at 10:59 AM Andrii Nakryiko
> <[e-mail]> wrote:
> >
> > On Tue, Oct 12, 2021 at 8:29 AM Stanislav Fomichev <[e-mail]> wrote:
> > >
> > > On Mon, Oct 11, 2021 at 8:45 PM Andrii Nakryiko
> > > <[e-mail]> wrote:
> > > >
> > > > On Mon, Oct 11, 2021 at 11:24 PM Alexei Starovoitov <[e-mail]> wrote:
> > > > >
> > > > > On 10/8/21 2:44 PM, Toke Høiland-Jørgensen wrote:
> > > > > >
> > > > > > Hmm, so introduce a new 'map_name_long' field, and on query the kernel
> > > > > > will fill in the old map_name with a truncated version, and put the full
> > > > > > name in the new field? Yeah, I guess that would work too!
> > > > >
> > > > > Let's start storing full map names in BTF instead.
> > > > > Like we do already for progs.
> > > > > Some tools already fetch full prog names this way.
> > > >
> > > > We do have those names in BTF. Each map has either corresponding VAR
> > > > or DATASEC. The problem is that we don't know which.
> > > >
> > > > Are you proposing to add some extra "btf_def_type_id" field to specify
> > > > BTF type ID of what "defines" the map (VAR or DATASEC)? That would
> > > > work. Would still require UAPI and kernel changes, of course.
> > > >
> > > > The reason Toke and others were asking to preserve that object name
> > > > prefix for .rodata/.data maps was different though, and won't be
> > > > addressed by the above. Even if you know the BTF VAR/DATASEC, you
> > > > don't know the "object name" associated with the map. And the kernel
> > > > doesn't know because it's purely libbpf's abstraction. And sometimes
> > > > that abstraction doesn't make sense (e.g., if we create a map that's
> > > > pinned and reused from multiple BPF apps/objects).
> > >
> > > [..]
> > >
> > > > We do have BPF metadata that Stanislav added a while ago, so maybe we
> > > > should just punt that problem to that? I'd love to have clean
> > > > ".rodata" and ".data" names, of course.
> > >
> > > Are you suggesting we add some option to associate the metadata with
> > > the maps (might be an option)? IIRC, the metadata can only be
> > > associated with the progs right now.
> >
> > Well, maps have associated BTF fd, when they are created, no? So you
> > can find all the same metadata for the map, no?
>
> I guess that's true, we can store this metadata in the map itself
> using something like existing bpf_metadata_ prefix.

We had a discussion during the inaugural BSC meeting about having a
small set of "standardized" metadata strings. "owner" and
"description" (or maybe "app" for "application name") were two that
were clearly useful and generally useful. So if we update bpftool and
other tooling to recognize bpf_metadata_owner and bpf_metadata_app and
print them in some nice and meaningful way in bpftool output (in
addition to general btf_metadata dump), it would be great.

Which is a long way to say that once we have bpf_metadat_app, you
already have associated bpf_object name (or whatever user will decide
to name their BPF application). Which solves this map naming problem
as well (with tooling support, of course).

cc Quentin also. Thoughts?

Toke Høiland-Jørgensen Oct. 20, 2021, 10:03 p.m. UTC | #11

Andrii Nakryiko <[e-mail]> writes:

> On Wed, Oct 20, 2021 at 11:09 AM Stanislav Fomichev <[e-mail]> wrote:
>>
>> On Wed, Oct 20, 2021 at 10:59 AM Andrii Nakryiko
>> <[e-mail]> wrote:
>> >
>> > On Tue, Oct 12, 2021 at 8:29 AM Stanislav Fomichev <[e-mail]> wrote:
>> > >
>> > > On Mon, Oct 11, 2021 at 8:45 PM Andrii Nakryiko
>> > > <[e-mail]> wrote:
>> > > >
>> > > > On Mon, Oct 11, 2021 at 11:24 PM Alexei Starovoitov <[e-mail]> wrote:
>> > > > >
>> > > > > On 10/8/21 2:44 PM, Toke Høiland-Jørgensen wrote:
>> > > > > >
>> > > > > > Hmm, so introduce a new 'map_name_long' field, and on query the kernel
>> > > > > > will fill in the old map_name with a truncated version, and put the full
>> > > > > > name in the new field? Yeah, I guess that would work too!
>> > > > >
>> > > > > Let's start storing full map names in BTF instead.
>> > > > > Like we do already for progs.
>> > > > > Some tools already fetch full prog names this way.
>> > > >
>> > > > We do have those names in BTF. Each map has either corresponding VAR
>> > > > or DATASEC. The problem is that we don't know which.
>> > > >
>> > > > Are you proposing to add some extra "btf_def_type_id" field to specify
>> > > > BTF type ID of what "defines" the map (VAR or DATASEC)? That would
>> > > > work. Would still require UAPI and kernel changes, of course.
>> > > >
>> > > > The reason Toke and others were asking to preserve that object name
>> > > > prefix for .rodata/.data maps was different though, and won't be
>> > > > addressed by the above. Even if you know the BTF VAR/DATASEC, you
>> > > > don't know the "object name" associated with the map. And the kernel
>> > > > doesn't know because it's purely libbpf's abstraction. And sometimes
>> > > > that abstraction doesn't make sense (e.g., if we create a map that's
>> > > > pinned and reused from multiple BPF apps/objects).
>> > >
>> > > [..]
>> > >
>> > > > We do have BPF metadata that Stanislav added a while ago, so maybe we
>> > > > should just punt that problem to that? I'd love to have clean
>> > > > ".rodata" and ".data" names, of course.
>> > >
>> > > Are you suggesting we add some option to associate the metadata with
>> > > the maps (might be an option)? IIRC, the metadata can only be
>> > > associated with the progs right now.
>> >
>> > Well, maps have associated BTF fd, when they are created, no? So you
>> > can find all the same metadata for the map, no?
>>
>> I guess that's true, we can store this metadata in the map itself
>> using something like existing bpf_metadata_ prefix.
>
> We had a discussion during the inaugural BSC meeting about having a
> small set of "standardized" metadata strings. "owner" and
> "description" (or maybe "app" for "application name") were two that
> were clearly useful and generally useful. So if we update bpftool and
> other tooling to recognize bpf_metadata_owner and bpf_metadata_app and
> print them in some nice and meaningful way in bpftool output (in
> addition to general btf_metadata dump), it would be great.

I like the idea of specifying some well-known metadata names, especially
if libbpf can auto-populate them if the user doesn't.

Also, couldn't bpftool just print out all bpf_metadata_* fields? At
least in a verbose mode...

-Toke

Stanislav Fomichev Oct. 20, 2021, 10:24 p.m. UTC | #12

On Wed, Oct 20, 2021 at 3:03 PM Toke Høiland-Jørgensen <[e-mail]> wrote:
>
> Andrii Nakryiko <[e-mail]> writes:
>
> > On Wed, Oct 20, 2021 at 11:09 AM Stanislav Fomichev <[e-mail]> wrote:
> >>
> >> On Wed, Oct 20, 2021 at 10:59 AM Andrii Nakryiko
> >> <[e-mail]> wrote:
> >> >
> >> > On Tue, Oct 12, 2021 at 8:29 AM Stanislav Fomichev <[e-mail]> wrote:
> >> > >
> >> > > On Mon, Oct 11, 2021 at 8:45 PM Andrii Nakryiko
> >> > > <[e-mail]> wrote:
> >> > > >
> >> > > > On Mon, Oct 11, 2021 at 11:24 PM Alexei Starovoitov <[e-mail]> wrote:
> >> > > > >
> >> > > > > On 10/8/21 2:44 PM, Toke Høiland-Jørgensen wrote:
> >> > > > > >
> >> > > > > > Hmm, so introduce a new 'map_name_long' field, and on query the kernel
> >> > > > > > will fill in the old map_name with a truncated version, and put the full
> >> > > > > > name in the new field? Yeah, I guess that would work too!
> >> > > > >
> >> > > > > Let's start storing full map names in BTF instead.
> >> > > > > Like we do already for progs.
> >> > > > > Some tools already fetch full prog names this way.
> >> > > >
> >> > > > We do have those names in BTF. Each map has either corresponding VAR
> >> > > > or DATASEC. The problem is that we don't know which.
> >> > > >
> >> > > > Are you proposing to add some extra "btf_def_type_id" field to specify
> >> > > > BTF type ID of what "defines" the map (VAR or DATASEC)? That would
> >> > > > work. Would still require UAPI and kernel changes, of course.
> >> > > >
> >> > > > The reason Toke and others were asking to preserve that object name
> >> > > > prefix for .rodata/.data maps was different though, and won't be
> >> > > > addressed by the above. Even if you know the BTF VAR/DATASEC, you
> >> > > > don't know the "object name" associated with the map. And the kernel
> >> > > > doesn't know because it's purely libbpf's abstraction. And sometimes
> >> > > > that abstraction doesn't make sense (e.g., if we create a map that's
> >> > > > pinned and reused from multiple BPF apps/objects).
> >> > >
> >> > > [..]
> >> > >
> >> > > > We do have BPF metadata that Stanislav added a while ago, so maybe we
> >> > > > should just punt that problem to that? I'd love to have clean
> >> > > > ".rodata" and ".data" names, of course.
> >> > >
> >> > > Are you suggesting we add some option to associate the metadata with
> >> > > the maps (might be an option)? IIRC, the metadata can only be
> >> > > associated with the progs right now.
> >> >
> >> > Well, maps have associated BTF fd, when they are created, no? So you
> >> > can find all the same metadata for the map, no?
> >>
> >> I guess that's true, we can store this metadata in the map itself
> >> using something like existing bpf_metadata_ prefix.
> >
> > We had a discussion during the inaugural BSC meeting about having a
> > small set of "standardized" metadata strings. "owner" and
> > "description" (or maybe "app" for "application name") were two that
> > were clearly useful and generally useful. So if we update bpftool and
> > other tooling to recognize bpf_metadata_owner and bpf_metadata_app and
> > print them in some nice and meaningful way in bpftool output (in
> > addition to general btf_metadata dump), it would be great.
>
> I like the idea of specifying some well-known metadata names, especially
> if libbpf can auto-populate them if the user doesn't.

Yeah, I'd +1 that. I was exploring the idea of adding process's
cmdline into map/prog info a while ago. That's where this whole
metadata came out, but I've yet to add something to libbpf that's
"standardized".

> Also, couldn't bpftool just print out all bpf_metadata_* fields? At
> least in a verbose mode...

It already prints everything, but it prints them in a plain list.
Maybe we can integrate some of the data more nicely.

Andrii Nakryiko Oct. 20, 2021, 10:25 p.m. UTC | #13

On Wed, Oct 20, 2021 at 3:03 PM Toke Høiland-Jørgensen <[e-mail]> wrote:
>
> Andrii Nakryiko <[e-mail]> writes:
>
> > On Wed, Oct 20, 2021 at 11:09 AM Stanislav Fomichev <[e-mail]> wrote:
> >>
> >> On Wed, Oct 20, 2021 at 10:59 AM Andrii Nakryiko
> >> <[e-mail]> wrote:
> >> >
> >> > On Tue, Oct 12, 2021 at 8:29 AM Stanislav Fomichev <[e-mail]> wrote:
> >> > >
> >> > > On Mon, Oct 11, 2021 at 8:45 PM Andrii Nakryiko
> >> > > <[e-mail]> wrote:
> >> > > >
> >> > > > On Mon, Oct 11, 2021 at 11:24 PM Alexei Starovoitov <[e-mail]> wrote:
> >> > > > >
> >> > > > > On 10/8/21 2:44 PM, Toke Høiland-Jørgensen wrote:
> >> > > > > >
> >> > > > > > Hmm, so introduce a new 'map_name_long' field, and on query the kernel
> >> > > > > > will fill in the old map_name with a truncated version, and put the full
> >> > > > > > name in the new field? Yeah, I guess that would work too!
> >> > > > >
> >> > > > > Let's start storing full map names in BTF instead.
> >> > > > > Like we do already for progs.
> >> > > > > Some tools already fetch full prog names this way.
> >> > > >
> >> > > > We do have those names in BTF. Each map has either corresponding VAR
> >> > > > or DATASEC. The problem is that we don't know which.
> >> > > >
> >> > > > Are you proposing to add some extra "btf_def_type_id" field to specify
> >> > > > BTF type ID of what "defines" the map (VAR or DATASEC)? That would
> >> > > > work. Would still require UAPI and kernel changes, of course.
> >> > > >
> >> > > > The reason Toke and others were asking to preserve that object name
> >> > > > prefix for .rodata/.data maps was different though, and won't be
> >> > > > addressed by the above. Even if you know the BTF VAR/DATASEC, you
> >> > > > don't know the "object name" associated with the map. And the kernel
> >> > > > doesn't know because it's purely libbpf's abstraction. And sometimes
> >> > > > that abstraction doesn't make sense (e.g., if we create a map that's
> >> > > > pinned and reused from multiple BPF apps/objects).
> >> > >
> >> > > [..]
> >> > >
> >> > > > We do have BPF metadata that Stanislav added a while ago, so maybe we
> >> > > > should just punt that problem to that? I'd love to have clean
> >> > > > ".rodata" and ".data" names, of course.
> >> > >
> >> > > Are you suggesting we add some option to associate the metadata with
> >> > > the maps (might be an option)? IIRC, the metadata can only be
> >> > > associated with the progs right now.
> >> >
> >> > Well, maps have associated BTF fd, when they are created, no? So you
> >> > can find all the same metadata for the map, no?
> >>
> >> I guess that's true, we can store this metadata in the map itself
> >> using something like existing bpf_metadata_ prefix.
> >
> > We had a discussion during the inaugural BSC meeting about having a
> > small set of "standardized" metadata strings. "owner" and
> > "description" (or maybe "app" for "application name") were two that
> > were clearly useful and generally useful. So if we update bpftool and
> > other tooling to recognize bpf_metadata_owner and bpf_metadata_app and
> > print them in some nice and meaningful way in bpftool output (in
> > addition to general btf_metadata dump), it would be great.
>
> I like the idea of specifying some well-known metadata names, especially
> if libbpf can auto-populate them if the user doesn't.
>
> Also, couldn't bpftool just print out all bpf_metadata_* fields? At
> least in a verbose mode...

Yes, bpftool dumps all bpf_metadata_* fields already. The point of
converging on few common ones (say, bpf_metadata_owner and
bpf_metadata_app) would allow all the tools to use consistent subset
to display meaningful short info about a prog or map. Dumping all
metadata fields for something like "bpf top" doesn't make sense.

re: libbpf auto-populating some of them. It can populate "app"
metadata from bpf_object's name, but we need to think through all the
logistics carefully (e.g., not setting if user already specified that
explicitly, etc). There is no way libbpf can know "owner" meta,
though.

>
> -Toke
>

Toke Høiland-Jørgensen Oct. 21, 2021, 11:39 a.m. UTC | #14

Andrii Nakryiko <[e-mail]> writes:

> On Wed, Oct 20, 2021 at 3:03 PM Toke Høiland-Jørgensen <[e-mail]> wrote:
>>
>> Andrii Nakryiko <[e-mail]> writes:
>>
>> > On Wed, Oct 20, 2021 at 11:09 AM Stanislav Fomichev <[e-mail]> wrote:
>> >>
>> >> On Wed, Oct 20, 2021 at 10:59 AM Andrii Nakryiko
>> >> <[e-mail]> wrote:
>> >> >
>> >> > On Tue, Oct 12, 2021 at 8:29 AM Stanislav Fomichev <[e-mail]> wrote:
>> >> > >
>> >> > > On Mon, Oct 11, 2021 at 8:45 PM Andrii Nakryiko
>> >> > > <[e-mail]> wrote:
>> >> > > >
>> >> > > > On Mon, Oct 11, 2021 at 11:24 PM Alexei Starovoitov <[e-mail]> wrote:
>> >> > > > >
>> >> > > > > On 10/8/21 2:44 PM, Toke Høiland-Jørgensen wrote:
>> >> > > > > >
>> >> > > > > > Hmm, so introduce a new 'map_name_long' field, and on query the kernel
>> >> > > > > > will fill in the old map_name with a truncated version, and put the full
>> >> > > > > > name in the new field? Yeah, I guess that would work too!
>> >> > > > >
>> >> > > > > Let's start storing full map names in BTF instead.
>> >> > > > > Like we do already for progs.
>> >> > > > > Some tools already fetch full prog names this way.
>> >> > > >
>> >> > > > We do have those names in BTF. Each map has either corresponding VAR
>> >> > > > or DATASEC. The problem is that we don't know which.
>> >> > > >
>> >> > > > Are you proposing to add some extra "btf_def_type_id" field to specify
>> >> > > > BTF type ID of what "defines" the map (VAR or DATASEC)? That would
>> >> > > > work. Would still require UAPI and kernel changes, of course.
>> >> > > >
>> >> > > > The reason Toke and others were asking to preserve that object name
>> >> > > > prefix for .rodata/.data maps was different though, and won't be
>> >> > > > addressed by the above. Even if you know the BTF VAR/DATASEC, you
>> >> > > > don't know the "object name" associated with the map. And the kernel
>> >> > > > doesn't know because it's purely libbpf's abstraction. And sometimes
>> >> > > > that abstraction doesn't make sense (e.g., if we create a map that's
>> >> > > > pinned and reused from multiple BPF apps/objects).
>> >> > >
>> >> > > [..]
>> >> > >
>> >> > > > We do have BPF metadata that Stanislav added a while ago, so maybe we
>> >> > > > should just punt that problem to that? I'd love to have clean
>> >> > > > ".rodata" and ".data" names, of course.
>> >> > >
>> >> > > Are you suggesting we add some option to associate the metadata with
>> >> > > the maps (might be an option)? IIRC, the metadata can only be
>> >> > > associated with the progs right now.
>> >> >
>> >> > Well, maps have associated BTF fd, when they are created, no? So you
>> >> > can find all the same metadata for the map, no?
>> >>
>> >> I guess that's true, we can store this metadata in the map itself
>> >> using something like existing bpf_metadata_ prefix.
>> >
>> > We had a discussion during the inaugural BSC meeting about having a
>> > small set of "standardized" metadata strings. "owner" and
>> > "description" (or maybe "app" for "application name") were two that
>> > were clearly useful and generally useful. So if we update bpftool and
>> > other tooling to recognize bpf_metadata_owner and bpf_metadata_app and
>> > print them in some nice and meaningful way in bpftool output (in
>> > addition to general btf_metadata dump), it would be great.
>>
>> I like the idea of specifying some well-known metadata names, especially
>> if libbpf can auto-populate them if the user doesn't.
>>
>> Also, couldn't bpftool just print out all bpf_metadata_* fields? At
>> least in a verbose mode...
>
> Yes, bpftool dumps all bpf_metadata_* fields already. The point of
> converging on few common ones (say, bpf_metadata_owner and
> bpf_metadata_app) would allow all the tools to use consistent subset
> to display meaningful short info about a prog or map. Dumping all
> metadata fields for something like "bpf top" doesn't make sense.
>
> re: libbpf auto-populating some of them. It can populate "app"
> metadata from bpf_object's name, but we need to think through all the
> logistics carefully (e.g., not setting if user already specified that
> explicitly, etc). There is no way libbpf can know "owner" meta,
> though.

Right, I was mostly thinking about the "app" name; just so there's a
default if users don't set anything themselves, like there is today with
the prefix...

-Toke

Patch

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 054549846025..01ebdb8a36d1 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -9037,6 +9037,16 @@  bpf_object__find_map_by_name(const struct bpf_object *obj, const char *name)
 	struct bpf_map *pos;
 
 	bpf_object__for_each_map(pos, obj) {
+		/* if it's a special internal map name (which always starts
+		 * with dot) then check if that special name matches the
+		 * real map name (ELF section name)
+		 */
+		if (name[0] == '.') {
+			if (pos->real_name && strcmp(pos->real_name, name) == 0)
+				return pos;
+			continue;
+		}
+		/* otherwise map name has to be an exact match */
 		if (map_uses_real_name(pos)) {
 			if (strcmp(pos->real_name, name) == 0)
 				return pos;