On Mon, Feb 8, 2021 at 9:16 PM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote:
>
> Gilad Reti <gilad.reti@xxxxxxxxx> writes:
>
> > On Mon, Feb 8, 2021 at 7:55 PM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote:
> >>
> >> Gilad Reti <gilad.reti@xxxxxxxxx> writes:
> >>
> >> > On Mon, Feb 8, 2021 at 5:09 PM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote:
> >> >>
> >> >> Gilad Reti <gilad.reti@xxxxxxxxx> writes:
> >> >>
> >> >> > On Mon, Feb 8, 2021 at 4:28 PM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote:
> >> >> >>
> >> >> >> Gilad Reti <gilad.reti@xxxxxxxxx> writes:
> >> >> >>
> >> >> >> > On Mon, Feb 8, 2021 at 1:42 PM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote:
> >> >> >> >>
> >> >> >> >> Gilad Reti <gilad.reti@xxxxxxxxx> writes:
> >> >> >> >>
> >> >> >> >> > Also, is there a way to set the pin path to all maps/programs at once?
> >> >> >> >> > For example, bpf_object__pin_maps pins all maps at a specific path,
> >> >> >> >> > but as far as I was able to find there is no similar function to set
> >> >> >> >> > the pin path for all maps only (without pinning) so that at loading
> >> >> >> >> > time libbpf will try to reuse all maps. The only way to achieve a
> >> >> >> >> > complete reuse of all maps that I could find is to "reverse engineer"
> >> >> >> >> > libbpf's pin path generation algorithm (i.e. <path>/<map_name>) and
> >> >> >> >> > set the pin path on each map before load.
> >> >> >> >>
> >> >> >> >> You can set the 'pinning' attribute in the map definition - add
> >> >> >> >> '__uint(pinning, LIBBPF_PIN_BY_NAME);' to the map struct. By default
> >> >> >> >> this will pin beneath /sys/fs/bpf, but you can customise that by setting
> >> >> >> >> the pin_root_path attribute in bpf_object_open_opts.
> >> >> >> >
> >> >> >> > Yes, I am familiar with that feature, but it has some downsides:
> >> >> >> > 1. I need to set it manually on every map (and in cases that I have
> >> >> >> > only the compiled object file that would be hard).
> >> >> >> > 2. It only works for bpf maps and not bpf programs.
> >> >> >> > 3. It only works for bpf maps that are defined explicitly in the bpf
> >> >> >> > code and not for implicit (inner) bpf maps (bss, rodata, etc).
> >> >> >>
> >> >> >> Ah, right. Well, other than that I don't think there's a way to set pin
> >> >> >> paths in bulk, other than by manually iterating and setting them one at
> >> >> >> a time. But, erm, can't you just do that? :)
> >> >> >>
> >> >> >
> >> >> > Sure, I can, but I think we should avoid that. As I said this forces
> >> >> > the user to know libbpf's pin path naming algorithm, which is not part
> >> >> > of the libbpf api afaik.
> >> >>
> >> >> Why? If you set the pin path from your application, libbpf will also try
> >> >> to reuse the map from that path. So you don't need to know libbpf's
> >> >> algorithm if you just override it with your own paths?
> >> >>
> >> >
> >> > If I do bpf_object__pin_maps then libbpf decides where it wants to pin
> >> > them. I can set each path by my own, but then why do we need this
> >> > function?
> >>
> >> Erm, what do you mean, "libbpf decides". bpf_object__pin_maps(obj, path)
> >> does exactly what you're asking for: If you supply the path, all maps
> >> are going to be pinned by name underneath that directory...
> >
> > They are pinned under this directory, but with which filename? Today
> > libbpf builds the filename by taking the map name and escaping it, but
> > what will happen if this will have to change?
>
> Then that would have to be done in a way that was backwards-compatible
> so as not to break user code :)
>
> >> > For example, libbpf today uses <path>/<map_name> as the pin path, but
> >> > it is also doing sanitize_pin_path on each path. This means that after
> >> > if use bpf_object__pin_maps I also need to know how libbpf sanitizes
> >> > its paths and mimic that behavior on my side.
> >>
> >> The paths are sanitised so the kernel will accept them. If you're using
> >> invalid paths your pinning is not going to work at all. If you just want
> >> the paths that the maps are pinned under, use bpf_map__get_pin_path().
> >>
> >
> > I am not saying that sanitization is redundant, but rather that it
> > needs to be properly defined (i.e. all dots will always be replaced
> > with underscores), so either expose it in the api or document it so
> > that users don't have to look after the specific implementation.
>
> Lack of documentation is a perennial problem, and patches are always
> welcome. But it is API: libbpf does things a certain way today, and if
> it changes in a way that will break user programs, that is an API break.
>
Okay than, you got me convinced. If we agree that sanitization etc is
all part of the api then I am fine with the current state. I am still
somewhat uncomfortable with forcing the user to read libbpf's source
code just to find out the implementation details, but it may be that
we only need more documentation.
> >> >> > I think that if we have a method to pin all maps at a specific path
> >> >> > there should also be a method for reusing them all from this path,
> >> >> > either by exposing the function that builds the pin path, or a
> >> >> > function that sets all the paths from a root path.
> >> >>
> >> >> What you're asking for is basically a function
> >> >> bpf_object__set_all_pin_paths(obj, path)
> >> >>
> >> >> instead of having to do
> >> >>
> >> >> bpf_object__for_each_map(map, obj) {
> >> >> sprintf(path, "path/%s", bpf_map__name(map));
> >> >> bpf_map__set_pin_path(map, path);
> >> >> }
> >> >>
> >> >> or? Is that really needed?
> >> >>
> >> >
> >> > Yes, that is what I am asking for. Either that or a
> >> > bpf_map__build_pin_path(path, map) That will return a pin path that is
> >> > compatible with libbpf's one, and then I can iterate over all maps.
> >>
> >> See above; this is what bpf_object__pin_maps() does today?
> >
> > I didn't get this last comment. What I meant is that I want something
> > like the bpf_object__pin_maps but that doesn't pin the maps, just
> > exposing its naming part.
>
> Right, OK. Why, though? I can kinda see how it could be convenient to
> (basically )make libbpf behave as if all maps has the 'pinning'
> attribute set, for map reuse. But I'm not sure I can think any concrete
> use cases where this would be needed. What's yours?
>
I am using the same bpf objects (more specifically, the new skeleton
feature) in two different processes that need access to the same
maps/programs (for example, they both need access to shared maps).
Thus, I want to reuse the entire object in both. Since we already have
a way to pin an entire bpf object, I thought it would be convenient to
have a way of reusing it entirely (though I am fine with pinning and
reusing each one manually).
(I cannot set the __uint(pinning, LIBBPF_PIN_BY_NAME) on each since I
want to share the bss map too)
> -Toke
>
