Re: sched_clock and device suspend/resume

Re: sched_clock and device suspend/resume

[Steven Rostedt]

[ Resending as your Cc was screwed up and caused my reply to mess up
  the Cc list ]

On Wed, 10 Jul 2019 08:20:37 -0700
Sodagudi Prasad <psodagud@codeaurora.org> wrote:

> Another option is printing the epoch/cycles information in every print 
> statement similar to thread id or processor id added 
> recently(CONFIG_PRINTK_CALLER). This can be avoided if we start 
> accounting suspend time in sched_clock.

Or another option is add a new clock that printk and tracing can use.
tracing already can switch between clocks trivially.

sched_clock_continuous() ? (I know, horrible name), that simply keeps
track of the time delta at suspend and returns:

	sched_clock() + delta;

This will prevent other issues happening by modifying sched_clock(),
specifically, screwing up the scheduler (what sched_clock()'s main
purpose is for). We don't want the scheduler to think that a process
was running for hours when it has spent most of that time in the
suspend state. Which is probably your answer to why it was designed
that way.

-- Steve

Re: sched_clock and device suspend/resume

[Thomas Gleixner]

On Wed, 10 Jul 2019, Steven Rostedt wrote:

> 
> [ Resending as your Cc was screwed up and caused my reply to mess up
>   the Cc list ]
> 
> On Wed, 10 Jul 2019 08:20:37 -0700
> Sodagudi Prasad <psodagud@codeaurora.org> wrote:
> 
> > Another option is printing the epoch/cycles information in every print 
> > statement similar to thread id or processor id added 
> > recently(CONFIG_PRINTK_CALLER). This can be avoided if we start 
> > accounting suspend time in sched_clock.
> 
> Or another option is add a new clock that printk and tracing can use.
> tracing already can switch between clocks trivially.
> 
> sched_clock_continuous() ? (I know, horrible name), that simply keeps
> track of the time delta at suspend and returns:
> 
> 	sched_clock() + delta;

Which you get already when you do

# echo boot > /sys/kernel/debug/tracing/trace_clock

Thanks,

	tglx

Re: sched_clock and device suspend/resume

[Steven Rostedt]

On Wed, 10 Jul 2019 20:35:32 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Wed, 10 Jul 2019, Steven Rostedt wrote:
> 
> > 
> > [ Resending as your Cc was screwed up and caused my reply to mess up
> >   the Cc list ]
> > 
> > On Wed, 10 Jul 2019 08:20:37 -0700
> > Sodagudi Prasad <psodagud@codeaurora.org> wrote:
> >   
> > > Another option is printing the epoch/cycles information in every print 
> > > statement similar to thread id or processor id added 
> > > recently(CONFIG_PRINTK_CALLER). This can be avoided if we start 
> > > accounting suspend time in sched_clock.  
> > 
> > Or another option is add a new clock that printk and tracing can use.
> > tracing already can switch between clocks trivially.
> > 
> > sched_clock_continuous() ? (I know, horrible name), that simply keeps
> > track of the time delta at suspend and returns:
> > 
> > 	sched_clock() + delta;  
> 
> Which you get already when you do
> 
> # echo boot > /sys/kernel/debug/tracing/trace_clock
> 

So basically the answer here is to change printk to use
ktime_get_boot_fast_ns() instead of local_clock()?

-- Steve

Re: sched_clock and device suspend/resume

[Thomas Gleixner]

On Wed, 10 Jul 2019, Steven Rostedt wrote:
> On Wed, 10 Jul 2019 20:35:32 +0200 (CEST)
> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > On Wed, 10 Jul 2019, Steven Rostedt wrote:
> > 
> > > 
> > > [ Resending as your Cc was screwed up and caused my reply to mess up
> > >   the Cc list ]
> > > 
> > > On Wed, 10 Jul 2019 08:20:37 -0700
> > > Sodagudi Prasad <psodagud@codeaurora.org> wrote:
> > >   
> > > > Another option is printing the epoch/cycles information in every print 
> > > > statement similar to thread id or processor id added 
> > > > recently(CONFIG_PRINTK_CALLER). This can be avoided if we start 
> > > > accounting suspend time in sched_clock.  
> > > 
> > > Or another option is add a new clock that printk and tracing can use.
> > > tracing already can switch between clocks trivially.
> > > 
> > > sched_clock_continuous() ? (I know, horrible name), that simply keeps
> > > track of the time delta at suspend and returns:
> > > 
> > > 	sched_clock() + delta;  
> > 
> > Which you get already when you do
> > 
> > # echo boot > /sys/kernel/debug/tracing/trace_clock
> > 
> 
> So basically the answer here is to change printk to use
> ktime_get_boot_fast_ns() instead of local_clock()?

Aargh. That was tracing.

There was a patchset floating around which actually implemented that clock
choice for sched_clock as well. Don't know why that was never merged.

Thanks,

	tglx

Re: sched_clock and device suspend/resume

[Steven Rostedt]

[ Removed the two emails that were bouncing ]

On Wed, 10 Jul 2019 21:06:57 +0200 (CEST)
Thomas Gleixner <tglx@linutronix.de> wrote:

> > > > sched_clock_continuous() ? (I know, horrible name), that simply keeps
> > > > track of the time delta at suspend and returns:
> > > > 
> > > > 	sched_clock() + delta;    
> > > 
> > > Which you get already when you do
> > > 
> > > # echo boot > /sys/kernel/debug/tracing/trace_clock
> > >   
> > 
> > So basically the answer here is to change printk to use
> > ktime_get_boot_fast_ns() instead of local_clock()?  
> 
> Aargh. That was tracing.
> 
> There was a patchset floating around which actually implemented that clock
> choice for sched_clock as well. Don't know why that was never merged.

Will it cause issues with the scheduler though. If it doesn't stop
during suspend, can't that make the scheduler think that processes were
using the CPU during the entire suspend and screw up the accounting?

-- Steve

Re: sched_clock and device suspend/resume

[Thomas Gleixner]

On Wed, 10 Jul 2019, Steven Rostedt wrote:
> 
> [ Removed the two emails that were bouncing ]
> 
> On Wed, 10 Jul 2019 21:06:57 +0200 (CEST)
> Thomas Gleixner <tglx@linutronix.de> wrote:
> 
> > > > > sched_clock_continuous() ? (I know, horrible name), that simply keeps
> > > > > track of the time delta at suspend and returns:
> > > > > 
> > > > > 	sched_clock() + delta;    
> > > > 
> > > > Which you get already when you do
> > > > 
> > > > # echo boot > /sys/kernel/debug/tracing/trace_clock
> > > >   
> > > 
> > > So basically the answer here is to change printk to use
> > > ktime_get_boot_fast_ns() instead of local_clock()?  
> > 
> > Aargh. That was tracing.
> > 
> > There was a patchset floating around which actually implemented that clock
> > choice for sched_clock as well. Don't know why that was never merged.
> 
> Will it cause issues with the scheduler though. If it doesn't stop
> during suspend, can't that make the scheduler think that processes were
> using the CPU during the entire suspend and screw up the accounting?

Duh. My brain is not working.

Not sched_clock, the patches were for printk so you could select a printk
clock. Can't find them right now.

Thanks,

	tglx

Re: sched_clock and device suspend/resume

[Thomas Gleixner]

On Wed, 10 Jul 2019, Thomas Gleixner wrote:

> On Wed, 10 Jul 2019, Steven Rostedt wrote:
> > 
> > [ Removed the two emails that were bouncing ]
> > 
> > On Wed, 10 Jul 2019 21:06:57 +0200 (CEST)
> > Thomas Gleixner <tglx@linutronix.de> wrote:
> > 
> > > > > > sched_clock_continuous() ? (I know, horrible name), that simply keeps
> > > > > > track of the time delta at suspend and returns:
> > > > > > 
> > > > > > 	sched_clock() + delta;    
> > > > > 
> > > > > Which you get already when you do
> > > > > 
> > > > > # echo boot > /sys/kernel/debug/tracing/trace_clock
> > > > >   
> > > > 
> > > > So basically the answer here is to change printk to use
> > > > ktime_get_boot_fast_ns() instead of local_clock()?  
> > > 
> > > Aargh. That was tracing.
> > > 
> > > There was a patchset floating around which actually implemented that clock
> > > choice for sched_clock as well. Don't know why that was never merged.
> > 
> > Will it cause issues with the scheduler though. If it doesn't stop
> > during suspend, can't that make the scheduler think that processes were
> > using the CPU during the entire suspend and screw up the accounting?
> 
> Duh. My brain is not working.
> 
> Not sched_clock, the patches were for printk so you could select a printk
> clock. Can't find them right now.

Ah, we actually tried to merge them. Here is the full story:

  https://lore.kernel.org/lkml/alpine.DEB.2.20.1711131023170.1851@nanos/

Thanks,

	tglx