Re: [PATCH] fgraph: Use CPU hotplug mechanism to initialize idle shadow stacks

[Masami Hiramatsu (Google) <mhiramat@kernel.org>]

On Fri, 18 Oct 2024 21:43:00 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:

> From: Steven Rostedt <rostedt@goodmis.org>
> 
> The function graph infrastructure allocates a shadow stack for every task
> when enabled. This includes the idle tasks. The first time the function
> graph is invoked, the shadow stacks are created and never freed until the
> task exits. This includes the idle tasks.
> 
> Only the idle tasks that were for online CPUs had their shadow stacks
> created when function graph tracing started. If function graph tracing is
> enabled and a CPU comes online, the idle task representing that CPU will
> not have its shadow stack created, and all function graph tracing for that
> idle task will be silently dropped.
> 
> Instead, use the CPU hotplug mechanism to allocate the idle shadow stacks.
> This will include idle tasks for CPUs that come online during tracing.
> 
> This issue can be reproduced by:
> 
>  # cd /sys/kernel/tracing
>  # echo 0 > /sys/devices/system/cpu/cpu1/online
>  # echo 0 > set_ftrace_pid
>  # echo function_graph > current_tracer
>  # echo 1 > options/funcgraph-proc
>  # echo 1 > /sys/devices/system/cpu/cpu1
>  # grep '<idle>' per_cpu/cpu1/trace | head
> 
> Before, nothing would show up.
> 
> After:
>  1)    <idle>-0    |   0.811 us    |                        __enqueue_entity();
>  1)    <idle>-0    |   5.626 us    |                      } /* enqueue_entity */
>  1)    <idle>-0    |               |                      dl_server_update_idle_time() {
>  1)    <idle>-0    |               |                        dl_scaled_delta_exec() {
>  1)    <idle>-0    |   0.450 us    |                          arch_scale_cpu_capacity();
>  1)    <idle>-0    |   1.242 us    |                        }
>  1)    <idle>-0    |   1.908 us    |                      }
>  1)    <idle>-0    |               |                      dl_server_start() {
>  1)    <idle>-0    |               |                        enqueue_dl_entity() {
>  1)    <idle>-0    |               |                          task_contending() {
> 
> Note, if tracing stops and restarts, the old way would then initialize
> the onlined CPUs.
> 

Looks good to me, except one comment below;

Acked-by: Masami Hiramatsu (Google) <mhiramat@kernel.org>

[...]
>  int register_ftrace_graph(struct fgraph_ops *gops)
>  {
> +	static bool fgraph_initialized;
>  	int command = 0;
>  	int ret = 0;
>  	int i = -1;
>  
>  	mutex_lock(&ftrace_lock);
>  
> +	if (!fgraph_initialized) {
> +		ret = cpuhp_setup_state(CPUHP_AP_ONLINE_DYN, "fgraph_idle_init",

Nit: Maybe it is better to call it as "tracing/fgraph:online" ?

Thank you,

> +					fgraph_cpu_init, NULL);
> +		if (ret < 0) {
> +			pr_warn("fgraph: Error to init cpu hotplug support\n");
> +			return ret;
> +		}
> +		fgraph_initialized = true;
> +		ret = 0;
> +	}
> +
>  	if (!fgraph_array[0]) {
>  		/* The array must always have real data on it */
>  		for (i = 0; i < FGRAPH_ARRAY_SIZE; i++)
> -- 
> 2.45.2
> 


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>