linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] libbpf: fix BPF_MAP_TYPE_PERF_EVENT_ARRAY auto-pinning
@ 2022-02-22 20:42 Stijn Tintel
  2022-02-23  2:36 ` Song Liu
  0 siblings, 1 reply; 6+ messages in thread
From: Stijn Tintel @ 2022-02-22 20:42 UTC (permalink / raw)
  To: toke, netdev
  Cc: bpf, linux-kernel, kpsingh, john.fastabend, yhs, songliubraving,
	kafai, andrii, daniel, ast

When a BPF map of type BPF_MAP_TYPE_PERF_EVENT_ARRAY doesn't have the
max_entries parameter set, this parameter will be set to the number of
possible CPUs. Due to this, the map_is_reuse_compat function will return
false, causing the following error when trying to reuse the map:

libbpf: couldn't reuse pinned map at '/sys/fs/bpf/m_logging': parameter mismatch

Fix this by checking against the number of possible CPUs if the
max_entries parameter is not set in the map definition.

Fixes: 57a00f41644f ("libbpf: Add auto-pinning of maps when loading BPF objects")
Signed-off-by: Stijn Tintel <stijn@linux-ipv6.be>
---
 tools/lib/bpf/libbpf.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 7f10dd501a52..b076ab728f0e 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -4750,7 +4750,7 @@ static bool map_is_reuse_compat(const struct bpf_map *map, int map_fd)
 	struct bpf_map_info map_info = {};
 	char msg[STRERR_BUFSIZE];
 	__u32 map_info_len;
-	int err;
+	int def_max_entries, err;
 
 	map_info_len = sizeof(map_info);
 
@@ -4763,10 +4763,15 @@ static bool map_is_reuse_compat(const struct bpf_map *map, int map_fd)
 		return false;
 	}
 
+	if (map->def.type == BPF_MAP_TYPE_PERF_EVENT_ARRAY && !map->def.max_entries)
+		def_max_entries = libbpf_num_possible_cpus();
+	else
+		def_max_entries = map->def.max_entries;
+
 	return (map_info.type == map->def.type &&
 		map_info.key_size == map->def.key_size &&
 		map_info.value_size == map->def.value_size &&
-		map_info.max_entries == map->def.max_entries &&
+		map_info.max_entries == def_max_entries &&
 		map_info.map_flags == map->def.map_flags &&
 		map_info.map_extra == map->map_extra);
 }
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] libbpf: fix BPF_MAP_TYPE_PERF_EVENT_ARRAY auto-pinning
  2022-02-22 20:42 [PATCH] libbpf: fix BPF_MAP_TYPE_PERF_EVENT_ARRAY auto-pinning Stijn Tintel
@ 2022-02-23  2:36 ` Song Liu
  2022-02-23 23:15   ` Andrii Nakryiko
  0 siblings, 1 reply; 6+ messages in thread
From: Song Liu @ 2022-02-23  2:36 UTC (permalink / raw)
  To: Stijn Tintel
  Cc: Toke Høiland-Jørgensen, Networking, bpf, open list,
	KP Singh, John Fastabend, Yonghong Song, Song Liu,
	Martin KaFai Lau, Andrii Nakryiko, Daniel Borkmann,
	Alexei Starovoitov

On Tue, Feb 22, 2022 at 12:51 PM Stijn Tintel <stijn@linux-ipv6.be> wrote:
>
> When a BPF map of type BPF_MAP_TYPE_PERF_EVENT_ARRAY doesn't have the
> max_entries parameter set, this parameter will be set to the number of
> possible CPUs. Due to this, the map_is_reuse_compat function will return
> false, causing the following error when trying to reuse the map:
>
> libbpf: couldn't reuse pinned map at '/sys/fs/bpf/m_logging': parameter mismatch
>
> Fix this by checking against the number of possible CPUs if the
> max_entries parameter is not set in the map definition.
>
> Fixes: 57a00f41644f ("libbpf: Add auto-pinning of maps when loading BPF objects")
> Signed-off-by: Stijn Tintel <stijn@linux-ipv6.be>

Acked-by: Song Liu <songliubraving@fb.com>

I think the following fix would be more future proof, but the patch
as-is is better for
stable backport? How about we add a follow up patch on top of current
patch to fix
def->max_entries once for all?

Thanks,
Song

diff --git i/tools/lib/bpf/libbpf.c w/tools/lib/bpf/libbpf.c
index ad43b6ce825e..a1bc1c80bc69 100644
--- i/tools/lib/bpf/libbpf.c
+++ w/tools/lib/bpf/libbpf.c
@@ -4881,10 +4881,9 @@ static int bpf_object__create_map(struct
bpf_object *obj, struct bpf_map *map, b
                        return nr_cpus;
                }
                pr_debug("map '%s': setting size to %d\n", map->name, nr_cpus);
-               max_entries = nr_cpus;
-       } else {
-               max_entries = def->max_entries;
+               def->max_entries = nr_cpus;
        }
+       max_entries = def->max_entries;

        if (bpf_map__is_struct_ops(map))
                create_attr.btf_vmlinux_value_type_id =
map->btf_vmlinux_value_type_id;

^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] libbpf: fix BPF_MAP_TYPE_PERF_EVENT_ARRAY auto-pinning
  2022-02-23  2:36 ` Song Liu
@ 2022-02-23 23:15   ` Andrii Nakryiko
  2022-02-24 10:08     ` Stijn Tintel
  0 siblings, 1 reply; 6+ messages in thread
From: Andrii Nakryiko @ 2022-02-23 23:15 UTC (permalink / raw)
  To: Song Liu
  Cc: Stijn Tintel, Toke Høiland-Jørgensen, Networking, bpf,
	open list, KP Singh, John Fastabend, Yonghong Song, Song Liu,
	Martin KaFai Lau, Andrii Nakryiko, Daniel Borkmann,
	Alexei Starovoitov

On Tue, Feb 22, 2022 at 6:37 PM Song Liu <song@kernel.org> wrote:
>
> On Tue, Feb 22, 2022 at 12:51 PM Stijn Tintel <stijn@linux-ipv6.be> wrote:
> >
> > When a BPF map of type BPF_MAP_TYPE_PERF_EVENT_ARRAY doesn't have the
> > max_entries parameter set, this parameter will be set to the number of
> > possible CPUs. Due to this, the map_is_reuse_compat function will return
> > false, causing the following error when trying to reuse the map:
> >
> > libbpf: couldn't reuse pinned map at '/sys/fs/bpf/m_logging': parameter mismatch
> >
> > Fix this by checking against the number of possible CPUs if the
> > max_entries parameter is not set in the map definition.
> >
> > Fixes: 57a00f41644f ("libbpf: Add auto-pinning of maps when loading BPF objects")
> > Signed-off-by: Stijn Tintel <stijn@linux-ipv6.be>
>
> Acked-by: Song Liu <songliubraving@fb.com>
>
> I think the following fix would be more future proof, but the patch
> as-is is better for
> stable backport? How about we add a follow up patch on top of current
> patch to fix
> def->max_entries once for all?

Keeping special logic for PERF_EVENT_ARRAY in one place is
preferrable. With this, the changes in map_is_reuse_compat() shouldn't
be necessary at all. Stijn, can you please send v2 with Song's
proposed changes?

>
> Thanks,
> Song
>
> diff --git i/tools/lib/bpf/libbpf.c w/tools/lib/bpf/libbpf.c
> index ad43b6ce825e..a1bc1c80bc69 100644
> --- i/tools/lib/bpf/libbpf.c
> +++ w/tools/lib/bpf/libbpf.c
> @@ -4881,10 +4881,9 @@ static int bpf_object__create_map(struct
> bpf_object *obj, struct bpf_map *map, b
>                         return nr_cpus;
>                 }
>                 pr_debug("map '%s': setting size to %d\n", map->name, nr_cpus);
> -               max_entries = nr_cpus;
> -       } else {
> -               max_entries = def->max_entries;
> +               def->max_entries = nr_cpus;
>         }
> +       max_entries = def->max_entries;
>
>         if (bpf_map__is_struct_ops(map))
>                 create_attr.btf_vmlinux_value_type_id =
> map->btf_vmlinux_value_type_id;

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] libbpf: fix BPF_MAP_TYPE_PERF_EVENT_ARRAY auto-pinning
  2022-02-23 23:15   ` Andrii Nakryiko
@ 2022-02-24 10:08     ` Stijn Tintel
  2022-02-24 15:38       ` Stijn Tintel
  0 siblings, 1 reply; 6+ messages in thread
From: Stijn Tintel @ 2022-02-24 10:08 UTC (permalink / raw)
  To: Andrii Nakryiko, Song Liu
  Cc: Toke Høiland-Jørgensen, Networking, bpf, open list,
	KP Singh, John Fastabend, Yonghong Song, Song Liu,
	Martin KaFai Lau, Andrii Nakryiko, Daniel Borkmann,
	Alexei Starovoitov

On 24/02/2022 01:15, Andrii Nakryiko wrote:
> On Tue, Feb 22, 2022 at 6:37 PM Song Liu <song@kernel.org> wrote:
>> On Tue, Feb 22, 2022 at 12:51 PM Stijn Tintel <stijn@linux-ipv6.be> wrote:
>>> When a BPF map of type BPF_MAP_TYPE_PERF_EVENT_ARRAY doesn't have the
>>> max_entries parameter set, this parameter will be set to the number of
>>> possible CPUs. Due to this, the map_is_reuse_compat function will return
>>> false, causing the following error when trying to reuse the map:
>>>
>>> libbpf: couldn't reuse pinned map at '/sys/fs/bpf/m_logging': parameter mismatch
>>>
>>> Fix this by checking against the number of possible CPUs if the
>>> max_entries parameter is not set in the map definition.
>>>
>>> Fixes: 57a00f41644f ("libbpf: Add auto-pinning of maps when loading BPF objects")
>>> Signed-off-by: Stijn Tintel <stijn@linux-ipv6.be>
>> Acked-by: Song Liu <songliubraving@fb.com>
>>
>> I think the following fix would be more future proof, but the patch
>> as-is is better for
>> stable backport? How about we add a follow up patch on top of current
>> patch to fix
>> def->max_entries once for all?
> Keeping special logic for PERF_EVENT_ARRAY in one place is
> preferrable. With this, the changes in map_is_reuse_compat() shouldn't
> be necessary at all. Stijn, can you please send v2 with Song's
> proposed changes?
>
Will do!

Thanks,
Stijn


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] libbpf: fix BPF_MAP_TYPE_PERF_EVENT_ARRAY auto-pinning
  2022-02-24 10:08     ` Stijn Tintel
@ 2022-02-24 15:38       ` Stijn Tintel
  2022-02-25  3:48         ` Song Liu
  0 siblings, 1 reply; 6+ messages in thread
From: Stijn Tintel @ 2022-02-24 15:38 UTC (permalink / raw)
  To: Andrii Nakryiko, Song Liu
  Cc: Toke Høiland-Jørgensen, Networking, bpf, open list,
	KP Singh, John Fastabend, Yonghong Song, Song Liu,
	Martin KaFai Lau, Andrii Nakryiko, Daniel Borkmann,
	Alexei Starovoitov

On 24/02/2022 12:08, Stijn Tintel wrote:
> On 24/02/2022 01:15, Andrii Nakryiko wrote:
>> On Tue, Feb 22, 2022 at 6:37 PM Song Liu <song@kernel.org> wrote:
>>> On Tue, Feb 22, 2022 at 12:51 PM Stijn Tintel <stijn@linux-ipv6.be> wrote:
>>>> When a BPF map of type BPF_MAP_TYPE_PERF_EVENT_ARRAY doesn't have the
>>>> max_entries parameter set, this parameter will be set to the number of
>>>> possible CPUs. Due to this, the map_is_reuse_compat function will return
>>>> false, causing the following error when trying to reuse the map:
>>>>
>>>> libbpf: couldn't reuse pinned map at '/sys/fs/bpf/m_logging': parameter mismatch
>>>>
>>>> Fix this by checking against the number of possible CPUs if the
>>>> max_entries parameter is not set in the map definition.
>>>>
>>>> Fixes: 57a00f41644f ("libbpf: Add auto-pinning of maps when loading BPF objects")
>>>> Signed-off-by: Stijn Tintel <stijn@linux-ipv6.be>
>>> Acked-by: Song Liu <songliubraving@fb.com>
>>>
>>> I think the following fix would be more future proof, but the patch
>>> as-is is better for
>>> stable backport? How about we add a follow up patch on top of current
>>> patch to fix
>>> def->max_entries once for all?
>> Keeping special logic for PERF_EVENT_ARRAY in one place is
>> preferrable. With this, the changes in map_is_reuse_compat() shouldn't
>> be necessary at all. Stijn, can you please send v2 with Song's
>> proposed changes?
>>
> Will do!

Unfortunately that doesn't work. In bpf_object__create_maps, we call
bpf_object__reuse_map and map_is_reuse_compat before
bpf_object__create_map, so we check map_info.max_entries ==
map->def.max_entries before the latter is being overwritten.

So I propose to send a v2 based on my initial submission, but use __u32
for def_max_entries instead of int, unless someone has another suggestion?

Thanks,
Stijn


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] libbpf: fix BPF_MAP_TYPE_PERF_EVENT_ARRAY auto-pinning
  2022-02-24 15:38       ` Stijn Tintel
@ 2022-02-25  3:48         ` Song Liu
  0 siblings, 0 replies; 6+ messages in thread
From: Song Liu @ 2022-02-25  3:48 UTC (permalink / raw)
  To: Stijn Tintel
  Cc: Andrii Nakryiko, Toke Høiland-Jørgensen, Networking,
	bpf, open list, KP Singh, John Fastabend, Yonghong Song,
	Song Liu, Martin KaFai Lau, Andrii Nakryiko, Daniel Borkmann,
	Alexei Starovoitov

On Thu, Feb 24, 2022 at 7:38 AM Stijn Tintel <stijn@linux-ipv6.be> wrote:
>
> On 24/02/2022 12:08, Stijn Tintel wrote:
> > On 24/02/2022 01:15, Andrii Nakryiko wrote:
> >> On Tue, Feb 22, 2022 at 6:37 PM Song Liu <song@kernel.org> wrote:
> >>> On Tue, Feb 22, 2022 at 12:51 PM Stijn Tintel <stijn@linux-ipv6.be> wrote:
> >>>> When a BPF map of type BPF_MAP_TYPE_PERF_EVENT_ARRAY doesn't have the
> >>>> max_entries parameter set, this parameter will be set to the number of
> >>>> possible CPUs. Due to this, the map_is_reuse_compat function will return
> >>>> false, causing the following error when trying to reuse the map:
> >>>>
> >>>> libbpf: couldn't reuse pinned map at '/sys/fs/bpf/m_logging': parameter mismatch
> >>>>
> >>>> Fix this by checking against the number of possible CPUs if the
> >>>> max_entries parameter is not set in the map definition.
> >>>>
> >>>> Fixes: 57a00f41644f ("libbpf: Add auto-pinning of maps when loading BPF objects")
> >>>> Signed-off-by: Stijn Tintel <stijn@linux-ipv6.be>
> >>> Acked-by: Song Liu <songliubraving@fb.com>
> >>>
> >>> I think the following fix would be more future proof, but the patch
> >>> as-is is better for
> >>> stable backport? How about we add a follow up patch on top of current
> >>> patch to fix
> >>> def->max_entries once for all?
> >> Keeping special logic for PERF_EVENT_ARRAY in one place is
> >> preferrable. With this, the changes in map_is_reuse_compat() shouldn't
> >> be necessary at all. Stijn, can you please send v2 with Song's
> >> proposed changes?
> >>
> > Will do!
>
> Unfortunately that doesn't work. In bpf_object__create_maps, we call
> bpf_object__reuse_map and map_is_reuse_compat before
> bpf_object__create_map, so we check map_info.max_entries ==
> map->def.max_entries before the latter is being overwritten.
>
> So I propose to send a v2 based on my initial submission, but use __u32
> for def_max_entries instead of int, unless someone has another suggestion?

How about we move

if (def->type == BPF_MAP_TYPE_PERF_EVENT_ARRAY && !def->max_entries) {
  ...
}

to bpf_object__create_maps() before bpf_object__reuse_map(). And maybe add
a helper function with it?

Thanks,
Song

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2022-02-25  3:48 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-02-22 20:42 [PATCH] libbpf: fix BPF_MAP_TYPE_PERF_EVENT_ARRAY auto-pinning Stijn Tintel
2022-02-23  2:36 ` Song Liu
2022-02-23 23:15   ` Andrii Nakryiko
2022-02-24 10:08     ` Stijn Tintel
2022-02-24 15:38       ` Stijn Tintel
2022-02-25  3:48         ` Song Liu

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).