[PATCH] time/sched_clock: mark sched_clock_read_begin as notrace

[PATCH] time/sched_clock: mark sched_clock_read_begin as notrace

[quanyang.wang]

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

Re: [PATCH] time/sched_clock: mark sched_clock_read_begin as notrace

[Peter Zijlstra]

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.

Re: [PATCH] time/sched_clock: mark sched_clock_read_begin as notrace

[Quanyang Wang]

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.

Re: [PATCH] time/sched_clock: mark sched_clock_read_begin as notrace

[Steven Rostedt]

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

Re: [PATCH] time/sched_clock: mark sched_clock_read_begin as notrace

[Peter Zijlstra]

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 :/

Re: [PATCH] time/sched_clock: mark sched_clock_read_begin as notrace

[Steven Rostedt]

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