linux-kernel - [PATCH] nohz_full: Make sched_should_stop

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <1459539771-4251-1-git-send-email-cmetcalf@mellanox.com>
Date:	Fri, 1 Apr 2016 15:42:51 -0400
From:	Chris Metcalf <cmetcalf@...lanox.com>
To:	Frederic Weisbecker <fweisbec@...il.com>,
	Christoph Lameter <cl@...ux.com>,
	Ingo Molnar <mingo@...nel.org>,
	Luiz Capitulino <lcapitulino@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Rik van Riel <riel@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Viresh Kumar <viresh.kumar@...aro.org>,
	<linux-kernel@...r.kernel.org>
CC:	Chris Metcalf <cmetcalf@...lanox.com>
Subject: [PATCH] nohz_full: Make sched_should_stop_tick() more conservative

On arm64, when calling enqueue_task_fair() from migration_cpu_stop(),
we find the nr_running value updated by add_nr_running(), but the
cfs.nr_running value has not always yet been updated.  Accordingly,
the sched_can_stop_tick() false returns true when we are migrating a
second task onto a core.

Correct this by using rq->nr_running instead of rq->cfs.nr_running.
This should always be more conservative, and reverts the test to the
form it had before commit 76d92ac305f2 ("sched: Migrate sched to use
new tick dependency mask model").

Signed-off-by: Chris Metcalf <cmetcalf@...lanox.com>
---
I found this bug because I had a program running in nohz_full
on a core, and from a different core I called sched_setaffinity()
to force that task onto the nohz_full core, but I did not end up with
a kick to the nohz_full core, so tick-based scheduling did not start.
This is probably bad enough that we should fix it for 4.6.

Strangely, for some reason, the existing code worked correctly for me
for tilegx, but not for arm64.  I see that the enqueue_task_fair()
code calls enqueue_entity(), which calls account_entity_enqueue() to
adjust cfs.nr_running.  That seemed to happen on tilegx, but not arm64.
Perhaps there is some difference in how the sched_entity stuff is done,
but frankly that took me a little deeper into the CFS stuff than I was
willing to dive in this moment.

I could also argue that sched/core.c shouldn't have a lot of CFS
stuff in it anyway, and if we view the FIFO/RR stuff as handling the
real special cases in sched_can_stop_tick() anyway, then just checking
the core nr_running feels like the right thing to do regardless.

 kernel/sched/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 00649f7ad567..1737d63c65fa 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -599,7 +599,7 @@ bool sched_can_stop_tick(struct rq *rq)
 	}

 	/* Normal multitasking need periodic preemption checks */
-	if (rq->cfs.nr_running > 1)
+	if (rq->nr_running > 1)
 		return false;

 	return true;
-- 
2.7.2