On Thu, Jul 8, 2021 at 4:43 AM Martynas Pumputis <m@xxxxxxxxx> wrote:
>
>
>
> On 7/8/21 12:58 AM, Andrii Nakryiko wrote:
> > On Tue, Jul 6, 2021 at 10:24 AM Martynas Pumputis <m@xxxxxxxxx> wrote:
> >>
> >> When loading a BPF program with a pinned map, the loader checks whether
> >> the pinned map can be reused, i.e. their properties match. To derive
> >> such of the pinned map, the loader invokes BPF_OBJ_GET_INFO_BY_FD and
> >> then does the comparison.
> >>
> >> Unfortunately, on < 4.12 kernels the BPF_OBJ_GET_INFO_BY_FD is not
> >> available, so loading the program fails with the following error:
> >>
> >> libbpf: failed to get map info for map FD 5: Invalid argument
> >> libbpf: couldn't reuse pinned map at
> >> '/sys/fs/bpf/tc/globals/cilium_call_policy': parameter
> >> mismatch"
> >> libbpf: map 'cilium_call_policy': error reusing pinned map
> >> libbpf: map 'cilium_call_policy': failed to create:
> >> Invalid argument(-22)
> >> libbpf: failed to load object 'bpf_overlay.o'
> >>
> >> To fix this, probe the kernel for BPF_OBJ_GET_INFO_BY_FD support. If it
> >> doesn't support, then fallback to derivation of the map properties via
> >> /proc/$PID/fdinfo/$MAP_FD.
> >>
> >> Signed-off-by: Martynas Pumputis <m@xxxxxxxxx>
> >> ---
> >> tools/lib/bpf/libbpf.c | 103 +++++++++++++++++++++++++++++++++++++++++++++------
> >> 1 file changed, 92 insertions(+), 11 deletions(-)
> >>
> >
> > [...]
> >
> >> @@ -4406,10 +4478,19 @@ static bool map_is_reuse_compat(const struct bpf_map *map, int map_fd)
> >>
> >> map_info_len = sizeof(map_info);
> >>
> >> - if (bpf_obj_get_info_by_fd(map_fd, &map_info, &map_info_len)) {
> >> - pr_warn("failed to get map info for map FD %d: %s\n",
> >> - map_fd, libbpf_strerror_r(errno, msg, sizeof(msg)));
> >> - return false;
> >> + if (kernel_supports(obj, FEAT_OBJ_GET_INFO_BY_FD)) {
> >
> > why not just try bpf_obj_get_info_by_fd() first, and if it fails
> > always fallback to bpf_get_map_info_from_fdinfo(). No need to do
> > feature detection. This will cut down on the amount of code without
> > any regression in behavior. More so, it will actually now be
> > consistent and good behavior in case of bpf_map__reuse_fd() where we
> > don't have obj. WDYT?
>
> I was thinking about it, but then decided to use the kernel probing
> instead. The reasoning was the following:
>
> 1) For programs with many pinned maps we would issue many failing
> BPF_OBJ_GET_INFO_BY_FD calls (instead of a single one) which might
> hinder the performance.
you can't have so many maps per BPF program to really notice
performance drop for doing an almost no-op bpf() syscall, so I find
this a weak argument
> 2) A canonical way in libbpf to detect features is via kernel_supports()
> and friends, so I didn't want to diverge there.
There are places where we just gracefully handle missing features.
E.g., when loading map with BTF and it fails, we'll retry without BTF.
Or in __perf_buffer__new we allow BPF_OBJ_GET_INFO_BY_FD to fail.
I'd start with a simple fallback and do explicit feature detection
later if it turns out to be problematic.
>
> Re bpf_map__reuse_fd(), if we are OK to break the API before libbpf
> v1.0, then we could extend bpf_map__reuse_fd() to accept the obj.
> However, this would break some consumers of the lib, e.g., iproute2 [1].
No, we are not ok to just arbitrarily break API. And especially that
passing obj to bpf_map API seems weird. We could solve this problem by
remembering the bpf_object pointer inside struct bpf_map, just like we
do for struct bpf_program, but again, I'm not sure it's worth it in
this case. But strategy doesn't help perf_buffer__new() case.
>
> Anyway, if you think that we can ignore 1) and 2), then I'm happy to
> change. Also, I'm going to submit to bpf-next.
I'd like bpf_map__reuse_fd() to have consistent behavior. And avoid
extra code if possible, so let's try a simple fallback for now?
>
> [1]:
> https://github.com/shemminger/iproute2/blob/v5.11.0/lib/bpf_libbpf.c#L98
>
> >
> >
> >> + if (bpf_obj_get_info_by_fd(map_fd, &map_info, &map_info_len)) {
> >> + pr_warn("failed to get map info for map FD %d: %s\n",
> >> + map_fd,
> >> + libbpf_strerror_r(errno, msg, sizeof(msg)));
> >> + return false;
> >> + }
> >
> > [...]
> >
