On Mon, 10 Apr 2023 21:56:16 +0800
Yafang Shao <laoar.shao@xxxxxxxxx> wrote:
> Thanks for your explanation again.
> BPF trampoline is a little special. It includes three parts, as follows,
>
> ret = __bpf_prog_enter();
> if (ret)
> prog->bpf_func();
> __bpf_prog_exit();
>
> migrate_disable() is called in __bpf_prog_enter() and migrate_enable()
> in __bpf_prog_exit():
>
> ret = __bpf_prog_enter();
> migrate_disable();
> if (ret)
> prog->bpf_func();
> __bpf_prog_exit();
> migrate_enable();
>
> That said, if we haven't executed migrate_disable() in
> __bpf_prog_enter(), we shouldn't execute migrate_enable() in
> __bpf_prog_exit().
> Can ftrace_test_recursion_trylock() be applied to this pattern ?
Yes, it can! And in this you would need to not call migrate_enable()
because if the trace_recursion_trylock() failed, it would prevent
migrate_disable() from being called (and should not let the bpf_func() from
being called either. And then the migrate_enable in __bpf_prog_exit() would
need to know not to call migrate_enable() which checking the return value
of ftrace_test_recursion_trylock() would give the same value as what the
one before migrate_disable() had.
>
> > Note, the ftrace_test_recursion_*() code needs to be updated because it
> > currently does disable preemption, which it doesn't have to. And that
> > can cause migrate_disable() to do something different. It only disabled
> > preemption, as there was a time that it needed to, but now it doesn't.
> > But the users of it will need to be audited to make sure that they
> > don't need the side effect of it disabling preemption.
> >
>
> disabling preemption is not expected by bpf prog, so I think we should
> change it.
The disabling of preemption was just done because every place that used it
happened to also disable preemption. So it was just a clean up, not a
requirement. Although the documentation said it did disable preemption :-/
See ce5e48036c9e7 ("ftrace: disable preemption when recursion locked")
I think I can add a ftrace_test_recursion_try_aquire() and release() that
is does the same thing without preemption. That way, we don't need to
revert that patch, and use that instead.
-- Steve
