Re: [BUG, bisect] hrtimer: severe lag after suspend & resume

[John Stultz]

On Wed, Jun 3, 2015 at 5:56 PM, Jeremiah Mahler <jmmahler@xxxxxxxxx> wrote:
> all,
>
> After a fresh boot, the Chrome web browser behaves normally.  Pages
> load quickly and scroll fast.  Even image heavy sites such as
> images.google.com work fine.  However, after a suspend and resume
> cycle, Chrome becomes very slow.  Pages take ten seconds or more to
> load.  The scroll bars and buttons are almost completely
> unresponsive.  Interestingly, I can run Firefox on the same sites
> and it has no issue whatsoever.
>
> I have bisected the kernel and found that the following commit
> introduced the bug.  It is present in the latest linux-next (20150602).
>
>   From 868a3e915f7f5eba8f8cb4f7da2276760807c51c Mon Sep 17 00:00:00 2001
>   From: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>   Date: Tue, 14 Apr 2015 21:08:37 +0000
>   Subject: [PATCH] hrtimer: Make offset update smarter
>
>   On every tick/hrtimer interrupt we update the offset variables of the
>   clock bases. That's silly because these offsets change very seldom.
>
>   Add a sequence counter to the time keeping code which keeps track of
>   the offset updates (clock_was_set()). Have a sequence cache in the
>   hrtimer cpu bases to evaluate whether the offsets must be updated or
>   not. This allows us later to avoid pointless cacheline pollution.
>
>   Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>   Reviewed-by: Preeti U Murthy <preeti@xxxxxxxxxxxxxxxxxx>
>   Acked-by: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
>   Cc: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
>   Cc: Marcelo Tosatti <mtosatti@xxxxxxxxxx>
>   Cc: Frederic Weisbecker <fweisbec@xxxxxxxxx>
>   Cc: John Stultz <john.stultz@xxxxxxxxxx>
>   Link: http://lkml.kernel.org/r/20150414203501.132820245@xxxxxxxxxxxxx
>   Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
>   Cc: John Stultz <john.stultz@xxxxxxxxxx>
>   ---
>    include/linux/hrtimer.h             |  4 ++--
>    include/linux/timekeeper_internal.h |  2 ++
>    kernel/time/hrtimer.c               |  3 ++-
>    kernel/time/timekeeping.c           | 23 ++++++++++++++++-------
>    kernel/time/timekeeping.h           |  7 ++++---
>    5 files changed, 26 insertions(+), 13 deletions(-)


So I suspect the problem is the change to clock_was_set_seq in
timekeeping_update is done prior to mirroring the time state to the
shadow-timekeeper. Thus the next time we do update_wall_time() the
updated sequence is overwritten by whats in the shadow copy. The
attached patch moving the modification up seems to avoid the issue for
me.

Thomas:  Looking at the problematic change, I'm not a big fan of it.
Caching timekeeping state here in the hrtimer code has been a source
of bugs in the past, and I'm not sure I see how avoiding copying
24bytes is that big of a win. Especially since it adds more state to
the timekeeper and hrtimer base that we have to read and mange.
Personally I'd prefer a revert to my fix.

thanks
-john
diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
index 90ed5db..53be796 100644
--- a/kernel/time/timekeeping.c
+++ b/kernel/time/timekeeping.c
@@ -580,6 +580,9 @@ static void timekeeping_update(struct timekeeper *tk, unsigned int action)
 		ntp_clear();
 	}
 
+	if (action & TK_CLOCK_WAS_SET)
+		tk->clock_was_set_seq++;
+
 	tk_update_ktime_data(tk);
 
 	update_vsyscall(tk);
@@ -591,9 +594,6 @@ static void timekeeping_update(struct timekeeper *tk, unsigned int action)
 
 	update_fast_timekeeper(&tk->tkr_mono, &tk_fast_mono);
 	update_fast_timekeeper(&tk->tkr_raw,  &tk_fast_raw);
-
-	if (action & TK_CLOCK_WAS_SET)
-		tk->clock_was_set_seq++;
 }
 
 /**