Re: [PATCH 1/2] Dynamic Tick: Prevent clocksource wrapping during idle

[Thomas Gleixner <tglx@linutronix.de>]

On Thu, 28 May 2009, Jon Hunter wrote:
 
> Thomas Gleixner wrote:
> >   Please make this a real function. There is no reason to stick this
> >   into a header file. The only user is clocksource.c anyway, so please
> >   put it there as a static function and let the compiler decide what
> >   to do with it.
> 
> No problem. Please see below. Let me know if this is ok and there is anything
> else.

  Looks good now.
 
>  /**
> + * timekeeping_max_deferment - Returns max time the clocksource can be
> deferred
> + *
> + * IMPORTANT: Caller must observe xtime_lock via read_seqbegin/read_seqretry
> + * to ensure that the clocksource does not change!
> + */

  Just nitpicking here. For the intended use case this is irrelevant.
  
  On UP this is called from an irq disabled section, so nothing is
  going to change the clock source.

  On SMP it does not matter if CPU A goes to sleep with the old clock
  source and CPU B changes the clock source while A is idle. When B
  goes idle it will take the change into account.

  But that leads me to an interesting observation:

  On SMP we really should only care for the CPU which has the do_timer
  duty assigned. All other CPUs can sleep as long as they want. When
  that CPU goes idle and drops the do_timer duty it needs to look at
  max_deferement, but the others can sleep as long as they want.

  So the rule would be:

  if (cpu == tick_do_timer_cpu || tick_do_timer_cpu == TICK_DO_TIMER_NONE)
     check_max_deferment();
  else
     sleep_as_long_as_you_want;

  Could you add that perhaps ?

> +s64 timekeeping_max_deferment(void)
> +{
> +	return clock->max_idle_ns;
> +}
> +

Thanks for your patience,

	tglx