| Message ID | 20241018214300.6df82178@rorschach (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 2c02f7375e658ae93d57a31a66f91b62754ef8f1 |
| Headers | show |
| Series | fgraph: Use CPU hotplug mechanism to initialize idle shadow stacks | expand |
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 >
On Mon, 21 Oct 2024 14:58:10 +0900 Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > > + 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" ? Ah this already went upstream. But yeah, I can rename it for the merge window. -- Steve
On Mon, 21 Oct 2024 14:58:10 +0900 Masami Hiramatsu (Google) <mhiramat@kernel.org> wrote: > > + 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" ? I'm going to call it "fgraph:online" as it's not technically tracing. I'll also add it to my urgent branch so it gets into 6.12. -- Steve
diff --git a/kernel/trace/fgraph.c b/kernel/trace/fgraph.c index d7d4fb403f6f..43f4e3f57438 100644 --- a/kernel/trace/fgraph.c +++ b/kernel/trace/fgraph.c @@ -1160,19 +1160,13 @@ void fgraph_update_pid_func(void) static int start_graph_tracing(void) { unsigned long **ret_stack_list; - int ret, cpu; + int ret; ret_stack_list = kmalloc(SHADOW_STACK_SIZE, GFP_KERNEL); if (!ret_stack_list) return -ENOMEM; - /* The cpu_boot init_task->ret_stack will never be freed */ - for_each_online_cpu(cpu) { - if (!idle_task(cpu)->ret_stack) - ftrace_graph_init_idle_task(idle_task(cpu), cpu); - } - do { ret = alloc_retstack_tasklist(ret_stack_list); } while (ret == -EAGAIN); @@ -1242,14 +1236,34 @@ static void ftrace_graph_disable_direct(bool disable_branch) fgraph_direct_gops = &fgraph_stub; } +/* The cpu_boot init_task->ret_stack will never be freed */ +static int fgraph_cpu_init(unsigned int cpu) +{ + if (!idle_task(cpu)->ret_stack) + ftrace_graph_init_idle_task(idle_task(cpu), cpu); + return 0; +} + 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", + 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++)