* [PATCH] timers: Clear must_forward_clk inside base lock
@ 2018-07-26 8:42 Gaurav Kohli
2018-07-31 4:29 ` Kohli, Gaurav
2018-07-31 15:41 ` Thomas Gleixner
0 siblings, 2 replies; 4+ messages in thread
From: Gaurav Kohli @ 2018-07-26 8:42 UTC (permalink / raw)
To: john.stultz, tglx, sboyd; +Cc: linux-kernel, linux-arm-msm, Gaurav Kohli
While migrating timer to new base, there is a need
to update base clk by calling forward_timer_base to
avoid stale clock , but at the same time if run_timer
is exectuing in new core it may set must_forward_clk
to false and due to this forward base logic may fail as
per below check:
if (likely(!base->must_forward_clk))
return;
So preventing the same by putting must_forward_clk inside
base lock.
Signed-off-by: Gaurav Kohli <gkohli@codeaurora.org>
diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index cc2d23e..675241d 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -1657,6 +1657,19 @@ static inline void __run_timers(struct timer_base *base)
raw_spin_lock_irq(&base->lock);
+ /*
+ * must_forward_clk must be cleared before running timers so that any
+ * timer functions that call mod_timer will not try to forward the
+ * base. idle trcking / clock forwarding logic is only used with
+ * BASE_STD timers.
+ *
+ * The deferrable base does not do idle tracking at all, so we do
+ * not forward it. This can result in very large variations in
+ * granularity for deferrable timers, but they can be deferred for
+ * long periods due to idle.
+ */
+ base->must_forward_clk = false;
+
while (time_after_eq(jiffies, base->clk)) {
levels = collect_expired_timers(base, heads);
@@ -1676,19 +1689,6 @@ static __latent_entropy void run_timer_softirq(struct softirq_action *h)
{
struct timer_base *base = this_cpu_ptr(&timer_bases[BASE_STD]);
- /*
- * must_forward_clk must be cleared before running timers so that any
- * timer functions that call mod_timer will not try to forward the
- * base. idle trcking / clock forwarding logic is only used with
- * BASE_STD timers.
- *
- * The deferrable base does not do idle tracking at all, so we do
- * not forward it. This can result in very large variations in
- * granularity for deferrable timers, but they can be deferred for
- * long periods due to idle.
- */
- base->must_forward_clk = false;
-
__run_timers(base);
if (IS_ENABLED(CONFIG_NO_HZ_COMMON))
__run_timers(this_cpu_ptr(&timer_bases[BASE_DEF]));
--
1.9.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] timers: Clear must_forward_clk inside base lock
2018-07-26 8:42 [PATCH] timers: Clear must_forward_clk inside base lock Gaurav Kohli
@ 2018-07-31 4:29 ` Kohli, Gaurav
2018-07-31 14:48 ` Thomas Gleixner
2018-07-31 15:41 ` Thomas Gleixner
1 sibling, 1 reply; 4+ messages in thread
From: Kohli, Gaurav @ 2018-07-31 4:29 UTC (permalink / raw)
To: john.stultz, tglx, sboyd; +Cc: linux-kernel, linux-arm-msm
Hi John, Thomas,
Can you please review below patch and update your comments:
Regards
Gaurav
On 7/26/2018 2:12 PM, Gaurav Kohli wrote:
> While migrating timer to new base, there is a need
> to update base clk by calling forward_timer_base to
> avoid stale clock , but at the same time if run_timer
> is exectuing in new core it may set must_forward_clk
> to false and due to this forward base logic may fail as
> per below check:
>
> if (likely(!base->must_forward_clk))
> return;
>
> So preventing the same by putting must_forward_clk inside
> base lock.
>
> Signed-off-by: Gaurav Kohli <gkohli@codeaurora.org>
>
> diff --git a/kernel/time/timer.c b/kernel/time/timer.c
> index cc2d23e..675241d 100644
> --- a/kernel/time/timer.c
> +++ b/kernel/time/timer.c
> @@ -1657,6 +1657,19 @@ static inline void __run_timers(struct timer_base *base)
>
> raw_spin_lock_irq(&base->lock);
>
> + /*
> + * must_forward_clk must be cleared before running timers so that any
> + * timer functions that call mod_timer will not try to forward the
> + * base. idle trcking / clock forwarding logic is only used with
> + * BASE_STD timers.
> + *
> + * The deferrable base does not do idle tracking at all, so we do
> + * not forward it. This can result in very large variations in
> + * granularity for deferrable timers, but they can be deferred for
> + * long periods due to idle.
> + */
> + base->must_forward_clk = false;
> +
> while (time_after_eq(jiffies, base->clk)) {
>
> levels = collect_expired_timers(base, heads);
> @@ -1676,19 +1689,6 @@ static __latent_entropy void run_timer_softirq(struct softirq_action *h)
> {
> struct timer_base *base = this_cpu_ptr(&timer_bases[BASE_STD]);
>
> - /*
> - * must_forward_clk must be cleared before running timers so that any
> - * timer functions that call mod_timer will not try to forward the
> - * base. idle trcking / clock forwarding logic is only used with
> - * BASE_STD timers.
> - *
> - * The deferrable base does not do idle tracking at all, so we do
> - * not forward it. This can result in very large variations in
> - * granularity for deferrable timers, but they can be deferred for
> - * long periods due to idle.
> - */
> - base->must_forward_clk = false;
> -
> __run_timers(base);
> if (IS_ENABLED(CONFIG_NO_HZ_COMMON))
> __run_timers(this_cpu_ptr(&timer_bases[BASE_DEF]));
>
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] timers: Clear must_forward_clk inside base lock
2018-07-31 4:29 ` Kohli, Gaurav
@ 2018-07-31 14:48 ` Thomas Gleixner
0 siblings, 0 replies; 4+ messages in thread
From: Thomas Gleixner @ 2018-07-31 14:48 UTC (permalink / raw)
To: Kohli, Gaurav; +Cc: john.stultz, sboyd, linux-kernel, linux-arm-msm
Gaurav,
On Tue, 31 Jul 2018, Kohli, Gaurav wrote:
> Can you please review below patch and update your comments:
It's on the todo list already. We're not machines and a week time before
sending a reminder is really appropriate.
Thanks,
tglx
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] timers: Clear must_forward_clk inside base lock
2018-07-26 8:42 [PATCH] timers: Clear must_forward_clk inside base lock Gaurav Kohli
2018-07-31 4:29 ` Kohli, Gaurav
@ 2018-07-31 15:41 ` Thomas Gleixner
1 sibling, 0 replies; 4+ messages in thread
From: Thomas Gleixner @ 2018-07-31 15:41 UTC (permalink / raw)
To: Gaurav Kohli; +Cc: John Stultz, sboyd, Anna-Maria Gleixner, LKML, linux-arm-msm
Gaurav,
On Thu, 26 Jul 2018, Gaurav Kohli wrote:
> While migrating timer to new base, there is a need
> to update base clk by calling forward_timer_base to
> avoid stale clock , but at the same time if run_timer
> is exectuing in new core it may set must_forward_clk
> to false and due to this forward base logic may fail as
> per below check:
>
> if (likely(!base->must_forward_clk))
> return;
After twisting my brain for a while I can understand what you are trying to
say, but please look at your own sentence once again. One sentence spawning
6 lines with a really convoluted structure and then you spend 3 lines to
copy a code snippet which is really not helpful.
Please try to structure the description and use a simple table to show the
race, e.g.:
base->must_forward_clock is indicating that the base clock might be stale
due to a long idle sleep. The forwarding takes either place in the timer
softirq or when a timer is enqueued while the base is idle. If the
enqueue to an idle base happens from a remote CPU then the following race
can happen:
CPU0 CPU1
run_timer_softirq() mod_timer(timer)
base->must_forward_clk = false; base = lock_base(timer);
__run_timers(base) if (base->must_forward_clk)
forward(base);
lock(base->lock);
queue_timer(base, timer);
^^^ Based on stale base->clk
unlock(base);
forward(base);
The root cause is that base->must_forward_clk is cleared outside the
base->lock held region, so the remote queueing CPU observes it as
cleared, but the base clock is still stale. This can cause large
granularity values for timers, i.e. the accuracy of the expiry time
suffers.
Prevent this by clearing the flag with base->lock held, so that the
forwarding takes place before the cleared flag is observable by a remote
CPU.
Can you see the difference?
> raw_spin_lock_irq(&base->lock);
>
> + /*
> + * must_forward_clk must be cleared before running timers so that any
> + * timer functions that call mod_timer will not try to forward the
> + * base. idle trcking / clock forwarding logic is only used with
I know that the 'trcking' typo was in the original comment, but it does not
make anything better if you just blindly move it.
> + * BASE_STD timers.
> + *
> + * The deferrable base does not do idle tracking at all, so we do
> + * not forward it. This can result in very large variations in
> + * granularity for deferrable timers, but they can be deferred for
> + * long periods due to idle.
This part of the comment is still correct, but now it's also confusing
because the flag is cleared for _ALL_ bases and not only for BASE_STD. So
at least you want to change that to something like this:
* The must_forward_clk flag is cleared unconditionally also for
* the deferrable base. The deferrable base is not affected by idle
* tracking and never forwarded, so clearing the flag is a NOOP.
*
* The fact that the deferrable base is never forwarded can cause
* large varations in granularity for deferrable timers, but they
* can be deferred for long periods due to idle anyway.
See? If you move a comment you really have to think about whether it is
still correct. If not, then you have to adjust it so it makes sense and not
just move it blindly around and be done with it. Think about yourself
looking at that code in a year from now when you forgot all the gory
details already.
Thanks,
tglx
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-07-31 15:41 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-26 8:42 [PATCH] timers: Clear must_forward_clk inside base lock Gaurav Kohli
2018-07-31 4:29 ` Kohli, Gaurav
2018-07-31 14:48 ` Thomas Gleixner
2018-07-31 15:41 ` Thomas Gleixner
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).