* [PATCH] time/sched_clock: mark sched_clock_read_begin as notrace
@ 2020-09-28 10:49 quanyang.wang
2020-09-28 10:58 ` Peter Zijlstra
0 siblings, 1 reply; 6+ messages in thread
From: quanyang.wang @ 2020-09-28 10:49 UTC (permalink / raw)
To: linux-kernel
Cc: Peter Zijlstra, Thomas Gleixner, Leo Yan, Will Deacon, a.darwish,
Daniel Lezcano, Paul Cercueil, Randy Dunlap, ben.dooks
From: Quanyang Wang <quanyang.wang@windriver.com>
Since sched_clock_read_begin is called by notrace function sched_clock,
it shouldn't be traceable either, or else __ftrace_graph_caller will
run into a dead loop on the path (arm for instance):
ftrace_graph_caller
prepare_ftrace_return
function_graph_enter
ftrace_push_return_trace
trace_clock_local
sched_clock
sched_clock_read_begin
Fixes: 1b86abc1c645 ("sched_clock: Expose struct clock_read_data")
Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com>
---
kernel/time/sched_clock.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
index 1c03eec6ca9b..58459e1359d7 100644
--- a/kernel/time/sched_clock.c
+++ b/kernel/time/sched_clock.c
@@ -68,7 +68,7 @@ static inline u64 notrace cyc_to_ns(u64 cyc, u32 mult, u32 shift)
return (cyc * mult) >> shift;
}
-struct clock_read_data *sched_clock_read_begin(unsigned int *seq)
+notrace struct clock_read_data *sched_clock_read_begin(unsigned int *seq)
{
*seq = raw_read_seqcount_latch(&cd.seq);
return cd.read_data + (*seq & 1);
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] time/sched_clock: mark sched_clock_read_begin as notrace
2020-09-28 10:49 [PATCH] time/sched_clock: mark sched_clock_read_begin as notrace quanyang.wang
@ 2020-09-28 10:58 ` Peter Zijlstra
2020-09-28 12:41 ` Quanyang Wang
2020-09-28 21:33 ` Steven Rostedt
0 siblings, 2 replies; 6+ messages in thread
From: Peter Zijlstra @ 2020-09-28 10:58 UTC (permalink / raw)
To: quanyang.wang
Cc: linux-kernel, Thomas Gleixner, Leo Yan, Will Deacon, a.darwish,
Daniel Lezcano, Paul Cercueil, Randy Dunlap, ben.dooks,
Steven Rostedt
On Mon, Sep 28, 2020 at 06:49:52PM +0800, quanyang.wang@windriver.com wrote:
> From: Quanyang Wang <quanyang.wang@windriver.com>
>
> Since sched_clock_read_begin is called by notrace function sched_clock,
> it shouldn't be traceable either, or else __ftrace_graph_caller will
> run into a dead loop on the path (arm for instance):
>
> ftrace_graph_caller
> prepare_ftrace_return
> function_graph_enter
> ftrace_push_return_trace
> trace_clock_local
> sched_clock
> sched_clock_read_begin
>
> Fixes: 1b86abc1c645 ("sched_clock: Expose struct clock_read_data")
> Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com>
> ---
> kernel/time/sched_clock.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
> index 1c03eec6ca9b..58459e1359d7 100644
> --- a/kernel/time/sched_clock.c
> +++ b/kernel/time/sched_clock.c
> @@ -68,7 +68,7 @@ static inline u64 notrace cyc_to_ns(u64 cyc, u32 mult, u32 shift)
> return (cyc * mult) >> shift;
> }
>
> -struct clock_read_data *sched_clock_read_begin(unsigned int *seq)
> +notrace struct clock_read_data *sched_clock_read_begin(unsigned int *seq)
> {
> *seq = raw_read_seqcount_latch(&cd.seq);
> return cd.read_data + (*seq & 1);
At the very least sched_clock_read_retry() should also be marked such.
But Steve, how come x86 works? Our sched_clock() doesn't have notrace on
at all.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] time/sched_clock: mark sched_clock_read_begin as notrace
2020-09-28 10:58 ` Peter Zijlstra
@ 2020-09-28 12:41 ` Quanyang Wang
2020-09-28 21:33 ` Steven Rostedt
1 sibling, 0 replies; 6+ messages in thread
From: Quanyang Wang @ 2020-09-28 12:41 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux-kernel, Thomas Gleixner, Leo Yan, Will Deacon, a.darwish,
Daniel Lezcano, Paul Cercueil, Randy Dunlap, ben.dooks,
Steven Rostedt
Hi Peter,
On 9/28/20 6:58 PM, Peter Zijlstra wrote:
> On Mon, Sep 28, 2020 at 06:49:52PM +0800, quanyang.wang@windriver.com wrote:
>> From: Quanyang Wang <quanyang.wang@windriver.com>
>>
>> Since sched_clock_read_begin is called by notrace function sched_clock,
>> it shouldn't be traceable either, or else __ftrace_graph_caller will
>> run into a dead loop on the path (arm for instance):
>>
>> ftrace_graph_caller
>> prepare_ftrace_return
>> function_graph_enter
>> ftrace_push_return_trace
>> trace_clock_local
>> sched_clock
>> sched_clock_read_begin
>>
>> Fixes: 1b86abc1c645 ("sched_clock: Expose struct clock_read_data")
>> Signed-off-by: Quanyang Wang <quanyang.wang@windriver.com>
>> ---
>> kernel/time/sched_clock.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/time/sched_clock.c b/kernel/time/sched_clock.c
>> index 1c03eec6ca9b..58459e1359d7 100644
>> --- a/kernel/time/sched_clock.c
>> +++ b/kernel/time/sched_clock.c
>> @@ -68,7 +68,7 @@ static inline u64 notrace cyc_to_ns(u64 cyc, u32 mult, u32 shift)
>> return (cyc * mult) >> shift;
>> }
>>
>> -struct clock_read_data *sched_clock_read_begin(unsigned int *seq)
>> +notrace struct clock_read_data *sched_clock_read_begin(unsigned int *seq)
>> {
>> *seq = raw_read_seqcount_latch(&cd.seq);
>> return cd.read_data + (*seq & 1);
> At the very least sched_clock_read_retry() should also be marked such.
In fact, the sched_clock_read_retry is treated as a "inline" function, so
it doesn't trigger the dead loop. But for safe, add notrace to it is
better.
I will send a V2 patch.
Thanks,
Quanyang
>
> But Steve, how come x86 works? Our sched_clock() doesn't have notrace on
> at all.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] time/sched_clock: mark sched_clock_read_begin as notrace
2020-09-28 10:58 ` Peter Zijlstra
2020-09-28 12:41 ` Quanyang Wang
@ 2020-09-28 21:33 ` Steven Rostedt
2020-09-29 7:13 ` Peter Zijlstra
1 sibling, 1 reply; 6+ messages in thread
From: Steven Rostedt @ 2020-09-28 21:33 UTC (permalink / raw)
To: Peter Zijlstra
Cc: quanyang.wang, linux-kernel, Thomas Gleixner, Leo Yan,
Will Deacon, a.darwish, Daniel Lezcano, Paul Cercueil,
Randy Dunlap, ben.dooks
On Mon, 28 Sep 2020 12:58:59 +0200
Peter Zijlstra <peterz@infradead.org> wrote:
> > -struct clock_read_data *sched_clock_read_begin(unsigned int *seq)
> > +notrace struct clock_read_data *sched_clock_read_begin(unsigned int *seq)
> > {
> > *seq = raw_read_seqcount_latch(&cd.seq);
> > return cd.read_data + (*seq & 1);
>
> At the very least sched_clock_read_retry() should also be marked such.
>
> But Steve, how come x86 works? Our sched_clock() doesn't have notrace on
> at all.
It's because of that magic in the Makefile that you love so much ;-)
CFLAGS_REMOVE_tsc.o = -pg
-- Steve
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] time/sched_clock: mark sched_clock_read_begin as notrace
2020-09-28 21:33 ` Steven Rostedt
@ 2020-09-29 7:13 ` Peter Zijlstra
2020-09-29 14:43 ` Steven Rostedt
0 siblings, 1 reply; 6+ messages in thread
From: Peter Zijlstra @ 2020-09-29 7:13 UTC (permalink / raw)
To: Steven Rostedt
Cc: quanyang.wang, linux-kernel, Thomas Gleixner, Leo Yan,
Will Deacon, a.darwish, Daniel Lezcano, Paul Cercueil,
Randy Dunlap, ben.dooks
On Mon, Sep 28, 2020 at 05:33:31PM -0400, Steven Rostedt wrote:
> On Mon, 28 Sep 2020 12:58:59 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
>
> > > -struct clock_read_data *sched_clock_read_begin(unsigned int *seq)
> > > +notrace struct clock_read_data *sched_clock_read_begin(unsigned int *seq)
> > > {
> > > *seq = raw_read_seqcount_latch(&cd.seq);
> > > return cd.read_data + (*seq & 1);
> >
> > At the very least sched_clock_read_retry() should also be marked such.
> >
> > But Steve, how come x86 works? Our sched_clock() doesn't have notrace on
> > at all.
>
> It's because of that magic in the Makefile that you love so much ;-)
>
> CFLAGS_REMOVE_tsc.o = -pg
ARGH!!, I really should write a script to fix up that mess :/
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] time/sched_clock: mark sched_clock_read_begin as notrace
2020-09-29 7:13 ` Peter Zijlstra
@ 2020-09-29 14:43 ` Steven Rostedt
0 siblings, 0 replies; 6+ messages in thread
From: Steven Rostedt @ 2020-09-29 14:43 UTC (permalink / raw)
To: Peter Zijlstra
Cc: quanyang.wang, linux-kernel, Thomas Gleixner, Leo Yan,
Will Deacon, a.darwish, Daniel Lezcano, Paul Cercueil,
Randy Dunlap, ben.dooks
On Tue, 29 Sep 2020 09:13:33 +0200
Peter Zijlstra <peterz@infradead.org> wrote:
> On Mon, Sep 28, 2020 at 05:33:31PM -0400, Steven Rostedt wrote:
> > On Mon, 28 Sep 2020 12:58:59 +0200
> > Peter Zijlstra <peterz@infradead.org> wrote:
> >
> > > > -struct clock_read_data *sched_clock_read_begin(unsigned int *seq)
> > > > +notrace struct clock_read_data *sched_clock_read_begin(unsigned int *seq)
> > > > {
> > > > *seq = raw_read_seqcount_latch(&cd.seq);
> > > > return cd.read_data + (*seq & 1);
> > >
> > > At the very least sched_clock_read_retry() should also be marked such.
> > >
> > > But Steve, how come x86 works? Our sched_clock() doesn't have notrace on
> > > at all.
> >
> > It's because of that magic in the Makefile that you love so much ;-)
> >
> > CFLAGS_REMOVE_tsc.o = -pg
>
> ARGH!!, I really should write a script to fix up that mess :/
Please don't.
I did this because these files contain lots of functions, that if traced
can cause function tracing to reboot without any information, and as you
have recently found, they are very hard to find when they do happen.
I much rather blacklist an entire file, than to spend time debugging what
function caused the system to reboot.
I'd be OK with both. That is, add "notrace" to all the functions in the
file that isn't traced, just for documentation purposes. Perhaps call it
something else:
file_notrace ?
and have a comment about it being "this is just to let you know that the
functions are not traced because of the file".
And this "file_notrace" could even be:
/*
* Denote functions that are not traced because the make file removes the
* tracing features via a CONFIG_REMOVE_xxx.o = CC_FLAGS_FTRACE or similar.
* This is for documentation purposes only, to remind people why a function
* is not being traced.
*/
#define file_notrace /* Not traced because the file is blacklisted */
We could also add:
#define dir_notrace /* Not traced because the entire directory is blacklisted */
-- Steve
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-09-29 14:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-28 10:49 [PATCH] time/sched_clock: mark sched_clock_read_begin as notrace quanyang.wang
2020-09-28 10:58 ` Peter Zijlstra
2020-09-28 12:41 ` Quanyang Wang
2020-09-28 21:33 ` Steven Rostedt
2020-09-29 7:13 ` Peter Zijlstra
2020-09-29 14:43 ` Steven Rostedt
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).