Re: [PATCH 6/7] arch: __get_wchan() || ARCH_STACKWALK

[Mark Rutland <mark.rutland@arm.com>]

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@infradead.org>
> ---
>  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@arm.com> [arm64]
Tested-by: Mark Rutland <mark.rutland@arm.com> [arm64]

Thanks,
Mark.