On Feb 15 2024, Martin KaFai Lau wrote:
> 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?
Probably :)
TBH, everytime I work with RCUs I spent countless hours trying to
re-understand everything, and in this case I'm currently in the "let's
make it work" process than fixing concurrency issues.
I still gave it a shot in case it solves my issue, but no, I still have
the crash.
But given that callback_fn might sleep, isn't it an issue to keep the
RCU_reader lock so long? (we don't seem to call synchronize_rcu() so it
might be fine, but I'd like the confirmation from someone else).
>
> 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.
Basically I'm just a monkey here. I've been told that I should use
bpf_timer[0]. But my implementation is not finished, as Alexei mentioned
that we should bypass hrtimer if I'm not wrong [1].
>
> Have you thought about if it is possible to create a more generic kfunc like
> bpf_schedule_work() to delay work to a workqueue ?
>
AFAIU if we were to have a separate bpf_schedule_work(), we still need
all of the infra of bpf_timer, because we need to keep the programs
around in the same way bpf_timer does. So basically, bpf_timer will not
only be about hrtimers, but anything that need to run an async callback.
I submitted this RFC v2 not for the "this is ready", but mostly because
there is a crash and I can't see where it comes from, and I suspect this
is from a piece I do not understand (translation from the BPF langage
into actual elf assembly).
Cheers,
Benjamin
[0] https://lore.kernel.org/bpf/ztou4yyrsdfmmhdwgu2f2noartpqklhvtbw7vj2ptk54eqohvb@qci7bcnbd56q/T/#mc9cab17138b13c83299f0836ca0b2dde0643ea4b
[1] https://lore.kernel.org/bpf/ztou4yyrsdfmmhdwgu2f2noartpqklhvtbw7vj2ptk54eqohvb@qci7bcnbd56q/T/#mf59824ad625992b980afbc4f27c83e76245815e7
>
>
> > + 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);
> > +}
> > +
>
