Skip to contentSkip to conversation

Patch "sched/fair: Fix cfs_rq avg tracking underflow" has been added to the 4.6-stable tree

PATCH [email protected] 1 participant 1 message Jul 13, 2016

Subsystems (2)
lore
Share & tools
lore

[email protected]

author maintainer Applied
#0 lore raw

This is a note to let you know that I've just added the patch titled

sched/fair: Fix cfs_rq avg tracking underflow

to the 4.6-stable tree which can be found at:
http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary

The filename of the patch is:
sched-fair-fix-cfs_rq-avg-tracking-underflow.patch
and it can be found in the queue-4.6 subdirectory.

If you, or anyone else, feels it should not be added to the stable tree, please let <[email protected]> know about it.

Quoted 1 line

From 8974189222159154c55f24ddad33e3613960521a Mon Sep 17 00:00:00 2001

From: Peter Zijlstra <[email protected]>
Date: Thu, 16 Jun 2016 10:50:40 +0200
Subject: sched/fair: Fix cfs_rq avg tracking underflow

From: Peter Zijlstra <[email protected]>

commit 8974189222159154c55f24ddad33e3613960521a upstream.

As per commit:

b7fa30c9cc48 ("sched/fair: Fix post_init_entity_util_avg() serialization")

Quoted 22 lines

the code generated from update_cfs_rq_load_avg():

	if (atomic_long_read(&cfs_rq->removed_load_avg)) {
		s64 r = atomic_long_xchg(&cfs_rq->removed_load_avg, 0);
		sa->load_avg = max_t(long, sa->load_avg - r, 0);
		sa->load_sum = max_t(s64, sa->load_sum - r * LOAD_AVG_MAX, 0);
		removed_load = 1;
	}

turns into:

ffffffff81087064:       49 8b 85 98 00 00 00    mov    0x98(%r13),%rax
ffffffff8108706b:       48 85 c0                test   %rax,%rax
ffffffff8108706e:       74 40                   je     ffffffff810870b0 <update_blocked_averages+0xc0>
ffffffff81087070:       4c 89 f8                mov    %r15,%rax
ffffffff81087073:       49 87 85 98 00 00 00    xchg   %rax,0x98(%r13)
ffffffff8108707a:       49 29 45 70             sub    %rax,0x70(%r13)
ffffffff8108707e:       4c 89 f9                mov    %r15,%rcx
ffffffff81087081:       bb 01 00 00 00          mov    $0x1,%ebx
ffffffff81087086:       49 83 7d 70 00          cmpq   $0x0,0x70(%r13)
ffffffff8108708b:       49 0f 49 4d 70          cmovns 0x70(%r13),%rcx
Quoted 2 lines

Which you'll note ends up with sa->load_avg -= r in memory at ffffffff8108707a.

So I _should_ have looked at other unserialized users of ->load_avg, but alas. Luckily nikbor reported a similar /0 from task_h_load() which instantly triggered recollection of this here problem.

Aside from the intermediate value hitting memory and causing problems, there's another problem: the underflow detection relies on the signed bit. This reduces the effective width of the variables, IOW its effectively the same as having these variables be of signed type.

This patch changes to a different means of unsigned underflow detection to not rely on the signed bit. This allows the variables to use the 'full' unsigned range. And it does so with explicit LOAD - STORE to ensure any intermediate value will never be visible in memory, allowing these unserialized loads.

Note: GCC generates crap code for this, might warrant a look later.

Note2: I say 'full' above, if we end up at U*_MAX we'll still explode;
maybe we should do clamping on add too.

Cc Andrey Ryabinin <[email protected]>
Cc Chris Wilson <[email protected]>
Cc Linus Torvalds <[email protected]>
Cc Mike Galbraith <[email protected]>
Cc Peter Zijlstra <[email protected]>
Cc Thomas Gleixner <[email protected]>
Cc Yuyang Du <[email protected]>
Fixes 9d89c257dfb9 ("sched/fair: Rewrite runnable load and utilization average tracking")
---
 kernel/sched/fair.c |   33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)
kernel/sched/fair.c +25 -8
@@ -2821,6 +2821,23 @@ static inline void update_tg_load_avg(st
 
 static inline u64 cfs_rq_clock_task(struct cfs_rq *cfs_rq);
 
+/*
+ * Unsigned subtract and clamp on underflow.
+ *
+ * Explicitly do a load-store to ensure the intermediate value never hits
+ * memory. This allows lockless observations without ever seeing the negative
+ * values.
+ */
+#define sub_positive(_ptr, _val) do {				\
+	typeof(_ptr) ptr = (_ptr);				\
+	typeof(*ptr) val = (_val);				\
+	typeof(*ptr) res, var = READ_ONCE(*ptr);		\
+	res = var - val;					\
+	if (res > var)						\
+		res = 0;					\
+	WRITE_ONCE(*ptr, res);					\
+} while (0)
+
 /* Group cfs_rq's load_avg is used for task_h_load and update_cfs_share */
 static inline int update_cfs_rq_load_avg(u64 now, struct cfs_rq *cfs_rq)
 {
@@ -2829,15 +2846,15 @@ static inline int update_cfs_rq_load_avg
 
 	if (atomic_long_read(&cfs_rq->removed_load_avg)) {
 		s64 r = atomic_long_xchg(&cfs_rq->removed_load_avg, 0);
-		sa->load_avg = max_t(long, sa->load_avg - r, 0);
-		sa->load_sum = max_t(s64, sa->load_sum - r * LOAD_AVG_MAX, 0);
+		sub_positive(&sa->load_avg, r);
+		sub_positive(&sa->load_sum, r * LOAD_AVG_MAX);
 		removed = 1;
 	}
 
 	if (atomic_long_read(&cfs_rq->removed_util_avg)) {
 		long r = atomic_long_xchg(&cfs_rq->removed_util_avg, 0);
-		sa->util_avg = max_t(long, sa->util_avg - r, 0);
-		sa->util_sum = max_t(s32, sa->util_sum - r * LOAD_AVG_MAX, 0);
+		sub_positive(&sa->util_avg, r);
+		sub_positive(&sa->util_sum, r * LOAD_AVG_MAX);
 	}
 
 	decayed = __update_load_avg(now, cpu_of(rq_of(cfs_rq)), sa,
@@ -2927,10 +2944,10 @@ static void detach_entity_load_avg(struc
 			  &se->avg, se->on_rq * scale_load_down(se->load.weight),
 			  cfs_rq->curr == se, NULL);
 
-	cfs_rq->avg.load_avg = max_t(long, cfs_rq->avg.load_avg - se->avg.load_avg, 0);
-	cfs_rq->avg.load_sum = max_t(s64,  cfs_rq->avg.load_sum - se->avg.load_sum, 0);
-	cfs_rq->avg.util_avg = max_t(long, cfs_rq->avg.util_avg - se->avg.util_avg, 0);
-	cfs_rq->avg.util_sum = max_t(s32,  cfs_rq->avg.util_sum - se->avg.util_sum, 0);
+	sub_positive(&cfs_rq->avg.load_avg, se->avg.load_avg);
+	sub_positive(&cfs_rq->avg.load_sum, se->avg.load_sum);
+	sub_positive(&cfs_rq->avg.util_avg, se->avg.util_avg);
+	sub_positive(&cfs_rq->avg.util_sum, se->avg.util_sum);
 }
 
 /* Add the load generated by se into cfs_rq's load average */

Patches currently in stable-queue which might be from [email protected] are

queue-4.6/futex-calculate-the-futex-key-based-on-a-tail-page-for-file-based-futexes.patch queue-4.6/kprobes-x86-clear-tf-bit-in-fault-on-single-stepping.patch queue-4.6/sched-fair-fix-cfs_rq-avg-tracking-underflow.patch queue-4.6/x86-amd_nb-fix-boot-crash-on-non-amd-systems.patch queue-4.6/perf-x86-fix-32-bit-perf-user-callgraph-collection.patch queue-4.6/locking-qspinlock-fix-spin_unlock_wait-some-more.patch queue-4.6/locking-static_key-fix-concurrent-static_key_slow_inc.patch queue-4.6/perf-x86-intel-rapl-fix-pmus-free-during-cleanup.patch queue-4.6/locking-ww_mutex-report-recursive-ww_mutex-locking-early.patch