On Mon, Apr 10, 2023 at 10:12 PM Steven Rostedt <rostedt@xxxxxxxxxxx> wrote:
>
> 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.
>
That needs some changes in invoke_bpf_prog() in files
arch/${ARCH}/net/bpf_jit_comp.c.
But I will have a try. We can then remove the bpf_prog->active, that
will be a good cleanup as well.
>
> >
> > > 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
--
Regards
Yafang
