Messages in this thread Patch in this message | | | From | Mark-PK Tsai <> | Subject | Re: [PATCH] tracing: make tracer_init_tracefs initcall asynchronous | Date | Mon, 14 Mar 2022 19:47:49 +0800 |
| |
> On Fri, 11 Mar 2022 19:26:56 +0800 > Mark-PK Tsai <mark-pk.tsai@mediatek.com> wrote: > > > tracer_init_tracefs() is slow especially when there are > > lots of trace events. > > Create a kthread to do tracer_init_tracefs() asynchronously > > When making comments like this, please provide the benchmarks you used, > with the numbers before and after.
I've retest it with kernel 5.17-rc7 on my arm64 board, and I found that the critical path is trace_eval_sync which spend about 430 ms. It's almost half of the time the do_initcalls spends. Below is the test result.
before after tracer_init_tracefs 29872 us 66 us trace_eval_sync 429695 us 459370 us do_initcalls 797818 us 799890 us
I locally skip trace_eval_sync and got below result.
before after diff do_initcalls 359252 us 341725 us -17527 us
So beside this patch, could we add a kernel parameter or a option to skip it when it doesn't used right after kernel boot?
> > > to speed up the initialization of kernel and move the > > related functions and variables out of init section. > > Thus we sacrifice memory for boot time. I'd like to also see how much > memory is freed from init before and after this patch. >
Below is the INIT_TEXT and INIT_DATA diff:
before after diff INIT_TEXT 7F290 7EDAC -0x4e4 bytes INIT_DATA 116FE8 116FE8 0 bytes
And the init section is 64K aligned on arm64 so that when I test on my platform, the actual memory freed by initmem_free() have no diffrence after apply this patch.
#define SEGMENT_ALIGN SZ_64K
> > > > Signed-off-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com> > > --- > > fs/tracefs/inode.c | 8 ++++---- > > kernel/trace/ftrace.c | 12 ++++++------ > > kernel/trace/trace.c | 21 ++++++++++++++++----- > > kernel/trace/trace_events.c | 2 +- > > 4 files changed, 27 insertions(+), 16 deletions(-) > > > > diff --git a/fs/tracefs/inode.c b/fs/tracefs/inode.c > > index de7252715b12..9a713d6bcb7e 100644 > > --- a/fs/tracefs/inode.c > > +++ b/fs/tracefs/inode.c > > @@ -561,10 +561,10 @@ struct dentry *tracefs_create_dir(const char *name, struct dentry *parent) > > * > > * Returns the dentry of the instances directory. > > */ > > -__init struct dentry *tracefs_create_instance_dir(const char *name, > > - struct dentry *parent, > > - int (*mkdir)(const char *name), > > - int (*rmdir)(const char *name)) > > +struct dentry *tracefs_create_instance_dir(const char *name, > > + struct dentry *parent, > > + int (*mkdir)(const char *name), > > + int (*rmdir)(const char *name)) > > { > > struct dentry *dentry; > > > > diff --git a/kernel/trace/ftrace.c b/kernel/trace/ftrace.c > > index a4b462b6f944..197630cbd5dd 100644 > > --- a/kernel/trace/ftrace.c > > +++ b/kernel/trace/ftrace.c > > @@ -940,7 +940,7 @@ static const struct file_operations ftrace_profile_fops = { > > }; > > > > /* used to initialize the real stat files */ > > -static struct tracer_stat function_stats __initdata = { > > +static struct tracer_stat function_stats = { > > .name = "functions", > > .stat_start = function_stat_start, > > .stat_next = function_stat_next, > > @@ -949,7 +949,7 @@ static struct tracer_stat function_stats __initdata = { > > .stat_show = function_stat_show > > }; > > > > -static __init void ftrace_profile_tracefs(struct dentry *d_tracer) > > +static void ftrace_profile_tracefs(struct dentry *d_tracer) > > { > > struct ftrace_profile_stat *stat; > > struct dentry *entry; > > @@ -991,7 +991,7 @@ static __init void ftrace_profile_tracefs(struct dentry *d_tracer) > > } > > > > #else /* CONFIG_FUNCTION_PROFILER */ > > -static __init void ftrace_profile_tracefs(struct dentry *d_tracer) > > +static void ftrace_profile_tracefs(struct dentry *d_tracer) > > { > > } > > #endif /* CONFIG_FUNCTION_PROFILER */ > > @@ -6359,7 +6359,7 @@ void ftrace_destroy_filter_files(struct ftrace_ops *ops) > > mutex_unlock(&ftrace_lock); > > } > > > > -static __init int ftrace_init_dyn_tracefs(struct dentry *d_tracer) > > +static int ftrace_init_dyn_tracefs(struct dentry *d_tracer) > > { > > > > trace_create_file("available_filter_functions", TRACE_MODE_READ, > > @@ -7754,8 +7754,8 @@ void ftrace_init_tracefs(struct trace_array *tr, struct dentry *d_tracer) > > d_tracer, tr, &ftrace_no_pid_fops); > > } > > > > -void __init ftrace_init_tracefs_toplevel(struct trace_array *tr, > > - struct dentry *d_tracer) > > +void ftrace_init_tracefs_toplevel(struct trace_array *tr, > > + struct dentry *d_tracer) > > { > > /* Only the top level directory has the dyn_tracefs and profile */ > > WARN_ON(!(tr->flags & TRACE_ARRAY_FL_GLOBAL)); > > diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c > > index eb44418574f9..f55da82060e2 100644 > > --- a/kernel/trace/trace.c > > +++ b/kernel/trace/trace.c > > @@ -9562,10 +9562,10 @@ int tracing_init_dentry(void) > > extern struct trace_eval_map *__start_ftrace_eval_maps[]; > > extern struct trace_eval_map *__stop_ftrace_eval_maps[]; > > > > -static struct workqueue_struct *eval_map_wq __initdata; > > -static struct work_struct eval_map_work __initdata; > > +static struct workqueue_struct *eval_map_wq; > > +static struct work_struct eval_map_work; > > > > -static void __init eval_map_work_func(struct work_struct *work) > > +static void eval_map_work_func(struct work_struct *work) > > { > > int len; > > > > @@ -9573,7 +9573,7 @@ static void __init eval_map_work_func(struct work_struct *work) > > trace_insert_eval_map(NULL, __start_ftrace_eval_maps, len); > > } > > > > -static int __init trace_eval_init(void) > > +static int trace_eval_init(void) > > { > > INIT_WORK(&eval_map_work, eval_map_work_func); > > > > @@ -9671,7 +9671,7 @@ static struct notifier_block trace_module_nb = { > > }; > > #endif /* CONFIG_MODULES */ > > > > -static __init int tracer_init_tracefs(void) > > +static int tracefs_init(void *data) > > { > > int ret; > > > > @@ -9721,6 +9721,17 @@ static __init int tracer_init_tracefs(void) > > return 0; > > } > > > > +static __init int tracer_init_tracefs(void) > > +{ > > + struct task_struct *thread; > > + > > + thread = kthread_run(tracefs_init, NULL, "tracefs_init"); > > + if (IS_ERR(thread)) > > + return PTR_ERR(thread); > > + > > + return 0; > > +} > > + > > fs_initcall(tracer_init_tracefs); > > > > static int trace_panic_handler(struct notifier_block *this, > > diff --git a/kernel/trace/trace_events.c b/kernel/trace/trace_events.c > > index 3147614c1812..fe055bef1e8f 100644 > > --- a/kernel/trace/trace_events.c > > +++ b/kernel/trace/trace_events.c > > @@ -3687,7 +3687,7 @@ static __init int event_trace_init_fields(void) > > return 0; > > } > > > > -__init int event_trace_init(void) > > +int event_trace_init(void) > > { > > struct trace_array *tr; > > struct dentry *entry; > > Hmm, this calls early_event_tracer() which is also in __init. Looks like > there's going to be a ripple effect due to this change. > > If we want to go this route, then first a change must be made to remove the > needed functions from init, and then see if we can consolidate it. As there > are some init functions that are duplicated for init purposes.
Got it!
> > -- Steve
| |