On Mon, Jun 28, 2021 at 11:34 PM Andrii Nakryiko
<andrii.nakryiko@xxxxxxxxx> wrote:
>
> Have you considered alternatively to implement something like
> bpf_ringbuf_query() for BPF ringbuf that will allow to query various
> things about the timer (e.g., whether it is active or not, and, of
> course, remaining expiry time). That will be more general, easier to
> extend, and will cover this use case:
>
> long exp = bpf_timer_query(&t->timer, BPF_TIMER_EXPIRY);
> bpf_timer_start(&t->timer, new_callback, exp);
yes, but...
hrtimer_get_remaining + timer_start to that value is racy
and not accurate.
hrtimer_get_expires_ns + timer_start(MODE_ABS)
would be accurate, but that's an unnecessary complication.
To live replace old bpf prog with new one
bpf_for_each_map_elem() { bpf_timer_set_callback(new_prog); }
is much faster, since timers don't need to be dequeue, enqueue.
No need to worry about hrtimer machinery internal changes, etc.
bpf prog being replaced shouldn't be affecting the rest of the system.
> This will keep common timer scenarios to just two steps, init + start,
> but won't prevent more complicated ones. Things like extending
> expiration by one second relative that what was remaining will be
> possible as well.
Extending expiration would be more accurate with hrtimer_forward_now().
All of the above points are minor compared to the verifier advantage.
bpf_timer_set_callback() typically won't be called from the callback.
So verifier's insn_procssed will be drastically lower.
The combinatorial explosion of states even for this small
selftests/bpf/progs/timer.c is significant.
With bpf_timer_set_callback() is done outside of callback the verifier
behavior will be predictable.
To some degree patches 4-6 could have been delayed, but since the
the algo is understood and it's working, I'm going to keep them.
It's nice to have that flexibility, but the less pressure on the
verifier the better.
