* [PATCH] timers: Fix up get_target_base() to use old base properly @ 2019-06-03 13:29 Peter Xu 2019-06-04 5:42 ` Peter Xu ` (3 more replies) 0 siblings, 4 replies; 9+ messages in thread From: Peter Xu @ 2019-06-03 13:29 UTC (permalink / raw) To: linux-kernel Cc: peterx, Thomas Gleixner, John Stultz, Stephen Boyd, Luiz Capitulino, Marcelo Tosatti 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". CC: Thomas Gleixner <tglx@linutronix.de> CC: John Stultz <john.stultz@linaro.org> CC: Stephen Boyd <sboyd@kernel.org> CC: Luiz Capitulino <lcapitulino@redhat.com> CC: Marcelo Tosatti <mtosatti@redhat.com> CC: linux-kernel@vger.kernel.org Signed-off-by: Peter Xu <peterx@redhat.com> --- 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; } static inline void forward_timer_base(struct timer_base *base) -- 2.17.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] timers: Fix up get_target_base() to use old base properly 2019-06-03 13:29 [PATCH] timers: Fix up get_target_base() to use old base properly Peter Xu @ 2019-06-04 5:42 ` Peter Xu 2019-06-06 15:28 ` Marcelo Tosatti ` (2 subsequent siblings) 3 siblings, 0 replies; 9+ messages in thread From: Peter Xu @ 2019-06-04 5:42 UTC (permalink / raw) To: linux-kernel Cc: Thomas Gleixner, John Stultz, Stephen Boyd, Luiz Capitulino, Marcelo Tosatti On Mon, Jun 03, 2019 at 09:29:44PM +0800, 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". I'm trying to think of a detailed scenario of this patch: 1. setup a timer T1 with TIMER_PINNED on cpu 3 and arm it 2. on another cpu (e.g., cpu 4), call mod_timer() upon T1 before the timer fires itself 2.1. in __mod_timer(), lock_timer_base() will return cpu 3's timer base because it was pinned with cpu 3 2.2. in the same __mod_timer(), get_target_base() will return cpu 4's timer base if without this patch, and will return cpu 3's timer base if with this patch I don't know whether step 2 is easy to happen but I don't see why it was forbidden so I'm assuming it could still happen... Then IMHO if without this patch, the timer T1 will be queued on cpu 4's timer base rather than cpu 3's, which seems to break TIMER_PINNED. And just in case if this patch makes sense - get_timer_this_cpu_base() can be dropped together since not used any more. -- Peter Xu ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] timers: Fix up get_target_base() to use old base properly 2019-06-03 13:29 [PATCH] timers: Fix up get_target_base() to use old base properly Peter Xu 2019-06-04 5:42 ` Peter Xu @ 2019-06-06 15:28 ` Marcelo Tosatti 2019-06-10 1:05 ` Peter Xu 2019-06-17 4:09 ` Peter Xu 2019-06-17 6:09 ` Thomas Gleixner 3 siblings, 1 reply; 9+ messages in thread From: Marcelo Tosatti @ 2019-06-06 15:28 UTC (permalink / raw) To: Peter Xu Cc: linux-kernel, Thomas Gleixner, John Stultz, Stephen Boyd, Luiz Capitulino On Mon, Jun 03, 2019 at 09:29:44PM +0800, 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". Hi Peter, I think its a dead parameter: you always want to use the local base if the timer is not pinned. > CC: Thomas Gleixner <tglx@linutronix.de> > CC: John Stultz <john.stultz@linaro.org> > CC: Stephen Boyd <sboyd@kernel.org> > CC: Luiz Capitulino <lcapitulino@redhat.com> > CC: Marcelo Tosatti <mtosatti@redhat.com> > CC: linux-kernel@vger.kernel.org > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > 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; > } > > static inline void forward_timer_base(struct timer_base *base) > -- > 2.17.1 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] timers: Fix up get_target_base() to use old base properly 2019-06-06 15:28 ` Marcelo Tosatti @ 2019-06-10 1:05 ` Peter Xu 0 siblings, 0 replies; 9+ messages in thread From: Peter Xu @ 2019-06-10 1:05 UTC (permalink / raw) To: Marcelo Tosatti Cc: linux-kernel, Thomas Gleixner, John Stultz, Stephen Boyd, Luiz Capitulino On Thu, Jun 06, 2019 at 12:28:08PM -0300, Marcelo Tosatti wrote: > On Mon, Jun 03, 2019 at 09:29:44PM +0800, 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". > > Hi Peter, Hi, Marcelo, > > I think its a dead parameter: you always want to use the local base > if the timer is not pinned. Thanks for the comment. But what if it was pinned? Could the old code always do right even in the scenario I mentioned below? https://lkml.org/lkml/2019/6/4/34 Also note that if the timer was not pinned, IMHO it'll go into the check below and it seems to likely to have another timer base instead of the local base if it's not a housekeeping cpu (because get_nohz_timer_target() will always try to provision timers onto the housekeeping ones), or did I misunderstand your comment? #if defined(CONFIG_SMP) && defined(CONFIG_NO_HZ_COMMON) if (static_branch_likely(&timers_migration_enabled) && !(tflags & TIMER_PINNED)) return get_timer_cpu_base(tflags, get_nohz_timer_target()); #endif Regards, -- Peter Xu ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] timers: Fix up get_target_base() to use old base properly 2019-06-03 13:29 [PATCH] timers: Fix up get_target_base() to use old base properly Peter Xu 2019-06-04 5:42 ` Peter Xu 2019-06-06 15:28 ` Marcelo Tosatti @ 2019-06-17 4:09 ` Peter Xu 2019-06-17 6:09 ` Thomas Gleixner 3 siblings, 0 replies; 9+ messages in thread From: Peter Xu @ 2019-06-17 4:09 UTC (permalink / raw) To: linux-kernel Cc: Thomas Gleixner, John Stultz, Stephen Boyd, Luiz Capitulino, Marcelo Tosatti On Mon, Jun 03, 2019 at 09:29:44PM +0800, 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". Ping? Note that as reference hrtimer is using the same logic as in this patch. > > CC: Thomas Gleixner <tglx@linutronix.de> > CC: John Stultz <john.stultz@linaro.org> > CC: Stephen Boyd <sboyd@kernel.org> > CC: Luiz Capitulino <lcapitulino@redhat.com> > CC: Marcelo Tosatti <mtosatti@redhat.com> > CC: linux-kernel@vger.kernel.org > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > 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; > } > > static inline void forward_timer_base(struct timer_base *base) > -- > 2.17.1 > Regards, -- Peter Xu ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] timers: Fix up get_target_base() to use old base properly 2019-06-03 13:29 [PATCH] timers: Fix up get_target_base() to use old base properly Peter Xu ` (2 preceding siblings ...) 2019-06-17 4:09 ` Peter Xu @ 2019-06-17 6:09 ` Thomas Gleixner 2019-06-17 9:11 ` Peter Xu 3 siblings, 1 reply; 9+ messages in thread From: Thomas Gleixner @ 2019-06-17 6:09 UTC (permalink / raw) To: Peter Xu Cc: linux-kernel, John Stultz, Stephen Boyd, Luiz Capitulino, Marcelo Tosatti 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] timers: Fix up get_target_base() to use old base properly 2019-06-17 6:09 ` Thomas Gleixner @ 2019-06-17 9:11 ` Peter Xu 2019-06-17 12:07 ` Thomas Gleixner 0 siblings, 1 reply; 9+ messages in thread From: Peter Xu @ 2019-06-17 9:11 UTC (permalink / raw) To: Thomas Gleixner Cc: linux-kernel, John Stultz, Stephen Boyd, Luiz Capitulino, Marcelo Tosatti On Mon, Jun 17, 2019 at 08:09:20AM +0200, Thomas Gleixner wrote: > Peter, Hi, Thomas, Thanks for replying. > > 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(). Indeed. If add_timer_on() should always be used when with pinned timers, IMHO it would be good to comment probably above TIMER_PINNED about the fact so people will never misuse the interfaces (it seems to be mis-used somehow but I cannot be 100% sure, please see below). > > 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); Purely for curiousity - why would we like to use current cpu base even if it's unpinned? My humble opinion is that if we use base directly at least we can avoid potential migration of the timer. But I can be missing some real reason here... > > 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. It's not a lot of places that used TIMER_PINNED so I gave a quick look on it. Two conditions are always correct: - when add_timer_on is used as explained, or, - when timer_setup(..., TIMER_PINNED) is used in the same context as timer_mod() (e.g., mod_timer is called right after timer_setup). For above two cases, the timer will always be running with expected CPU that was pinned. Though, I see two outliers: ====================== *** drivers/cpufreq/powernv-cpufreq.c: powernv_cpufreq_cpu_init[867] TIMER_PINNED | TIMER_DEFERRABLE); *** net/ipv4/inet_timewait_sock.c: inet_twsk_alloc[189] timer_setup(&tw->tw_timer, tw_timer_handler, TIMER_PINNED); ====================== For the first one I can't say it's problematic since even if it's not pinned correctly it has the logic to re-pin the timer so it'll finally be fine: void gpstate_timer_handler(struct timer_list *t) { ... /* * If the timer has migrated to the different cpu then bring * it back to one of the policy->cpus */ if (!cpumask_test_cpu(raw_smp_processor_id(), policy->cpus)) { gpstates->timer.expires = jiffies + msecs_to_jiffies(1); add_timer_on(&gpstates->timer, cpumask_first(policy->cpus)); spin_unlock(&gpstates->gpstate_lock); return; } ... } For the other one, the timer is setup during inet_twsk_alloc(), but the mod_timer() is in __inet_twsk_schedule() which seems to be problematic. Thanks, -- Peter Xu ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] timers: Fix up get_target_base() to use old base properly 2019-06-17 9:11 ` Peter Xu @ 2019-06-17 12:07 ` Thomas Gleixner 2019-06-18 0:41 ` Peter Xu 0 siblings, 1 reply; 9+ messages in thread From: Thomas Gleixner @ 2019-06-17 12:07 UTC (permalink / raw) To: Peter Xu Cc: linux-kernel, John Stultz, Stephen Boyd, Luiz Capitulino, Marcelo Tosatti Peter, On Mon, 17 Jun 2019, Peter Xu wrote: > On Mon, Jun 17, 2019 at 08:09:20AM +0200, Thomas Gleixner wrote: > > 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(). > > Indeed. If add_timer_on() should always be used when with pinned > timers, IMHO it would be good to comment probably above TIMER_PINNED > about the fact so people will never misuse the interfaces (it seems to > be mis-used somehow but I cannot be 100% sure, please see below). Yeah, some documentation would be good. > > 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); > > Purely for curiousity - why would we like to use current cpu base even > if it's unpinned? My humble opinion is that if we use base directly > at least we can avoid potential migration of the timer. But I can be > missing some real reason here... In most cases it's desired to move the timer over. Assume you have a network interrupt moving from one cpu to the other and then the tcp timers would stay on the old cpu forever. So you'd pay the remote access price every time you touch it and if it fires the callback is pretty much guaranteed to be cache cold. > Though, I see two outliers: > > ====================== > > *** drivers/cpufreq/powernv-cpufreq.c: > powernv_cpufreq_cpu_init[867] TIMER_PINNED | TIMER_DEFERRABLE); > > *** net/ipv4/inet_timewait_sock.c: > inet_twsk_alloc[189] timer_setup(&tw->tw_timer, tw_timer_handler, TIMER_PINNED); That's fine. It just wants to make sure that the timer is not queued on a remote CPU if NOHZ is active. That gives them a serialization guarantee of the network softirq vs. the timer softirq so they can spare some locking stuff. Thanks, tglx ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] timers: Fix up get_target_base() to use old base properly 2019-06-17 12:07 ` Thomas Gleixner @ 2019-06-18 0:41 ` Peter Xu 0 siblings, 0 replies; 9+ messages in thread From: Peter Xu @ 2019-06-18 0:41 UTC (permalink / raw) To: Thomas Gleixner Cc: linux-kernel, John Stultz, Stephen Boyd, Luiz Capitulino, Marcelo Tosatti On Mon, Jun 17, 2019 at 02:07:48PM +0200, Thomas Gleixner wrote: > Peter, > > On Mon, 17 Jun 2019, Peter Xu wrote: > > On Mon, Jun 17, 2019 at 08:09:20AM +0200, Thomas Gleixner wrote: > > > 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(). > > > > Indeed. If add_timer_on() should always be used when with pinned > > timers, IMHO it would be good to comment probably above TIMER_PINNED > > about the fact so people will never misuse the interfaces (it seems to > > be mis-used somehow but I cannot be 100% sure, please see below). > > Yeah, some documentation would be good. > > > > 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); > > > > Purely for curiousity - why would we like to use current cpu base even > > if it's unpinned? My humble opinion is that if we use base directly > > at least we can avoid potential migration of the timer. But I can be > > missing some real reason here... > > In most cases it's desired to move the timer over. Assume you have a > network interrupt moving from one cpu to the other and then the tcp timers > would stay on the old cpu forever. So you'd pay the remote access price > every time you touch it and if it fires the callback is pretty much > guaranteed to be cache cold. I see. > > > Though, I see two outliers: > > > > ====================== > > > > *** drivers/cpufreq/powernv-cpufreq.c: > > powernv_cpufreq_cpu_init[867] TIMER_PINNED | TIMER_DEFERRABLE); > > > > *** net/ipv4/inet_timewait_sock.c: > > inet_twsk_alloc[189] timer_setup(&tw->tw_timer, tw_timer_handler, TIMER_PINNED); > > That's fine. It just wants to make sure that the timer is not queued on a > remote CPU if NOHZ is active. That gives them a serialization guarantee of > the network softirq vs. the timer softirq so they can spare some locking > stuff. Thanks for the analysis. Instead of this patch, let me post a documentation update for pinned timers. Thanks, -- Peter Xu ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2019-06-18 0:42 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-03 13:29 [PATCH] timers: Fix up get_target_base() to use old base properly Peter Xu 2019-06-04 5:42 ` Peter Xu 2019-06-06 15:28 ` Marcelo Tosatti 2019-06-10 1:05 ` Peter Xu 2019-06-17 4:09 ` Peter Xu 2019-06-17 6:09 ` Thomas Gleixner 2019-06-17 9:11 ` Peter Xu 2019-06-17 12:07 ` Thomas Gleixner 2019-06-18 0:41 ` Peter Xu
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.