Re: [PATCH bpf-next v2 6/8] selftests/bpf: Check bpf_wq value membership

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Low] selftests/bpf: multi-line comment style violation in
  bpf_wq_membership.c
- [Low] selftests/bpf: single-use macros DEFINE_WQ_MAP and
  WQ_DELETE_INIT
--

commit 67a78af8389db430f474e1214b9e087d1c068104
Author: Yun Lu <luyun@kylinos.cn>

selftests/bpf: Check bpf_wq value membership

This commit adds BPF selftests covering bpf_wq initialization on deleted or
replaced map values as well as live HASH and ARRAY map callbacks.

> diff --git a/tools/testing/selftests/bpf/progs/bpf_wq_membership.c b/tools/testing/selftests/bpf/progs/bpf_wq_membership.c
> new file mode 100644
> index 0000000000000..b78b4f1422b3d
> --- /dev/null
> +++ b/tools/testing/selftests/bpf/progs/bpf_wq_membership.c

[ ... ]

> +/* Delete and initialize through the retained pointer in one RCU section.
> + * Never configure or start work through that pointer, even if init succeeds.
> + */

[Severity: Low]
This isn't a bug, but BPF coding style places the opening /* of a
multi-line comment on its own line.

> +#define WQ_DELETE_INIT(map_sym, v)					\
> +	({								\
> +		long __e;						\
> +		bpf_rcu_read_lock();					\
> +		res_delete_err = bpf_map_delete_elem(&map_sym, &key);	\
> +		__e = res_delete_err;					\
> +		if (!__e)						\
> +			res_init_err = bpf_wq_init(&(v)->wq, &map_sym, 0); \
> +		bpf_rcu_read_unlock();					\
> +		__e;							\
> +	})
> +
> +#define DEFINE_WQ_MAP(map_sym, value_type)				\
> +static int map_sym##_cb(void *map, int *key, void *value)		\
> +{									\
> +	value_type *v = value;						\
> +									\
> +	__sync_fetch_and_add(&cb_count[v->cookie & (CB_SLOTS - 1)], 1);	\
> +	return 0;							\
> +}									\
> +									\
> +SEC("syscall")								\
> +int map_sym##_run(void *ctx)						\
> +{									\

[ ... ]

> +DEFINE_WQ_MAP(values, struct wq_value)

[Severity: Low]
This isn't a bug, but DEFINE_WQ_MAP() and WQ_DELETE_INIT() are only
expanded once, and WQ_DELETE_INIT() implicitly captures the caller's local
key variable while returning an unused value.  Could values_cb() and
values_run() be written directly as plain functions with WQ_DELETE_INIT()
inlined, and could these subtests be added to
tools/testing/selftests/bpf/prog_tests/wq.c instead of a separate test
file?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106095747.471718-1-luyun_611@163.com?part=6