sched_clock -> check_tsc_unstable -> tsc_read_c3_time ?!?

sched_clock -> check_tsc_unstable -> tsc_read_c3_time ?!?

[Lee Revell]

Looking at the latency traces it appears that sched_clock could be
optimized a bit:

evolutio-16296 0D.h4   32us : activate_task (try_to_wake_up)
evolutio-16296 0D.h4   33us : sched_clock (activate_task)
evolutio-16296 0D.h4   33us : check_tsc_unstable (sched_clock)
evolutio-16296 0D.h4   34us : tsc_read_c3_time (sched_clock)
evolutio-16296 0D.h4   35us : recalc_task_prio (activate_task)

check_tsc_unstable and tsc_read_c3_time appear to be new.  Here they
are:

     49 /* Code to mark and check if the TSC is unstable
     50  * due to cpufreq or due to unsynced TSCs
     51  */
     52 static int tsc_unstable;
     53 int check_tsc_unstable(void)
     54 {
     55         return tsc_unstable;
     56 }

     73 u64 tsc_read_c3_time(void)
     74 {
     75         return tsc_c3_offset;
     76 }

Shouldn't these be inlined or something?  I know it's only a few
microseconds, but it seems like excessive function call overhead to me.
I don't use power management and the TSC is stable on this machine.  Why
do we have to call these simple accessor functions over and over?

Lee

Re: sched_clock -> check_tsc_unstable -> tsc_read_c3_time ?!?

[john stultz]

On Thu, 2005-10-13 at 16:01 -0400, Lee Revell wrote:
> Looking at the latency traces it appears that sched_clock could be
> optimized a bit:
> 
> evolutio-16296 0D.h4   32us : activate_task (try_to_wake_up)
> evolutio-16296 0D.h4   33us : sched_clock (activate_task)
> evolutio-16296 0D.h4   33us : check_tsc_unstable (sched_clock)
> evolutio-16296 0D.h4   34us : tsc_read_c3_time (sched_clock)
> evolutio-16296 0D.h4   35us : recalc_task_prio (activate_task)
> 
> check_tsc_unstable and tsc_read_c3_time appear to be new.  Here they
> are:
> 
>      49 /* Code to mark and check if the TSC is unstable
>      50  * due to cpufreq or due to unsynced TSCs
>      51  */
>      52 static int tsc_unstable;
>      53 int check_tsc_unstable(void)
>      54 {
>      55         return tsc_unstable;
>      56 }
> 
>      73 u64 tsc_read_c3_time(void)
>      74 {
>      75         return tsc_c3_offset;
>      76 }
> 
> Shouldn't these be inlined or something?  I know it's only a few
> microseconds, but it seems like excessive function call overhead to me.

Yea, you're right about the inlining. Although I'm not sure why those
functions should take microseconds to execute. That's very strange.

> I don't use power management and the TSC is stable on this machine.  Why
> do we have to call these simple accessor functions over and over?

Basically its there to detect if the cpu goes into a low power mode and
halts the TSC. 

They probably could be #defined if C3 really cannot triggered, but some
check will be necessary otherwise.

The arch specific bits in my patches have gotten less of my attention
recently, but hopefully with the arch generic code getting pretty solid
and fast I'll be back to focusing on it.

thanks
-john

Re: sched_clock -> check_tsc_unstable -> tsc_read_c3_time ?!?

[Lee Revell]

On Thu, 2005-10-13 at 13:11 -0700, john stultz wrote:
> Yea, you're right about the inlining. Although I'm not sure why those
> functions should take microseconds to execute. That's very strange. 

Latency tracing overhead plus a slow (600MHz) machine?

Lee

Re: sched_clock -> check_tsc_unstable -> tsc_read_c3_time ?!?

[Ingo Molnar]

* Lee Revell <rlrevell@joe-job.com> wrote:

> On Thu, 2005-10-13 at 13:11 -0700, john stultz wrote:
> > Yea, you're right about the inlining. Although I'm not sure why those
> > functions should take microseconds to execute. That's very strange. 
> 
> Latency tracing overhead plus a slow (600MHz) machine?

yeah. The micro-timings of latency tracing can be misleading. Function 
calls are very fast on most CPUs (even on a 600MHz one), but with the 
latency tracer generating one trace entry per function call, there's 
considerable added overhead.

we could in theory calibrate the tracing overhead and subtract it from 
cycle readings [i've done this in a previous mcount() based tracer 
implementation, years ago], but that would make the latency trace 
timestamps less useful as a global time reference.

	Ingo