All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 01/10] drm/tegra: dc: Use unsigned int for register offsets
@ 2017-08-15 13:41 Thierry Reding
  2017-08-15 13:41 ` [PATCH 02/10] drm/tegra: dpaux: " Thierry Reding
                   ` (3 more replies)
  0 siblings, 4 replies; 16+ messages in thread
From: Thierry Reding @ 2017-08-15 13:41 UTC (permalink / raw)
  To: Thierry Reding; +Cc: linux-tegra, dri-devel, Mikko Perttunen

From: Thierry Reding <treding@nvidia.com>

Register offsets are usually fairly small numbers, so an unsigned int is
more than enough to represent them.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/tegra/dc.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/tegra/dc.h b/drivers/gpu/drm/tegra/dc.h
index 3b42754c5b7a..63461eb769f0 100644
--- a/drivers/gpu/drm/tegra/dc.h
+++ b/drivers/gpu/drm/tegra/dc.h
@@ -70,12 +70,12 @@ static inline struct tegra_dc *to_tegra_dc(struct drm_crtc *crtc)
 }
 
 static inline void tegra_dc_writel(struct tegra_dc *dc, u32 value,
-				   unsigned long offset)
+				   unsigned int offset)
 {
 	writel(value, dc->regs + (offset << 2));
 }
 
-static inline u32 tegra_dc_readl(struct tegra_dc *dc, unsigned long offset)
+static inline u32 tegra_dc_readl(struct tegra_dc *dc, unsigned int offset)
 {
 	return readl(dc->regs + (offset << 2));
 }
-- 
2.13.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 16+ messages in thread
* [PATCH v2] timers: Fix excessive granularity of new timers after a nohz idle
@ 2017-08-22  8:43 Nicholas Piggin
  2017-08-23  3:21 ` [v2] " jeffy
  0 siblings, 1 reply; 16+ messages in thread
From: Nicholas Piggin @ 2017-08-22  8:43 UTC (permalink / raw)
  To: Thomas Gleixner
  Cc: Nicholas Piggin, akpm, John Stultz, Stephen Boyd,
	Jonathan.Cameron, paulmck, mpe, dzickus, sfr, linuxarm, abdhalee,
	linux-kernel, torvalds

When a timer base is idle, it is forwarded when a new timer is added
to ensure that granularity does not become excessive. When not idle,
the timer tick is expected to increment the base.

However there are several problems:

- If an existing timer is modified, the base is forwarded only after
  the index is calculated.

- The base is not forwarded by add_timer_on.

- There is a window after a timer is restarted from a nohz ide, after
  it is marked not-idle and before the timer tick on this CPU, where a
  timer may be added but the ancient base does not get forwarded.

These result in excessive granularity (a 1 jiffy timeout can blow out
to 100s of jiffies), which cause the rcu lockup detector to trigger,
among other things.

Fix this by keeping track of whether the timer base has been idle
since it was last run or forwarded, and if so then forward it before
adding a new timer.

There is still a case where mod_timer optimises the case of a pending
timer mod with the same expiry time, where the timer can see excessive
granularity relative to the new, shorter interval. A comment is added,
but it's not changed because it is an important fastpath for
networking.

This has been tested and found to fix the RCU softlockup messages.

Testing was also done with tracing to measure requested versus
achieved wakeup latencies for all non-deferrable timers in an idle
system (with no lockup watchdogs running). Wakeup latency relative to
absolute latency is calculated (note this suffers from round-up skew
at low absolute times) and analysed:

             max     avg      std
upstream   506.0    1.20     4.68
patched      2.0    1.08     0.15

The bug was noticed due to the lockup detector Kconfig changes
dropping it out of people's .configs and resulting in larger base
clk skew When the lockup detectors are enabled, no CPU can go idle for
longer than 4 seconds, which limits the granularity errors.
Sub-optimal timer behaviour is observable on a smaller scale in that
case:

	     max     avg      std
upstream     9.0    1.05     0.19
patched      2.0    1.04     0.11

Tested-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Tested-by: David Miller <davem@davemloft.net>
Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
---

- Address Thomas' comments (hopefully).
- Reword the changelog a bit.
- Added Jonathan's tested-by

 kernel/time/timer.c | 50 +++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 41 insertions(+), 9 deletions(-)

diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index 8f5d1bf18854..f2674a056c26 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -203,6 +203,7 @@ struct timer_base {
 	bool			migration_enabled;
 	bool			nohz_active;
 	bool			is_idle;
+	bool			must_forward_clk;
 	DECLARE_BITMAP(pending_map, WHEEL_SIZE);
 	struct hlist_head	vectors[WHEEL_SIZE];
 } ____cacheline_aligned;
@@ -856,13 +857,19 @@ get_target_base(struct timer_base *base, unsigned tflags)
 
 static inline void forward_timer_base(struct timer_base *base)
 {
-	unsigned long jnow = READ_ONCE(jiffies);
+	unsigned long jnow;
 
 	/*
-	 * We only forward the base when it's idle and we have a delta between
-	 * base clock and jiffies.
+	 * We only forward the base when we are idle or have just come out of
+	 * idle (must_forward_clk logic), and have a delta between base clock
+	 * and jiffies. In the common case, run_timers will take care of it.
 	 */
-	if (!base->is_idle || (long) (jnow - base->clk) < 2)
+	if (likely(!base->must_forward_clk))
+		return;
+
+	jnow = READ_ONCE(jiffies);
+	base->must_forward_clk = base->is_idle;
+	if ((long)(jnow - base->clk) < 2)
 		return;
 
 	/*
@@ -938,6 +945,11 @@ __mod_timer(struct timer_list *timer, unsigned long expires, bool pending_only)
 	 * same array bucket then just return:
 	 */
 	if (timer_pending(timer)) {
+		/*
+		 * The downside of this optimization is that it can result in
+		 * larger granularity than you would get from adding a new
+		 * timer with this expiry.
+		 */
 		if (timer->expires == expires)
 			return 1;
 
@@ -948,6 +960,7 @@ __mod_timer(struct timer_list *timer, unsigned long expires, bool pending_only)
 		 * dequeue/enqueue dance.
 		 */
 		base = lock_timer_base(timer, &flags);
+		forward_timer_base(base);
 
 		clk = base->clk;
 		idx = calc_wheel_index(expires, clk);
@@ -964,6 +977,7 @@ __mod_timer(struct timer_list *timer, unsigned long expires, bool pending_only)
 		}
 	} else {
 		base = lock_timer_base(timer, &flags);
+		forward_timer_base(base);
 	}
 
 	ret = detach_if_pending(timer, base, false);
@@ -991,12 +1005,10 @@ __mod_timer(struct timer_list *timer, unsigned long expires, bool pending_only)
 			raw_spin_lock(&base->lock);
 			WRITE_ONCE(timer->flags,
 				   (timer->flags & ~TIMER_BASEMASK) | base->cpu);
+			forward_timer_base(base);
 		}
 	}
 
-	/* Try to forward a stale timer base clock */
-	forward_timer_base(base);
-
 	timer->expires = expires;
 	/*
 	 * If 'idx' was calculated above and the base time did not advance
@@ -1112,6 +1124,7 @@ void add_timer_on(struct timer_list *timer, int cpu)
 		WRITE_ONCE(timer->flags,
 			   (timer->flags & ~TIMER_BASEMASK) | cpu);
 	}
+	forward_timer_base(base);
 
 	debug_activate(timer, timer->expires);
 	internal_add_timer(base, timer);
@@ -1497,10 +1510,16 @@ u64 get_next_timer_interrupt(unsigned long basej, u64 basem)
 		if (!is_max_delta)
 			expires = basem + (u64)(nextevt - basej) * TICK_NSEC;
 		/*
-		 * If we expect to sleep more than a tick, mark the base idle:
+		 * If we expect to sleep more than a tick, mark the base idle.
+		 * Also the tick is stopped so any added timer must forward
+		 * the base clk itself to keep granularity small. This idle
+		 * logic is only maintained for the BASE_STD base, deferrable
+		 * timers may still see large granularity skew (by design).
 		 */
-		if ((expires - basem) > TICK_NSEC)
+		if ((expires - basem) > TICK_NSEC) {
+			base->must_forward_clk = true;
 			base->is_idle = true;
+		}
 	}
 	raw_spin_unlock(&base->lock);
 
@@ -1611,6 +1630,19 @@ 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) && base->nohz_active)
 		__run_timers(this_cpu_ptr(&timer_bases[BASE_DEF]));
-- 
2.13.3

^ permalink raw reply related	[flat|nested] 16+ messages in thread
end of thread, other threads:[~2017-08-24  8:36 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-08-15 13:41 [PATCH 01/10] drm/tegra: dc: Use unsigned int for register offsets Thierry Reding
2017-08-15 13:41 ` [PATCH 02/10] drm/tegra: dpaux: " Thierry Reding
2017-08-15 13:41 ` [PATCH 03/10] drm/tegra: dsi: " Thierry Reding
2017-08-15 13:41 ` [PATCH 04/10] drm/tegra: hdmi: " Thierry Reding
     [not found] ` <20170815134114.17547-1-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-08-15 13:41   ` [PATCH 05/10] drm/tegra: sor: " Thierry Reding
2017-08-15 13:41   ` [PATCH 06/10] drm/tegra: dc: Trace register accesses Thierry Reding
     [not found]     ` <20170815134114.17547-6-thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
2017-08-23  8:38       ` [v2] timers: Fix excessive granularity of new timers after a nohz idle jeffy
     [not found]         ` <599D3EF6.9030000-TNX95d0MmH7DzftRWevZcw@public.gmane.org>
2017-08-24  8:36           ` Thierry Reding
2017-08-15 13:41   ` [PATCH 07/10] drm/tegra: hdmi: Trace register accesses Thierry Reding
2017-08-15 13:41   ` [PATCH 08/10] drm/tegra: dsi: " Thierry Reding
2017-08-15 13:41   ` [PATCH 09/10] drm/tegra: dpaux: " Thierry Reding
2017-08-15 13:41   ` [PATCH 10/10] drm/tegra: sor: " Thierry Reding
  -- strict thread matches above, loose matches on Subject: below --
2017-08-22  8:43 [PATCH v2] timers: Fix excessive granularity of new timers after a nohz idle Nicholas Piggin
2017-08-23  3:21 ` [v2] " jeffy
2017-08-23  3:35   ` Stephen Rothwell
2017-08-23  7:01   ` Thomas Gleixner
2017-08-23  7:29     ` jeffy

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.