linux-kernel - Re: [PATCH] tracing: make tracer_init

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20220316155816.1094-1-mark-pk.tsai@mediatek.com>
Date:   Wed, 16 Mar 2022 23:58:16 +0800
From:   Mark-PK Tsai <mark-pk.tsai@...iatek.com>
To:     <mark-pk.tsai@...iatek.com>
CC:     <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>,
        <linux-mediatek@...ts.infradead.org>, <matthias.bgg@...il.com>,
        <mingo@...hat.com>, <rostedt@...dmis.org>, <yj.chiang@...iatek.com>
Subject: Re: [PATCH] tracing: make tracer_init_tracefs initcall asynchronous

> > On Fri, 11 Mar 2022 19:26:56 +0800
> > Mark-PK Tsai <mark-pk.tsai@...iatek.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?
> 

Please ignore this.
I do more tests and found that ftrace_eval_maps is in INIT_DATA.
So the eval_wq may crash if it doesn't finish before init mem
is freed.
And ftrace_eval_maps is big when there are lots of trace event
(About 6KB with arch/arm64/defconfig).
So it seems not a good idea to remove all the needed funcs from init.

So please review v2 [1] I've just push which queue the tracer_init_tracefs()
to eval_map_wq.

[1]: https://lore.kernel.org/lkml/20220316151639.9216-1-mark-pk.tsai@mediatek.com/

> > 
> > > 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
> 
> > >  
> > >  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!
>