On Sun, Oct 27, 2019 at 5:04 AM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote: > > Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> writes: > > > On Thu, Oct 24, 2019 at 6:11 AM Toke Høiland-Jørgensen <toke@xxxxxxxxxx> wrote: > >> > >> From: Toke Høiland-Jørgensen <toke@xxxxxxxxxx> > >> > >> With the functions added in previous commits that can automatically pin > >> maps based on their 'pinning' setting, we can support auto-pinning of maps > >> by the simple setting of an option to bpf_object__open. > >> > >> Since auto-pinning only does something if any maps actually have a > >> 'pinning' BTF attribute set, we default the new option to enabled, on the > >> assumption that seamless pinning is what most callers want. > >> > >> When a map has a pin_path set at load time, libbpf will compare the map > >> pinned at that location (if any), and if the attributes match, will re-use > >> that map instead of creating a new one. If no existing map is found, the > >> newly created map will instead be pinned at the location. > >> > >> Programs wanting to customise the pinning can override the pinning paths > >> using bpf_map__set_pin_path() before calling bpf_object__load(). > >> > >> Signed-off-by: Toke Høiland-Jørgensen <toke@xxxxxxxxxx> > >> --- > > > > How have you tested this? From reading the code, all the maps will be > > pinned irregardless of their .pinning setting? > > No, build_pin_path() checks map->pinning :) subtle... build_pin_path() definitely doesn't imply that it's a "maybe build pin path?", but see below for pin_path setting. > > > Please add proper tests to test_progs, testing various modes and > > overrides. > > Can do. > > > You keep trying to add more and more knobs :) Please stop doing that, > > even if we have a good mechanism for extensibility, it doesn't mean we > > need to increase a proliferation of options. > > But I like options! ;) > > > Each option has to be tested. In current version of your patches, you > > have something like 4 or 5 different knobs, do you really want to > > write tests testing each of them? ;) > > Heh, I guess I can cut down the number of options to the number of tests :P > > > Another high-level feedback. I think having separate passes over all > > maps (build_map_pin_paths, reuse, then we already have create_maps) is > > actually making everything more verbose and harder to extend. I'm > > thinking about all these as sub-steps of map creation. Can you please > > try refactoring so all these steps are happening per each map in one > > place: if map needs to be pinned, check if it can be reused, if not - > > create it. This actually will allow to handle races better, because > > you will be able to retry easily, while if it's all spread in > > independent passes, it becomes much harder. Please consider that. > > We'll need at least two passes: set pin_path on open, and check reuse / > create / pin on load. Don't have any objections to consolidating the > other passes into create_maps; will fix, along with your comments below. for BTF-defined maps, can't we just set a pin_path right when we are reading map definition? With that, we won't even need to store .pinning field. Would that work? > > -Toke
