lkml.org 
[lkml]   [2024]   [Feb]   [15]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH RFC bpf-next v2 02/10] bpf/helpers: introduce sleepable timers
From
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);
> +}
> +


\
 
 \ /
  Last update: 2024-05-27 15:06    [W:0.587 / U:0.444 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site