* [PATCH bpf-next v3 0/3] bpf_wq followup series
@ 2024-04-30 10:43 Benjamin Tissoires
2024-04-30 10:43 ` [PATCH bpf-next v3 1/3] bpf: do not walk twice the map on free Benjamin Tissoires
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Benjamin Tissoires @ 2024-04-30 10:43 UTC (permalink / raw
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan
Cc: bpf, linux-kernel, linux-kselftest, Benjamin Tissoires
Few patches that should have been there from day 1.
Anyway, they are coming now.
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
Changes in v3:
- fixed bpf_test module not being able to be removed, because the bpf_wq
was never freed
- Link to v2: https://lore.kernel.org/r/20240430-bpf-next-v2-0-140aa50f0f19@kernel.org
Changes in v2:
- fix wq in hashtabs not being freed (and static call not being used)
- Link to v1: https://lore.kernel.org/r/20240425-bpf-next-v1-0-1d8330e6c643@kernel.org
---
Benjamin Tissoires (3):
bpf: do not walk twice the map on free
bpf: do not walk twice the hash map on free
selftests/bpf: drop an unused local variable
kernel/bpf/arraymap.c | 15 ++++-----
kernel/bpf/hashtab.c | 49 ++++++++---------------------
tools/testing/selftests/bpf/prog_tests/wq.c | 2 --
3 files changed, 21 insertions(+), 45 deletions(-)
---
base-commit: 1bba3b3d373dbafae891e7cb06b8c82c8d62aba1
change-id: 20240425-bpf-next-2114350587e3
Best regards,
--
Benjamin Tissoires <bentiss@kernel.org>
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH bpf-next v3 1/3] bpf: do not walk twice the map on free
2024-04-30 10:43 [PATCH bpf-next v3 0/3] bpf_wq followup series Benjamin Tissoires
@ 2024-04-30 10:43 ` Benjamin Tissoires
2024-04-30 12:37 ` Kumar Kartikeya Dwivedi
2024-04-30 10:43 ` [PATCH bpf-next v3 2/3] bpf: do not walk twice the hash " Benjamin Tissoires
` (2 subsequent siblings)
3 siblings, 1 reply; 8+ messages in thread
From: Benjamin Tissoires @ 2024-04-30 10:43 UTC (permalink / raw
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan
Cc: bpf, linux-kernel, linux-kselftest, Benjamin Tissoires
If someone stores both a timer and a workqueue in a map, on free we
would walk it twice.
Add a check in array_map_free_timers_wq and free the timers
and workqueues if they are present.
Fixes: 246331e3f1ea ("bpf: allow struct bpf_wq to be embedded in arraymaps and hashmaps")
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
no changes in v3
no changes in v2
---
kernel/bpf/arraymap.c | 15 ++++++++-------
1 file changed, 8 insertions(+), 7 deletions(-)
diff --git a/kernel/bpf/arraymap.c b/kernel/bpf/arraymap.c
index 580d07b15471..feabc0193852 100644
--- a/kernel/bpf/arraymap.c
+++ b/kernel/bpf/arraymap.c
@@ -436,13 +436,14 @@ static void array_map_free_timers_wq(struct bpf_map *map)
/* We don't reset or free fields other than timer and workqueue
* on uref dropping to zero.
*/
- if (btf_record_has_field(map->record, BPF_TIMER))
- for (i = 0; i < array->map.max_entries; i++)
- bpf_obj_free_timer(map->record, array_map_elem_ptr(array, i));
-
- if (btf_record_has_field(map->record, BPF_WORKQUEUE))
- for (i = 0; i < array->map.max_entries; i++)
- bpf_obj_free_workqueue(map->record, array_map_elem_ptr(array, i));
+ if (btf_record_has_field(map->record, BPF_TIMER | BPF_WORKQUEUE)) {
+ for (i = 0; i < array->map.max_entries; i++) {
+ if (btf_record_has_field(map->record, BPF_TIMER))
+ bpf_obj_free_timer(map->record, array_map_elem_ptr(array, i));
+ if (btf_record_has_field(map->record, BPF_WORKQUEUE))
+ bpf_obj_free_workqueue(map->record, array_map_elem_ptr(array, i));
+ }
+ }
}
/* Called when map->refcnt goes to zero, either from workqueue or from syscall */
--
2.44.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH bpf-next v3 2/3] bpf: do not walk twice the hash map on free
2024-04-30 10:43 [PATCH bpf-next v3 0/3] bpf_wq followup series Benjamin Tissoires
2024-04-30 10:43 ` [PATCH bpf-next v3 1/3] bpf: do not walk twice the map on free Benjamin Tissoires
@ 2024-04-30 10:43 ` Benjamin Tissoires
2024-04-30 12:52 ` Kumar Kartikeya Dwivedi
2024-04-30 10:43 ` [PATCH bpf-next v3 3/3] selftests/bpf: drop an unused local variable Benjamin Tissoires
2024-04-30 14:40 ` [PATCH bpf-next v3 0/3] bpf_wq followup series patchwork-bot+netdevbpf
3 siblings, 1 reply; 8+ messages in thread
From: Benjamin Tissoires @ 2024-04-30 10:43 UTC (permalink / raw
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan
Cc: bpf, linux-kernel, linux-kselftest, Benjamin Tissoires
If someone stores both a timer and a workqueue in a hash map, on free, we
would walk it twice.
Add a check in htab_free_malloced_timers_or_wq and free the timers
and workqueues if they are present.
Fixes: 246331e3f1ea ("bpf: allow struct bpf_wq to be embedded in arraymaps and hashmaps")
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
changes in v3:
- fix unloading of bpf_wq, again
changes in v2:
- fix wq being not freed (and static call not used)
---
kernel/bpf/hashtab.c | 49 +++++++++++++------------------------------------
1 file changed, 13 insertions(+), 36 deletions(-)
diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
index 0179183c543a..06115f8728e8 100644
--- a/kernel/bpf/hashtab.c
+++ b/kernel/bpf/hashtab.c
@@ -221,13 +221,11 @@ static bool htab_has_extra_elems(struct bpf_htab *htab)
return !htab_is_percpu(htab) && !htab_is_lru(htab);
}
-static void htab_free_prealloced_timers(struct bpf_htab *htab)
+static void htab_free_prealloced_timers_and_wq(struct bpf_htab *htab)
{
u32 num_entries = htab->map.max_entries;
int i;
- if (!btf_record_has_field(htab->map.record, BPF_TIMER))
- return;
if (htab_has_extra_elems(htab))
num_entries += num_possible_cpus();
@@ -235,27 +233,12 @@ static void htab_free_prealloced_timers(struct bpf_htab *htab)
struct htab_elem *elem;
elem = get_htab_elem(htab, i);
- bpf_obj_free_timer(htab->map.record, elem->key + round_up(htab->map.key_size, 8));
- cond_resched();
- }
-}
-
-static void htab_free_prealloced_wq(struct bpf_htab *htab)
-{
- u32 num_entries = htab->map.max_entries;
- int i;
-
- if (!btf_record_has_field(htab->map.record, BPF_WORKQUEUE))
- return;
- if (htab_has_extra_elems(htab))
- num_entries += num_possible_cpus();
-
- for (i = 0; i < num_entries; i++) {
- struct htab_elem *elem;
-
- elem = get_htab_elem(htab, i);
- bpf_obj_free_workqueue(htab->map.record,
- elem->key + round_up(htab->map.key_size, 8));
+ if (btf_record_has_field(htab->map.record, BPF_TIMER))
+ bpf_obj_free_timer(htab->map.record,
+ elem->key + round_up(htab->map.key_size, 8));
+ if (btf_record_has_field(htab->map.record, BPF_WORKQUEUE))
+ bpf_obj_free_workqueue(htab->map.record,
+ elem->key + round_up(htab->map.key_size, 8));
cond_resched();
}
}
@@ -1515,7 +1498,7 @@ static void delete_all_elements(struct bpf_htab *htab)
migrate_enable();
}
-static void htab_free_malloced_timers_or_wq(struct bpf_htab *htab, bool is_timer)
+static void htab_free_malloced_timers_and_wq(struct bpf_htab *htab)
{
int i;
@@ -1527,10 +1510,10 @@ static void htab_free_malloced_timers_or_wq(struct bpf_htab *htab, bool is_timer
hlist_nulls_for_each_entry(l, n, head, hash_node) {
/* We only free timer on uref dropping to zero */
- if (is_timer)
+ if (btf_record_has_field(htab->map.record, BPF_TIMER))
bpf_obj_free_timer(htab->map.record,
l->key + round_up(htab->map.key_size, 8));
- else
+ if (btf_record_has_field(htab->map.record, BPF_WORKQUEUE))
bpf_obj_free_workqueue(htab->map.record,
l->key + round_up(htab->map.key_size, 8));
}
@@ -1544,17 +1527,11 @@ static void htab_map_free_timers_and_wq(struct bpf_map *map)
struct bpf_htab *htab = container_of(map, struct bpf_htab, map);
/* We only free timer and workqueue on uref dropping to zero */
- if (btf_record_has_field(htab->map.record, BPF_TIMER)) {
- if (!htab_is_prealloc(htab))
- htab_free_malloced_timers_or_wq(htab, true);
- else
- htab_free_prealloced_timers(htab);
- }
- if (btf_record_has_field(htab->map.record, BPF_WORKQUEUE)) {
+ if (btf_record_has_field(htab->map.record, BPF_TIMER | BPF_WORKQUEUE)) {
if (!htab_is_prealloc(htab))
- htab_free_malloced_timers_or_wq(htab, false);
+ htab_free_malloced_timers_and_wq(htab);
else
- htab_free_prealloced_wq(htab);
+ htab_free_prealloced_timers_and_wq(htab);
}
}
--
2.44.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [PATCH bpf-next v3 3/3] selftests/bpf: drop an unused local variable
2024-04-30 10:43 [PATCH bpf-next v3 0/3] bpf_wq followup series Benjamin Tissoires
2024-04-30 10:43 ` [PATCH bpf-next v3 1/3] bpf: do not walk twice the map on free Benjamin Tissoires
2024-04-30 10:43 ` [PATCH bpf-next v3 2/3] bpf: do not walk twice the hash " Benjamin Tissoires
@ 2024-04-30 10:43 ` Benjamin Tissoires
2024-04-30 12:37 ` Kumar Kartikeya Dwivedi
2024-04-30 14:40 ` [PATCH bpf-next v3 0/3] bpf_wq followup series patchwork-bot+netdevbpf
3 siblings, 1 reply; 8+ messages in thread
From: Benjamin Tissoires @ 2024-04-30 10:43 UTC (permalink / raw
To: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan
Cc: bpf, linux-kernel, linux-kselftest, Benjamin Tissoires
Some copy/paste leftover, this is never used
Fixes: e3d9eac99afd ("selftests/bpf: wq: add bpf_wq_init() checks")
Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
---
no changes in v3
no changes in v2
---
tools/testing/selftests/bpf/prog_tests/wq.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/wq.c b/tools/testing/selftests/bpf/prog_tests/wq.c
index c4bacd3160e1..99e438fe12ac 100644
--- a/tools/testing/selftests/bpf/prog_tests/wq.c
+++ b/tools/testing/selftests/bpf/prog_tests/wq.c
@@ -36,7 +36,5 @@ void serial_test_wq(void)
void serial_test_failures_wq(void)
{
- LIBBPF_OPTS(bpf_test_run_opts, topts);
-
RUN_TESTS(wq_failures);
}
--
2.44.0
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH bpf-next v3 1/3] bpf: do not walk twice the map on free
2024-04-30 10:43 ` [PATCH bpf-next v3 1/3] bpf: do not walk twice the map on free Benjamin Tissoires
@ 2024-04-30 12:37 ` Kumar Kartikeya Dwivedi
0 siblings, 0 replies; 8+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2024-04-30 12:37 UTC (permalink / raw
To: Benjamin Tissoires
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan, bpf, linux-kernel, linux-kselftest
On Tue, 30 Apr 2024 at 12:43, Benjamin Tissoires <bentiss@kernel.org> wrote:
>
> If someone stores both a timer and a workqueue in a map, on free we
> would walk it twice.
> Add a check in array_map_free_timers_wq and free the timers
> and workqueues if they are present.
>
> Fixes: 246331e3f1ea ("bpf: allow struct bpf_wq to be embedded in arraymaps and hashmaps")
> Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
>
> ---
>
> no changes in v3
>
> no changes in v2
> ---
Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> [...]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf-next v3 3/3] selftests/bpf: drop an unused local variable
2024-04-30 10:43 ` [PATCH bpf-next v3 3/3] selftests/bpf: drop an unused local variable Benjamin Tissoires
@ 2024-04-30 12:37 ` Kumar Kartikeya Dwivedi
0 siblings, 0 replies; 8+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2024-04-30 12:37 UTC (permalink / raw
To: Benjamin Tissoires
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan, bpf, linux-kernel, linux-kselftest
On Tue, 30 Apr 2024 at 12:44, Benjamin Tissoires <bentiss@kernel.org> wrote:
>
> Some copy/paste leftover, this is never used
>
> Fixes: e3d9eac99afd ("selftests/bpf: wq: add bpf_wq_init() checks")
> Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
>
> ---
>
> no changes in v3
>
> no changes in v2
> ---
Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf-next v3 2/3] bpf: do not walk twice the hash map on free
2024-04-30 10:43 ` [PATCH bpf-next v3 2/3] bpf: do not walk twice the hash " Benjamin Tissoires
@ 2024-04-30 12:52 ` Kumar Kartikeya Dwivedi
0 siblings, 0 replies; 8+ messages in thread
From: Kumar Kartikeya Dwivedi @ 2024-04-30 12:52 UTC (permalink / raw
To: Benjamin Tissoires
Cc: Alexei Starovoitov, Daniel Borkmann, Andrii Nakryiko,
Martin KaFai Lau, Eduard Zingerman, Song Liu, Yonghong Song,
John Fastabend, KP Singh, Stanislav Fomichev, Hao Luo, Jiri Olsa,
Mykola Lysenko, Shuah Khan, bpf, linux-kernel, linux-kselftest
On Tue, 30 Apr 2024 at 12:44, Benjamin Tissoires <bentiss@kernel.org> wrote:
>
> If someone stores both a timer and a workqueue in a hash map, on free, we
> would walk it twice.
> Add a check in htab_free_malloced_timers_or_wq and free the timers
> and workqueues if they are present.
>
> Fixes: 246331e3f1ea ("bpf: allow struct bpf_wq to be embedded in arraymaps and hashmaps")
> Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
>
> ---
I had forgotten how the extra_elems logic is working, turns out
everything is in the preallocated elems array and per-cpu extra_elems
stores the *pointer* to them for recycling without hitting the pcpu
allocator, so should be fine (just in case this confuses anyone else).
>
> changes in v3:
> - fix unloading of bpf_wq, again
>
> changes in v2:
> - fix wq being not freed (and static call not used)
> ---
Acked-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH bpf-next v3 0/3] bpf_wq followup series
2024-04-30 10:43 [PATCH bpf-next v3 0/3] bpf_wq followup series Benjamin Tissoires
` (2 preceding siblings ...)
2024-04-30 10:43 ` [PATCH bpf-next v3 3/3] selftests/bpf: drop an unused local variable Benjamin Tissoires
@ 2024-04-30 14:40 ` patchwork-bot+netdevbpf
3 siblings, 0 replies; 8+ messages in thread
From: patchwork-bot+netdevbpf @ 2024-04-30 14:40 UTC (permalink / raw
To: Benjamin Tissoires
Cc: ast, daniel, andrii, martin.lau, eddyz87, song, yonghong.song,
john.fastabend, kpsingh, sdf, haoluo, jolsa, mykolal, shuah, bpf,
linux-kernel, linux-kselftest
Hello:
This series was applied to bpf/bpf-next.git (master)
by Daniel Borkmann <daniel@iogearbox.net>:
On Tue, 30 Apr 2024 12:43:23 +0200 you wrote:
> Few patches that should have been there from day 1.
>
> Anyway, they are coming now.
>
> Signed-off-by: Benjamin Tissoires <bentiss@kernel.org>
> ---
> Changes in v3:
> - fixed bpf_test module not being able to be removed, because the bpf_wq
> was never freed
> - Link to v2: https://lore.kernel.org/r/20240430-bpf-next-v2-0-140aa50f0f19@kernel.org
>
> [...]
Here is the summary with links:
- [bpf-next,v3,1/3] bpf: do not walk twice the map on free
https://git.kernel.org/bpf/bpf-next/c/b98a5c68ccaa
- [bpf-next,v3,2/3] bpf: do not walk twice the hash map on free
https://git.kernel.org/bpf/bpf-next/c/a891711d0166
- [bpf-next,v3,3/3] selftests/bpf: drop an unused local variable
https://git.kernel.org/bpf/bpf-next/c/05cbc217aafb
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-04-30 14:40 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-30 10:43 [PATCH bpf-next v3 0/3] bpf_wq followup series Benjamin Tissoires
2024-04-30 10:43 ` [PATCH bpf-next v3 1/3] bpf: do not walk twice the map on free Benjamin Tissoires
2024-04-30 12:37 ` Kumar Kartikeya Dwivedi
2024-04-30 10:43 ` [PATCH bpf-next v3 2/3] bpf: do not walk twice the hash " Benjamin Tissoires
2024-04-30 12:52 ` Kumar Kartikeya Dwivedi
2024-04-30 10:43 ` [PATCH bpf-next v3 3/3] selftests/bpf: drop an unused local variable Benjamin Tissoires
2024-04-30 12:37 ` Kumar Kartikeya Dwivedi
2024-04-30 14:40 ` [PATCH bpf-next v3 0/3] bpf_wq followup series patchwork-bot+netdevbpf
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 read-only IMAP folder(s) and NNTP newsgroup(s).