sched clock: revert various sched_clock() changes

Found an interactivity problem on a quad core test-system - simple CPU loops would occasionally delay the system un an unacceptable way. After much debugging with Peter Zijlstra it turned out that the problem is caused by the string of sched_clock() changes - they caused the CPU clock to jump backwards a bit - which confuses the scheduler arithmetics. (which is unsigned for performance reasons) So revert: # c300ba2: sched_clock: and multiplier for TSC to gtod drift # c0c8773: sched_clock: only update deltas with local reads. # af52a90: sched_clock: stop maximum check on NO HZ # f7cce27: sched_clock: widen the max and min time This solves the interactivity problems. Signed-off-by: Ingo Molnar <mingo@elte.hu> Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl> Acked-by: Mike Galbraith <efault@gmx.de>

sched clock: revert various sched_clock() changes
Found an interactivity problem on a quad core test-system - simple CPU loops would occasionally delay the system un an unacceptable way. After much debugging with Peter Zijlstra it turned out that the problem is caused by the string of sched_clock() changes - they caused the CPU clock to jump backwards a bit - which confuses the scheduler arithmetics. (which is unsigned for performance reasons) So revert: # c300ba2: sched_clock: and multiplier for TSC to gtod drift # c0c8773: sched_clock: only update deltas with local reads. # af52a90: sched_clock: stop maximum check on NO HZ # f7cce27: sched_clock: widen the max and min time This solves the interactivity problems. Signed-off-by: Ingo Molnar <mingo@elte.hu> Acked-by: Peter Zijlstra <a.p.zijlstra@chello.nl> Acked-by: Mike Galbraith <efault@gmx.de>
Ingo Molnar
1 parent 39675e89fb
Showing 3 changed files with 14 additions and 114 deletions Side-by-side Diff
include/linux/sched.h
kernel/sched_clock.c
kernel/time/tick-sched.c
@@ -1572,28 +1572,13 @@
 static inline void sched_clock_idle_wakeup_event(u64 delta_ns)
 {
 }
-
-#ifdef CONFIG_NO_HZ
-static inline void sched_clock_tick_stop(int cpu)
-{
-}
-
-static inline void sched_clock_tick_start(int cpu)
-{
-}
-#endif
-
-#else /* CONFIG_HAVE_UNSTABLE_SCHED_CLOCK */
+#else
 extern void sched_clock_init(void);
 extern u64 sched_clock_cpu(int cpu);
 extern void sched_clock_tick(void);
 extern void sched_clock_idle_sleep_event(void);
 extern void sched_clock_idle_wakeup_event(u64 delta_ns);
-#ifdef CONFIG_NO_HZ
-extern void sched_clock_tick_stop(int cpu);
-extern void sched_clock_tick_start(int cpu);
 #endif
-#endif /* CONFIG_HAVE_UNSTABLE_SCHED_CLOCK */
  
 /*
  * For kernel-internal use: high-speed (but slightly incorrect) per-cpu
@@ -44,11 +44,6 @@
  
 #ifdef CONFIG_HAVE_UNSTABLE_SCHED_CLOCK
  
-#define MULTI_SHIFT 15
-/* Max is double, Min is 1/2 */
-#define MAX_MULTI (2LL << MULTI_SHIFT)
-#define MIN_MULTI (1LL << (MULTI_SHIFT-1))
-
 struct sched_clock_data {
 	/*
 	 * Raw spinlock - this is a special case: this might be called
@@ -62,10 +57,6 @@
 	u64			tick_raw;
 	u64			tick_gtod;
 	u64			clock;
-	s64			multi;
-#ifdef CONFIG_NO_HZ
-	int			check_max;
-#endif
 };
  
 static DEFINE_PER_CPU_SHARED_ALIGNED(struct sched_clock_data, sched_clock_data);
  
  
  
@@ -97,53 +88,18 @@
 		scd->tick_raw = 0;
 		scd->tick_gtod = ktime_now;
 		scd->clock = ktime_now;
-		scd->multi = 1 << MULTI_SHIFT;
-#ifdef CONFIG_NO_HZ
-		scd->check_max = 1;
-#endif
 	}
  
 	sched_clock_running = 1;
 }
  
-#ifdef CONFIG_NO_HZ
 /*
- * The dynamic ticks makes the delta jiffies inaccurate. This
- * prevents us from checking the maximum time update.
- * Disable the maximum check during stopped ticks.
- */
-void sched_clock_tick_stop(int cpu)
-{
-	struct sched_clock_data *scd = cpu_sdc(cpu);
-
-	scd->check_max = 0;
-}
-
-void sched_clock_tick_start(int cpu)
-{
-	struct sched_clock_data *scd = cpu_sdc(cpu);
-
-	scd->check_max = 1;
-}
-
-static int check_max(struct sched_clock_data *scd)
-{
-	return scd->check_max;
-}
-#else
-static int check_max(struct sched_clock_data *scd)
-{
-	return 1;
-}
-#endif /* CONFIG_NO_HZ */
-
-/*
  * update the percpu scd from the raw @now value
  *
  *  - filter out backward motion
  *  - use jiffies to generate a min,max window to clip the raw values
  */
-static void __update_sched_clock(struct sched_clock_data *scd, u64 now, u64 *time)
+static void __update_sched_clock(struct sched_clock_data *scd, u64 now)
 {
 	unsigned long now_jiffies = jiffies;
 	long delta_jiffies = now_jiffies - scd->tick_jiffies;
  
  
  
@@ -152,31 +108,16 @@
 	s64 delta = now - scd->prev_raw;
  
 	WARN_ON_ONCE(!irqs_disabled());
+	min_clock = scd->tick_gtod + delta_jiffies * TICK_NSEC;
  
-	/*
-	 * At schedule tick the clock can be just under the gtod. We don't
-	 * want to push it too prematurely.
-	 */
-	min_clock = scd->tick_gtod + (delta_jiffies * TICK_NSEC);
-	if (min_clock > TICK_NSEC)
-		min_clock -= TICK_NSEC / 2;
-
 	if (unlikely(delta < 0)) {
 		clock++;
 		goto out;
 	}
  
-	/*
-	 * The clock must stay within a jiffie of the gtod.
-	 * But since we may be at the start of a jiffy or the end of one
-	 * we add another jiffy buffer.
-	 */
-	max_clock = scd->tick_gtod + (2 + delta_jiffies) * TICK_NSEC;
+	max_clock = min_clock + TICK_NSEC;
  
-	delta *= scd->multi;
-	delta >>= MULTI_SHIFT;
-
-	if (unlikely(clock + delta > max_clock) && check_max(scd)) {
+	if (unlikely(clock + delta > max_clock)) {
 		if (clock < max_clock)
 			clock = max_clock;
 		else
@@ -189,12 +130,9 @@
 	if (unlikely(clock < min_clock))
 		clock = min_clock;
  
-	if (time)
-		*time = clock;
-	else {
-		scd->prev_raw = now;
-		scd->clock = clock;
-	}
+	scd->prev_raw = now;
+	scd->tick_jiffies = now_jiffies;
+	scd->clock = clock;
 }
  
 static void lock_double_clock(struct sched_clock_data *data1,
  
  
  
@@ -238,26 +176,21 @@
 		now -= scd->tick_gtod;
  
 		__raw_spin_unlock(&my_scd->lock);
-
-		__update_sched_clock(scd, now, &clock);
-
-		__raw_spin_unlock(&scd->lock);
-
 	} else {
 		__raw_spin_lock(&scd->lock);
-		__update_sched_clock(scd, now, NULL);
-		clock = scd->clock;
-		__raw_spin_unlock(&scd->lock);
 	}
  
+	__update_sched_clock(scd, now);
+	clock = scd->clock;
+
+	__raw_spin_unlock(&scd->lock);
+
 	return clock;
 }
  
 void sched_clock_tick(void)
 {
 	struct sched_clock_data *scd = this_scd();
-	unsigned long now_jiffies = jiffies;
-	s64 mult, delta_gtod, delta_raw;
 	u64 now, now_gtod;
  
 	if (unlikely(!sched_clock_running))
  
  
@@ -269,29 +202,14 @@
 	now = sched_clock();
  
 	__raw_spin_lock(&scd->lock);
-	__update_sched_clock(scd, now, NULL);
+	__update_sched_clock(scd, now);
 	/*
 	 * update tick_gtod after __update_sched_clock() because that will
 	 * already observe 1 new jiffy; adding a new tick_gtod to that would
 	 * increase the clock 2 jiffies.
 	 */
-	delta_gtod = now_gtod - scd->tick_gtod;
-	delta_raw = now - scd->tick_raw;
-
-	if ((long)delta_raw > 0) {
-		mult = delta_gtod << MULTI_SHIFT;
-		do_div(mult, delta_raw);
-		scd->multi = mult;
-		if (scd->multi > MAX_MULTI)
-			scd->multi = MAX_MULTI;
-		else if (scd->multi < MIN_MULTI)
-			scd->multi = MIN_MULTI;
-	} else
-		scd->multi = 1 << MULTI_SHIFT;
-
 	scd->tick_raw = now;
 	scd->tick_gtod = now_gtod;
-	scd->tick_jiffies = now_jiffies;
 	__raw_spin_unlock(&scd->lock);
 }
  
@@ -321,7 +239,6 @@
 	__raw_spin_lock(&scd->lock);
 	scd->prev_raw = now;
 	scd->clock += delta_ns;
-	scd->multi = 1 << MULTI_SHIFT;
 	__raw_spin_unlock(&scd->lock);
  
 	touch_softlockup_watchdog();
@@ -289,7 +289,6 @@
 			ts->tick_stopped = 1;
 			ts->idle_jiffies = last_jiffies;
 			rcu_enter_nohz();
-			sched_clock_tick_stop(cpu);
 		}
  
 		/*
@@ -392,7 +391,6 @@
 	select_nohz_load_balancer(0);
 	now = ktime_get();
 	tick_do_update_jiffies64(now);
-	sched_clock_tick_start(cpu);
 	cpu_clear(cpu, nohz_cpu_mask);
  
 	/*
...	...	@@ -1572,28 +1572,13 @@
1572	1572	static inline void sched_clock_idle_wakeup_event(u64 delta_ns)
1573	1573	{
1574	1574	}
1575		-
1576		-#ifdef CONFIG_NO_HZ
1577		-static inline void sched_clock_tick_stop(int cpu)
1578		-{
1579		-}
1580		-
1581		-static inline void sched_clock_tick_start(int cpu)
1582		-{
1583		-}
1584		-#endif
1585		-
1586		-#else /* CONFIG_HAVE_UNSTABLE_SCHED_CLOCK */
	1575	+#else
1587	1576	extern void sched_clock_init(void);
1588	1577	extern u64 sched_clock_cpu(int cpu);
1589	1578	extern void sched_clock_tick(void);
1590	1579	extern void sched_clock_idle_sleep_event(void);
1591	1580	extern void sched_clock_idle_wakeup_event(u64 delta_ns);
1592		-#ifdef CONFIG_NO_HZ
1593		-extern void sched_clock_tick_stop(int cpu);
1594		-extern void sched_clock_tick_start(int cpu);
1595	1581	#endif
1596		-#endif /* CONFIG_HAVE_UNSTABLE_SCHED_CLOCK */
1597	1582
1598	1583	/*
1599	1584	* For kernel-internal use: high-speed (but slightly incorrect) per-cpu
...	...	@@ -44,11 +44,6 @@
44	44
45	45	#ifdef CONFIG_HAVE_UNSTABLE_SCHED_CLOCK
46	46
47		-#define MULTI_SHIFT 15
48		-/* Max is double, Min is 1/2 */
49		-#define MAX_MULTI (2LL << MULTI_SHIFT)
50		-#define MIN_MULTI (1LL << (MULTI_SHIFT-1))
51		-
52	47	struct sched_clock_data {
53	48	/*
54	49	* Raw spinlock - this is a special case: this might be called
...	...	@@ -62,10 +57,6 @@
62	57	u64 tick_raw;
63	58	u64 tick_gtod;
64	59	u64 clock;
65		- s64 multi;
66		-#ifdef CONFIG_NO_HZ
67		- int check_max;
68		-#endif
69	60	};
70	61
71	62	static DEFINE_PER_CPU_SHARED_ALIGNED(struct sched_clock_data, sched_clock_data);
72	63
73	64
74	65
...	...	@@ -97,53 +88,18 @@
97	88	scd->tick_raw = 0;
98	89	scd->tick_gtod = ktime_now;
99	90	scd->clock = ktime_now;
100		- scd->multi = 1 << MULTI_SHIFT;
101		-#ifdef CONFIG_NO_HZ
102		- scd->check_max = 1;
103		-#endif
104	91	}
105	92
106	93	sched_clock_running = 1;
107	94	}
108	95
109		-#ifdef CONFIG_NO_HZ
110	96	/*
111		- * The dynamic ticks makes the delta jiffies inaccurate. This
112		- * prevents us from checking the maximum time update.
113		- * Disable the maximum check during stopped ticks.
114		- */
115		-void sched_clock_tick_stop(int cpu)
116		-{
117		- struct sched_clock_data *scd = cpu_sdc(cpu);
118		-
119		- scd->check_max = 0;
120		-}
121		-
122		-void sched_clock_tick_start(int cpu)
123		-{
124		- struct sched_clock_data *scd = cpu_sdc(cpu);
125		-
126		- scd->check_max = 1;
127		-}
128		-
129		-static int check_max(struct sched_clock_data *scd)
130		-{
131		- return scd->check_max;
132		-}
133		-#else
134		-static int check_max(struct sched_clock_data *scd)
135		-{
136		- return 1;
137		-}
138		-#endif /* CONFIG_NO_HZ */
139		-
140		-/*
141	97	* update the percpu scd from the raw @now value
142	98	*
143	99	* - filter out backward motion
144	100	* - use jiffies to generate a min,max window to clip the raw values
145	101	*/
146		-static void __update_sched_clock(struct sched_clock_data scd, u64 now, u64 time)
	102	+static void __update_sched_clock(struct sched_clock_data *scd, u64 now)
147	103	{
148	104	unsigned long now_jiffies = jiffies;
149	105	long delta_jiffies = now_jiffies - scd->tick_jiffies;
150	106
151	107
152	108
...	...	@@ -152,31 +108,16 @@
152	108	s64 delta = now - scd->prev_raw;
153	109
154	110	WARN_ON_ONCE(!irqs_disabled());
	111	+ min_clock = scd->tick_gtod + delta_jiffies * TICK_NSEC;
155	112
156		- /*
157		- * At schedule tick the clock can be just under the gtod. We don't
158		- * want to push it too prematurely.
159		- */
160		- min_clock = scd->tick_gtod + (delta_jiffies * TICK_NSEC);
161		- if (min_clock > TICK_NSEC)
162		- min_clock -= TICK_NSEC / 2;
163		-
164	113	if (unlikely(delta < 0)) {
165	114	clock++;
166	115	goto out;
167	116	}
168	117
169		- /*
170		- * The clock must stay within a jiffie of the gtod.
171		- * But since we may be at the start of a jiffy or the end of one
172		- * we add another jiffy buffer.
173		- */
174		- max_clock = scd->tick_gtod + (2 + delta_jiffies) * TICK_NSEC;
	118	+ max_clock = min_clock + TICK_NSEC;
175	119
176		- delta *= scd->multi;
177		- delta >>= MULTI_SHIFT;
178		-
179		- if (unlikely(clock + delta > max_clock) && check_max(scd)) {
	120	+ if (unlikely(clock + delta > max_clock)) {
180	121	if (clock < max_clock)
181	122	clock = max_clock;
182	123	else
...	...	@@ -189,12 +130,9 @@
189	130	if (unlikely(clock < min_clock))
190	131	clock = min_clock;
191	132
192		- if (time)
193		- *time = clock;
194		- else {
195		- scd->prev_raw = now;
196		- scd->clock = clock;
197		- }
	133	+ scd->prev_raw = now;
	134	+ scd->tick_jiffies = now_jiffies;
	135	+ scd->clock = clock;
198	136	}
199	137
200	138	static void lock_double_clock(struct sched_clock_data *data1,
201	139
202	140
203	141
...	...	@@ -238,26 +176,21 @@
238	176	now -= scd->tick_gtod;
239	177
240	178	__raw_spin_unlock(&my_scd->lock);
241		-
242		- __update_sched_clock(scd, now, &clock);
243		-
244		- __raw_spin_unlock(&scd->lock);
245		-
246	179	} else {
247	180	__raw_spin_lock(&scd->lock);
248		- __update_sched_clock(scd, now, NULL);
249		- clock = scd->clock;
250		- __raw_spin_unlock(&scd->lock);
251	181	}
252	182
	183	+ __update_sched_clock(scd, now);
	184	+ clock = scd->clock;
	185	+
	186	+ __raw_spin_unlock(&scd->lock);
	187	+
253	188	return clock;
254	189	}
255	190
256	191	void sched_clock_tick(void)
257	192	{
258	193	struct sched_clock_data *scd = this_scd();
259		- unsigned long now_jiffies = jiffies;
260		- s64 mult, delta_gtod, delta_raw;
261	194	u64 now, now_gtod;
262	195
263	196	if (unlikely(!sched_clock_running))
264	197
265	198
...	...	@@ -269,29 +202,14 @@
269	202	now = sched_clock();
270	203
271	204	__raw_spin_lock(&scd->lock);
272		- __update_sched_clock(scd, now, NULL);
	205	+ __update_sched_clock(scd, now);
273	206	/*
274	207	* update tick_gtod after __update_sched_clock() because that will
275	208	* already observe 1 new jiffy; adding a new tick_gtod to that would
276	209	* increase the clock 2 jiffies.
277	210	*/
278		- delta_gtod = now_gtod - scd->tick_gtod;
279		- delta_raw = now - scd->tick_raw;
280		-
281		- if ((long)delta_raw > 0) {
282		- mult = delta_gtod << MULTI_SHIFT;
283		- do_div(mult, delta_raw);
284		- scd->multi = mult;
285		- if (scd->multi > MAX_MULTI)
286		- scd->multi = MAX_MULTI;
287		- else if (scd->multi < MIN_MULTI)
288		- scd->multi = MIN_MULTI;
289		- } else
290		- scd->multi = 1 << MULTI_SHIFT;
291		-
292	211	scd->tick_raw = now;
293	212	scd->tick_gtod = now_gtod;
294		- scd->tick_jiffies = now_jiffies;
295	213	__raw_spin_unlock(&scd->lock);
296	214	}
297	215
...	...	@@ -321,7 +239,6 @@
321	239	__raw_spin_lock(&scd->lock);
322	240	scd->prev_raw = now;
323	241	scd->clock += delta_ns;
324		- scd->multi = 1 << MULTI_SHIFT;
325	242	__raw_spin_unlock(&scd->lock);
326	243
327	244	touch_softlockup_watchdog();
...	...	@@ -289,7 +289,6 @@
289	289	ts->tick_stopped = 1;
290	290	ts->idle_jiffies = last_jiffies;
291	291	rcu_enter_nohz();
292		- sched_clock_tick_stop(cpu);
293	292	}
294	293
295	294	/*
...	...	@@ -392,7 +391,6 @@
392	391	select_nohz_load_balancer(0);
393	392	now = ktime_get();
394	393	tick_do_update_jiffies64(now);
395		- sched_clock_tick_start(cpu);
396	394	cpu_clear(cpu, nohz_cpu_mask);
397	395
398	396	/*