On Fri, Oct 22, 2021 at 05:09:39PM +0200, Peter Zijlstra wrote:
> Use ARCH_STACKWALK to implement a generic __get_wchan().
>
> STACKTRACE should be possible, but the various implementations of
> stack_trace_save_tsk() are not consistent enough for this to work.
> ARCH_STACKWALK is a smaller set of architectures with a better defined
> interface.
>
> Since get_wchan() pins the task in a blocked state, it is not
> necessary to take a reference on the task stack, the task isn't going
> anywhere.
>
> Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> ---
> arch/arm/include/asm/processor.h | 2 -
> arch/arm/kernel/process.c | 22 --------------------
> arch/arm64/include/asm/processor.h | 2 -
> arch/arm64/kernel/process.c | 26 ------------------------
> arch/powerpc/include/asm/processor.h | 2 -
> arch/powerpc/kernel/process.c | 37 -----------------------------------
> arch/riscv/include/asm/processor.h | 3 --
> arch/riscv/kernel/stacktrace.c | 21 -------------------
> arch/s390/include/asm/processor.h | 1
> arch/s390/kernel/process.c | 29 ---------------------------
> arch/x86/include/asm/processor.h | 2 -
> arch/x86/kernel/process.c | 25 -----------------------
> kernel/sched/core.c | 24 ++++++++++++++++++++++
> 13 files changed, 24 insertions(+), 172 deletions(-)
Nice!
> --- a/arch/arm64/kernel/process.c
> +++ b/arch/arm64/kernel/process.c
> @@ -528,32 +528,6 @@ struct task_struct *__switch_to(struct t
> return last;
> }
>
> -unsigned long __get_wchan(struct task_struct *p)
> -{
> - struct stackframe frame;
> - unsigned long stack_page, ret = 0;
> - int count = 0;
> -
> - stack_page = (unsigned long)try_get_task_stack(p);
> - if (!stack_page)
> - return 0;
> -
> - start_backtrace(&frame, thread_saved_fp(p), thread_saved_pc(p));
> -
> - do {
> - if (unwind_frame(p, &frame))
> - goto out;
> - if (!in_sched_functions(frame.pc)) {
> - ret = frame.pc;
> - goto out;
> - }
> - } while (count++ < 16);
> -
> -out:
> - put_task_stack(p);
> - return ret;
> -}
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -1966,6 +1966,30 @@ bool sched_task_on_rq(struct task_struct
> return task_on_rq_queued(p);
> }
>
> +#ifdef CONFIG_ARCH_STACKWALK
> +
> +static bool consume_wchan(void *cookie, unsigned long addr)
> +{
> + unsigned long *wchan = cookie;
> +
> + if (in_sched_functions(addr))
> + return true;
> +
> + *wchan = addr;
> + return false;
> +}
> +
> +static unsigned long __get_wchan(struct task_struct *p)
> +{
> + unsigned long wchan = 0;
> +
> + arch_stack_walk(consume_wchan, &wchan, p, NULL);
> +
> + return wchan;
> +}
It's amazing how much simpler things become with the right
infrastructure!
I gave this a spin on arm64, and /proc/*/wchan looks as expected. The
code looks "obviously correct" to me given the changes in prior patches,
so FWIW:
Reviewed-by: Mark Rutland <mark.rutland@xxxxxxx> [arm64]
Tested-by: Mark Rutland <mark.rutland@xxxxxxx> [arm64]
Thanks,
Mark.
