On Mon, 10 Apr 2023 13:36:32 +0800 Yafang Shao <laoar.shao@xxxxxxxxx> wrote: > Many thanks for the detailed explanation. > I think ftrace_test_recursion_trylock() can't apply to migreate_enable(). > If we change as follows to prevent migrate_enable() from recursing, > > bit = ftrace_test_recursion_trylock(); > if (bit < 0) > return; > migrate_enable(); > ftrace_test_recursion_unlock(bit); > > We have called migrate_disable() before, so if we don't call > migrate_enable() it will cause other issues. Right. Because you called migrate_disable() before (and protected it with the ftrace_test_recursion_trylock(), the second call is guaranteed to succeed! [1] bit = ftrace_test_recursion_trylock(); if (bit < 0) return; migrate_disable(); ftrace_test_recursion_trylock(bit); [..] [2] ftrace_test_recursion_trylock(); migrate_enable(); ftrace_test_recursion_trylock(bit); You don't even need to read the bit again, because it will be the same as the first call [1]. That's because it returns the recursion level you are in. A function will have the same recursion level through out the function (as long as it always calls ftrace_test_recursion_unlock() between them). bpf_func() bit = ftrace_test_recursion_trylock(); <-- OK migrate_disable(); ftrace_test_recursion_unlock(bit); [..] ftrace_test_recursion_trylock(); <<-- guaranteed to be OK migrate_enable() <<<-- gets traced bpf_func() bit = ftrace_test_recursion_trylock() <-- FAILED if (bit < 0) return; ftrace_test_recursion_unlock(bit); See, still works! The migrate_enable() will never be called without the migrate_disable() as the migrate_disable() only gets called when not being recursed. 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. -- Steve
