From: Martin KaFai Lau <martin.lau@linux.dev>
To: Benjamin Tissoires <bentiss@kernel.org>
Cc: bpf@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-input@vger.kernel.org, linux-doc@vger.kernel.org,
linux-kselftest@vger.kernel.org,
Alexei Starovoitov <ast@kernel.org>,
Daniel Borkmann <daniel@iogearbox.net>,
John Fastabend <john.fastabend@gmail.com>,
Andrii Nakryiko <andrii@kernel.org>,
Eduard Zingerman <eddyz87@gmail.com>, Song Liu <song@kernel.org>,
Yonghong Song <yonghong.song@linux.dev>,
KP Singh <kpsingh@kernel.org>,
Stanislav Fomichev <sdf@google.com>, Hao Luo <haoluo@google.com>,
Jiri Olsa <jolsa@kernel.org>, Jiri Kosina <jikos@kernel.org>,
Benjamin Tissoires <benjamin.tissoires@redhat.com>,
Jonathan Corbet <corbet@lwn.net>, Shuah Khan <shuah@kernel.org>
Subject: Re: [PATCH RFC bpf-next v2 02/10] bpf/helpers: introduce sleepable timers
Date: Thu, 15 Feb 2024 22:36:16 -0800 [thread overview]
Message-ID: <a72147f5-2b7d-4267-9881-6a645c575838@linux.dev> (raw)
In-Reply-To: <20240214-hid-bpf-sleepable-v2-2-5756b054724d@kernel.org>
On 2/14/24 9:18 AM, Benjamin Tissoires wrote:
> +static void bpf_timer_work_cb(struct work_struct *work)
> +{
> + struct bpf_hrtimer *t = container_of(work, struct bpf_hrtimer, work);
> + struct bpf_map *map = t->map;
> + void *value = t->value;
> + bpf_callback_t callback_fn;
> + void *key;
> + u32 idx;
> +
> + BTF_TYPE_EMIT(struct bpf_timer);
> +
> + rcu_read_lock();
> + callback_fn = rcu_dereference(t->sleepable_cb_fn);
> + rcu_read_unlock();
I took a very brief look at patch 2. One thing that may worth to ask here, the
rcu_read_unlock() seems to be done too early. It is protecting the
t->sleepable_cb_fn (?), so should it be done after finished using the callback_fn?
A high level design question. The intention of the new
bpf_timer_set_sleepable_cb() kfunc is actually to delay work to a workqueue. It
is useful to delay work from the bpf_timer_cb and it may also useful to delay
work from other bpf running context (e.g. the networking hooks like "tc"). The
bpf_timer_set_sleepable_cb() seems to be unnecessary forcing delay-work must be
done in a bpf_timer_cb.
Have you thought about if it is possible to create a more generic kfunc like
bpf_schedule_work() to delay work to a workqueue ?
> + if (!callback_fn)
> + return;
> +
> + /* FIXME: do we need any locking? */
> + if (map->map_type == BPF_MAP_TYPE_ARRAY) {
> + struct bpf_array *array = container_of(map, struct bpf_array, map);
> +
> + /* compute the key */
> + idx = ((char *)value - array->value) / array->elem_size;
> + key = &idx;
> + } else { /* hash or lru */
> + key = value - round_up(map->key_size, 8);
> + }
> +
> + /* FIXME: this crashes the system with
> + * BUG: kernel NULL pointer dereference, address: 000000000000000b
> + */
> + /* callback_fn((u64)(long)map, (u64)(long)key, (u64)(long)value, 0, 0); */
> + /* The verifier checked that return value is zero. */
> +}
> +
[ ... ]
> +/* FIXME: use kernel doc style */
> +/* Description
> + * Configure the timer to call *callback_fn* static function in a
> + * sleepable context.
> + * Return
> + * 0 on success.
> + * **-EINVAL** if *timer* was not initialized with bpf_timer_init() earlier.
> + * **-EPERM** if *timer* is in a map that doesn't have any user references.
> + * The user space should either hold a file descriptor to a map with timers
> + * or pin such map in bpffs. When map is unpinned or file descriptor is
> + * closed all timers in the map will be cancelled and freed.
> + */
> +__bpf_kfunc int bpf_timer_set_sleepable_cb(struct bpf_timer_kern *timer,
> + int (callback_fn)(void *map, int *key, struct bpf_timer *timer))
> +{
> + struct bpf_throw_ctx ctx = {};
> +
> + /* FIXME: definietely not sure this is OK */
> + arch_bpf_stack_walk(bpf_stack_walker, &ctx);
> + WARN_ON_ONCE(!ctx.aux);
> +
> + if (!ctx.aux)
> + return -EINVAL;
> +
> + return __bpf_timer_set_callback(timer, (void *)callback_fn, ctx.aux, true);
> +}
> +
next prev parent reply other threads:[~2024-02-16 6:36 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-14 17:18 [PATCH RFC bpf-next v2 00/10] allow HID-BPF to do device IOs Benjamin Tissoires
2024-02-14 17:18 ` [PATCH RFC bpf-next v2 01/10] bpf/verifier: introduce in_sleepable() helper Benjamin Tissoires
2024-02-14 17:18 ` [PATCH RFC bpf-next v2 02/10] bpf/helpers: introduce sleepable timers Benjamin Tissoires
2024-02-15 15:23 ` Benjamin Tissoires
2024-02-16 9:50 ` Benjamin Tissoires
2024-02-21 2:59 ` Alexei Starovoitov
2024-02-21 16:06 ` Benjamin Tissoires
2024-02-16 6:36 ` Martin KaFai Lau [this message]
2024-02-16 8:13 ` Benjamin Tissoires
2024-02-16 14:18 ` Toke Høiland-Jørgensen
2024-02-16 14:58 ` Benjamin Tissoires
2024-02-17 13:42 ` Toke Høiland-Jørgensen
2024-02-21 2:49 ` Alexei Starovoitov
2024-02-16 14:20 ` Toke Høiland-Jørgensen
2024-02-16 16:58 ` Kui-Feng Lee
2024-02-14 17:18 ` [PATCH RFC bpf-next v2 03/10] bpf/verifier: allow more maps in sleepable bpf programs Benjamin Tissoires
2024-02-21 2:51 ` Alexei Starovoitov
2024-02-14 17:18 ` [PATCH RFC bpf-next v2 04/10] HID: bpf/dispatch: regroup kfuncs definitions Benjamin Tissoires
2024-02-14 17:18 ` [PATCH RFC bpf-next v2 05/10] HID: bpf: export hid_hw_output_report as a BPF kfunc Benjamin Tissoires
2024-02-14 17:18 ` [PATCH RFC bpf-next v2 06/10] selftests/hid: Add test for hid_bpf_hw_output_report Benjamin Tissoires
2024-02-14 17:18 ` [PATCH RFC bpf-next v2 07/10] HID: bpf: allow to inject HID event from BPF Benjamin Tissoires
2024-02-14 17:18 ` [PATCH RFC bpf-next v2 08/10] selftests/hid: add tests for hid_bpf_input_report Benjamin Tissoires
2024-02-14 17:18 ` [PATCH RFC bpf-next v2 09/10] HID: bpf: allow to use bpf_timer_set_sleepable_cb() in tracing callbacks Benjamin Tissoires
2024-02-14 17:18 ` [PATCH RFC bpf-next v2 10/10] selftests/hid: add test for bpf_timer Benjamin Tissoires
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=a72147f5-2b7d-4267-9881-6a645c575838@linux.dev \
--to=martin.lau@linux.dev \
--cc=andrii@kernel.org \
--cc=ast@kernel.org \
--cc=benjamin.tissoires@redhat.com \
--cc=bentiss@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=corbet@lwn.net \
--cc=daniel@iogearbox.net \
--cc=eddyz87@gmail.com \
--cc=haoluo@google.com \
--cc=jikos@kernel.org \
--cc=john.fastabend@gmail.com \
--cc=jolsa@kernel.org \
--cc=kpsingh@kernel.org \
--cc=linux-doc@vger.kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=sdf@google.com \
--cc=shuah@kernel.org \
--cc=song@kernel.org \
--cc=yonghong.song@linux.dev \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).