I realized this while having a map containing both a struct bpf_timer and a struct bpf_wq: the third argument provided to the bpf_wq callback is not the struct bpf_wq pointer itself, but the pointer to the value in the map.
Which means that the users need to double cast the provided "value" as this is not a struct bpf_wq *.
This is a change of API, but there doesn't seem to be much users of bpf_wq right now, so we should be able to go with this right now.
Signed-off-by: Benjamin Tissoires bentiss@kernel.org --- Benjamin Tissoires (2): bpf: helpers: fix bpf_wq_set_callback_impl signature selftests/bpf: amend for wrong bpf_wq_set_callback_impl signature
kernel/bpf/helpers.c | 2 +- tools/testing/selftests/bpf/bpf_experimental.h | 2 +- tools/testing/selftests/bpf/progs/wq.c | 8 ++++---- tools/testing/selftests/bpf/progs/wq_failures.c | 4 ++-- 4 files changed, 8 insertions(+), 8 deletions(-) --- base-commit: fd8db07705c55a995c42b1e71afc42faad675b0b change-id: 20240705-fix-wq-f069c7fb36c3
Best regards,
I realized this while having a map containing both a struct bpf_timer and a struct bpf_wq: the third argument provided to the bpf_wq callback is not the struct bpf_wq pointer itself, but the pointer to the value in the map.
Which means that the users need to double cast the provided "value" as this is not a struct bpf_wq *.
This is a change of API, but there doesn't seem to be much users of bpf_wq right now, so we should be able to go with this right now.
Fixes: 81f1d7a583fa ("bpf: wq: add bpf_wq_set_callback_impl") Signed-off-by: Benjamin Tissoires bentiss@kernel.org --- kernel/bpf/helpers.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c index 229396172026..5241ba671c5a 100644 --- a/kernel/bpf/helpers.c +++ b/kernel/bpf/helpers.c @@ -2734,7 +2734,7 @@ __bpf_kfunc int bpf_wq_start(struct bpf_wq *wq, unsigned int flags) }
__bpf_kfunc int bpf_wq_set_callback_impl(struct bpf_wq *wq, - int (callback_fn)(void *map, int *key, struct bpf_wq *wq), + int (callback_fn)(void *map, int *key, void *value), unsigned int flags, void *aux__ign) {
On Fri, 2024-07-05 at 15:44 +0200, Benjamin Tissoires wrote:
I realized this while having a map containing both a struct bpf_timer and a struct bpf_wq: the third argument provided to the bpf_wq callback is not the struct bpf_wq pointer itself, but the pointer to the value in the map.
Which means that the users need to double cast the provided "value" as this is not a struct bpf_wq *.
This is a change of API, but there doesn't seem to be much users of bpf_wq right now, so we should be able to go with this right now.
Fixes: 81f1d7a583fa ("bpf: wq: add bpf_wq_set_callback_impl") Signed-off-by: Benjamin Tissoires bentiss@kernel.org
Reviewed-by: Eduard Zingerman eddyz87@gmail.com
See the previous patch: the API was wrong, we were provided the pointer to the value, not the actual struct bpf_wq *.
Signed-off-by: Benjamin Tissoires bentiss@kernel.org --- tools/testing/selftests/bpf/bpf_experimental.h | 2 +- tools/testing/selftests/bpf/progs/wq.c | 8 ++++---- tools/testing/selftests/bpf/progs/wq_failures.c | 4 ++-- 3 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/tools/testing/selftests/bpf/bpf_experimental.h b/tools/testing/selftests/bpf/bpf_experimental.h index eede6fc2ccb4..828556cdc2f0 100644 --- a/tools/testing/selftests/bpf/bpf_experimental.h +++ b/tools/testing/selftests/bpf/bpf_experimental.h @@ -552,7 +552,7 @@ extern void bpf_iter_css_destroy(struct bpf_iter_css *it) __weak __ksym; extern int bpf_wq_init(struct bpf_wq *wq, void *p__map, unsigned int flags) __weak __ksym; extern int bpf_wq_start(struct bpf_wq *wq, unsigned int flags) __weak __ksym; extern int bpf_wq_set_callback_impl(struct bpf_wq *wq, - int (callback_fn)(void *map, int *key, struct bpf_wq *wq), + int (callback_fn)(void *map, int *key, void *value), unsigned int flags__k, void *aux__ign) __ksym; #define bpf_wq_set_callback(timer, cb, flags) \ bpf_wq_set_callback_impl(timer, cb, flags, NULL) diff --git a/tools/testing/selftests/bpf/progs/wq.c b/tools/testing/selftests/bpf/progs/wq.c index 49e712acbf60..e5ac0df59b86 100644 --- a/tools/testing/selftests/bpf/progs/wq.c +++ b/tools/testing/selftests/bpf/progs/wq.c @@ -53,7 +53,7 @@ __u32 ok; __u32 ok_sleepable;
static int test_elem_callback(void *map, int *key, - int (callback_fn)(void *map, int *key, struct bpf_wq *wq)) + int (callback_fn)(void *map, int *key, void *value)) { struct elem init = {}, *val; struct bpf_wq *wq; @@ -84,7 +84,7 @@ static int test_elem_callback(void *map, int *key, }
static int test_hmap_elem_callback(void *map, int *key, - int (callback_fn)(void *map, int *key, struct bpf_wq *wq)) + int (callback_fn)(void *map, int *key, void *value)) { struct hmap_elem init = {}, *val; struct bpf_wq *wq; @@ -114,7 +114,7 @@ static int test_hmap_elem_callback(void *map, int *key, }
/* callback for non sleepable workqueue */ -static int wq_callback(void *map, int *key, struct bpf_wq *work) +static int wq_callback(void *map, int *key, void *value) { bpf_kfunc_common_test(); ok |= (1 << *key); @@ -122,7 +122,7 @@ static int wq_callback(void *map, int *key, struct bpf_wq *work) }
/* callback for sleepable workqueue */ -static int wq_cb_sleepable(void *map, int *key, struct bpf_wq *work) +static int wq_cb_sleepable(void *map, int *key, void *value) { bpf_kfunc_call_test_sleepable(); ok_sleepable |= (1 << *key); diff --git a/tools/testing/selftests/bpf/progs/wq_failures.c b/tools/testing/selftests/bpf/progs/wq_failures.c index 4cbdb425f223..25b51a72fe0f 100644 --- a/tools/testing/selftests/bpf/progs/wq_failures.c +++ b/tools/testing/selftests/bpf/progs/wq_failures.c @@ -28,14 +28,14 @@ struct { } lru SEC(".maps");
/* callback for non sleepable workqueue */ -static int wq_callback(void *map, int *key, struct bpf_wq *work) +static int wq_callback(void *map, int *key, void *value) { bpf_kfunc_common_test(); return 0; }
/* callback for sleepable workqueue */ -static int wq_cb_sleepable(void *map, int *key, struct bpf_wq *work) +static int wq_cb_sleepable(void *map, int *key, void *value) { bpf_kfunc_call_test_sleepable(); return 0;
On Fri, 2024-07-05 at 15:44 +0200, Benjamin Tissoires wrote:
See the previous patch: the API was wrong, we were provided the pointer to the value, not the actual struct bpf_wq *.
Signed-off-by: Benjamin Tissoires bentiss@kernel.org
Would it make sense to update one of the tests, so that it checks the specific value put in the map? E.g. extend struct elem:
struct elem { int answer_to_the_ultimate_question; struct bpf_wq w; };
And put something in there?
[...]
On Fri, Jul 5, 2024 at 1:54 PM Eduard Zingerman eddyz87@gmail.com wrote:
On Fri, 2024-07-05 at 15:44 +0200, Benjamin Tissoires wrote:
See the previous patch: the API was wrong, we were provided the pointer to the value, not the actual struct bpf_wq *.
Signed-off-by: Benjamin Tissoires bentiss@kernel.org
Would it make sense to update one of the tests, so that it checks the specific value put in the map? E.g. extend struct elem:
struct elem { int answer_to_the_ultimate_question; struct bpf_wq w; };
And put something in there?
+1 let's tweak the selftest. pw-bot: cr
On Jul 06 2024, Alexei Starovoitov wrote:
On Fri, Jul 5, 2024 at 1:54 PM Eduard Zingerman eddyz87@gmail.com wrote:
On Fri, 2024-07-05 at 15:44 +0200, Benjamin Tissoires wrote:
See the previous patch: the API was wrong, we were provided the pointer to the value, not the actual struct bpf_wq *.
Signed-off-by: Benjamin Tissoires bentiss@kernel.org
Would it make sense to update one of the tests, so that it checks the specific value put in the map? E.g. extend struct elem:
struct elem { int answer_to_the_ultimate_question; struct bpf_wq w; };
And put something in there?
+1 let's tweak the selftest.
OK, v2 it is then :)
Cheers, Benjamin
linux-kselftest-mirror@lists.linaro.org