Hi Masami, thanks for the review and feedback.
On 2026/5/26 11:37, Masami Hiramatsu wrote: > On Mon, 25 May 2026 21:22:53 +0800 > Tengda Wu <[email protected]> wrote: > >> When a task calls schedule() to yield the CPU, its state remains >> TASK_RUNNING, but its stack is frozen and safe to walk. >> >> Replace task_is_running(tsk) with tsk->on_cpu to avoid overly >> conservative rejections. > > Please see the Sashiko's comment. > > https://sashiko.dev/#/patchset/20260525132253.1889726-1-wutengda%40huaweicloud.com > > When calling Unwind on a task other than the current, IMHO, it is > the responsibility of the caller of this function to ensure that the > stack trace of that task is safe. Agree. > We also should not use tsk->on_cpu, but should use task_on_cpu(tsk). > > BTW, should task_on_cpu() use READ_ONCE() etc? > wait_task_inactive() seems a bit fragile. > > Thanks, > It seems that using task_on_cpu() is not necessary here because: 1. It requires an additional 'rq' parameter not available in the rethook context. 2. It just returns p->on_cpu, which is identical to our current use of tsk->on_cpu. /* file: kernel/sched/sched.h */ static inline int task_on_cpu(struct rq *rq, struct task_struct *p) { return p->on_cpu; } Given these constraints, staying with tsk->on_cpu seems more straightforward for the rethook context. Thanks, Tengda >> >> Fixes: 54ecbe6f1ed5 ("rethook: Add a generic return hook") >> Signed-off-by: Tengda Wu <[email protected]> >> --- >> kernel/trace/rethook.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/kernel/trace/rethook.c b/kernel/trace/rethook.c >> index 5a8bdf88999a..bd5e5f455e85 100644 >> --- a/kernel/trace/rethook.c >> +++ b/kernel/trace/rethook.c >> @@ -250,7 +250,7 @@ unsigned long rethook_find_ret_addr(struct task_struct >> *tsk, unsigned long frame >> if (WARN_ON_ONCE(!cur)) >> return 0; >> >> - if (tsk != current && task_is_running(tsk)) >> + if (tsk != current && tsk->on_cpu) >> return 0; >> >> do { >> -- >> 2.34.1 >> >> > >
