Re: [PATCH] timers: Fix up get_target_base() to use old base properly

[Thomas Gleixner <tglx@linutronix.de>]

Peter,

On Mon, 3 Jun 2019, Peter Xu wrote:

> get_target_base() in the timer code is not using the "base" parameter
> at all.  My gut feeling is that instead of removing that extra
> parameter, what we really want to do is "return the old base if it
> does not suite for a new one".

Gut feelings are not really useful for technical decisions.

>  kernel/time/timer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/time/timer.c b/kernel/time/timer.c
> index 343c7ba33b1c..6ff6ffd2c719 100644
> --- a/kernel/time/timer.c
> +++ b/kernel/time/timer.c
> @@ -868,7 +868,7 @@ get_target_base(struct timer_base *base, unsigned tflags)
>  	    !(tflags & TIMER_PINNED))
>  		return get_timer_cpu_base(tflags, get_nohz_timer_target());
>  #endif
> -	return get_timer_this_cpu_base(tflags);
> +	return base;
>  }

Timers are supposed to be queued on the local CPU except for the following
cases:

 1) timer migration is enabled and the timer is not pinned

    In this case the get_nohz_timer_target() crystal ball logic tries to
    find a useful base for the timer, which is doomed but its that way
    until we reach the point where the pull model actually works.

 2) add_timer_on() is invoked which puts a timer on an explicit target
    CPU. That's not using the above.

That has been that way forever and the base parameter is stale as older
code used the base to check whether migration is enabled or not. When this
was converted to a static branch the parameter stayed and got unused.

So your change would prevent moving the timer to the current CPU.

You might argue that in case of an explicit pinned timer, the above logic
is wrong when the timer is modified as it might move to a different
CPU. But from day one when the pinned logic was introduced, pinned just
prevents it from being queued on a remote CPU. If you need a timer to stay
on a particular CPU even if modified from a remote CPU, then the only way
right now is to dequeue and requeue it with add_timer_on(). 

If we really want to change that, then we need to audit all usage sites of
pinned timers and figure out whether this would break anything.

The proper change would be in that case:

      return pinned ? base : get_timer_this_cpu_base(tflags);

But unless you can come up with a use case where the current logic is truly
broken, I don't see a reason to do that.

Thanks,

	tglx