| 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
| ||
|
Message-ID: <20190506193937.GA10264@sinkpad>
Date: Mon, 6 May 2019 15:39:37 -0400
From: Julien Desfossez <jdesfossez@...italocean.com>
To: Aaron Lu <aaron.lu@...ux.alibaba.com>
Cc: Vineeth Remanan Pillai <vpillai@...italocean.com>,
Phil Auld <pauld@...hat.com>,
Nishanth Aravamudan <naravamudan@...italocean.com>,
Peter Zijlstra <peterz@...radead.org>,
Tim Chen <tim.c.chen@...ux.intel.com>, mingo@...nel.org,
tglx@...utronix.de, pjt@...gle.com, torvalds@...ux-foundation.org,
linux-kernel@...r.kernel.org, subhra.mazumdar@...cle.com,
fweisbec@...il.com, keescook@...omium.org, kerrnel@...gle.com,
Aaron Lu <aaron.lwe@...il.com>,
Aubrey Li <aubrey.intel@...il.com>,
Valentin Schneider <valentin.schneider@....com>,
Mel Gorman <mgorman@...hsingularity.net>,
Pawan Gupta <pawan.kumar.gupta@...ux.intel.com>,
Paolo Bonzini <pbonzini@...hat.com>
Subject: Re: [RFC PATCH v2 00/17] Core scheduling v2
On 29-Apr-2019 11:53:21 AM, Aaron Lu wrote:
> On Tue, Apr 23, 2019 at 06:45:27PM +0000, Vineeth Remanan Pillai wrote:
> > >> - Processes with different tags can still share the core
> >
> > > I may have missed something... Could you explain this statement?
> >
> > > This, to me, is the whole point of the patch series. If it's not
> > > doing this then ... what?
> >
> > What I meant was, the patch needs some more work to be accurate.
> > There are some race conditions where the core violation can still
> > happen. In our testing, we saw around 1 to 5% of the time being
> > shared with incompatible processes. One example of this happening
> > is as follows(let cpu 0 and 1 be siblings):
> > - cpu 0 selects a process with a cookie
> > - cpu 1 selects a higher priority process without cookie
> > - Selection process restarts for cpu 0 and it might select a
> > process with cookie but with lesser priority.
> > - Since it is lesser priority, the logic in pick_next_task
> > doesn't compare again for the cookie(trusts pick_task) and
> > proceeds.
> >
> > This is one of the scenarios that we saw from traces, but there
> > might be other race conditions as well. Fix seems a little
> > involved and We are working on that.
>
> This is what I have used to make sure no two unmatched tasks being
> scheduled on the same core: (on top of v1, I thinks it's easier to just
> show the diff instead of commenting on various places of the patches :-)
We imported this fix in v2 and made some small changes and optimizations
(with and without Peter’s fix from https://lkml.org/lkml/2019/4/26/658)
and in both cases, the performance problem where the core can end up
idle with tasks in its runqueues came back.
This is pretty easy to reproduce with a multi-file disk write benchmark.
Here is the patch based on your changes applied on v2 (on top of Peter’s
fix):
diff --git a/kernel/sched/core.c b/kernel/sched/core.c
index 07f3f0c..e09fa25 100644
--- a/kernel/sched/core.c
+++ b/kernel/sched/core.c
@@ -3653,6 +3653,13 @@ static inline bool cookie_match(struct task_struct *a, struct task_struct *b)
}
// XXX fairness/fwd progress conditions
+/*
+ * Returns
+ * - NULL if there is no runnable task for this class.
+ * - the highest priority task for this runqueue if it matches
+ * rq->core->core_cookie or its priority is greater than max.
+ * - Else returns idle_task.
+ */
static struct task_struct *
pick_task(struct rq *rq, const struct sched_class *class, struct task_struct *max)
{
@@ -3660,19 +3667,36 @@ pick_task(struct rq *rq, const struct sched_class *class, struct task_struct *ma
unsigned long cookie = rq->core->core_cookie;
class_pick = class->pick_task(rq);
- if (!cookie)
+ if (!class_pick)
+ return NULL;
+
+ if (!cookie) {
+ /*
+ * If class_pick is tagged, return it only if it has
+ * higher priority than max.
+ */
+ if (max && class_pick->core_cookie &&
+ core_prio_less(class_pick, max))
+ return idle_sched_class.pick_task(rq);
+
+ return class_pick;
+ }
+
+ /*
+ * If there is a cooke match here, return early.
+ */
+ if (class_pick->core_cookie == cookie)
return class_pick;
cookie_pick = sched_core_find(rq, cookie);
- if (!class_pick)
- return cookie_pick;
/*
* If class > max && class > cookie, it is the highest priority task on
* the core (so far) and it must be selected, otherwise we must go with
* the cookie pick in order to satisfy the constraint.
*/
- if (cpu_prio_less(cookie_pick, class_pick) && core_prio_less(max, class_pick))
+ if (cpu_prio_less(cookie_pick, class_pick) &&
+ (!max || core_prio_less(max, class_pick)))
return class_pick;
return cookie_pick;
@@ -3742,8 +3766,16 @@ pick_next_task(struct rq *rq, struct task_struct *prev, struct rq_flags *rf)
rq_i->core_pick = NULL;
- if (i != cpu)
+ if (i != cpu) {
update_rq_clock(rq_i);
+
+ /*
+ * If a sibling is idle, we can initiate an
+ * unconstrained pick.
+ */
+ if (is_idle_task(rq_i->curr) && prev_cookie)
+ prev_cookie = 0UL;
+ }
}
/*
@@ -3820,12 +3852,14 @@ pick_next_task(struct rq *rq, struct task_struct *prev, struct rq_flags *rf)
/*
* If this new candidate is of higher priority than the
* previous; and they're incompatible; we need to wipe
- * the slate and start over.
+ * the slate and start over. pick_task makes sure that
+ * p's priority is more than max if it doesn't match
+ * max's cookie.
*
* NOTE: this is a linear max-filter and is thus bounded
* in execution time.
*/
- if (!max || core_prio_less(max, p)) {
+ if (!max || !cookie_match(max, p)) {
struct task_struct *old_max = max;
rq->core->core_cookie = p->core_cookie;
@@ -3833,7 +3867,7 @@ pick_next_task(struct rq *rq, struct task_struct *prev, struct rq_flags *rf)
trace_printk("max: %s/%d %lx\n", max->comm, max->pid, max->core_cookie);
- if (old_max && !cookie_match(old_max, p)) {
+ if (old_max) {
for_each_cpu(j, smt_mask) {
if (j == i)
continue;
@@ -3879,6 +3913,23 @@ next_class:;
trace_printk("picked: %s/%d %lx\n", next->comm, next->pid, next->core_cookie);
+ /* make sure we didn't break L1TF */
+ for_each_cpu(i, smt_mask) {
+ struct rq *rq_i = cpu_rq(i);
+ if (i == cpu)
+ continue;
+
+ if (likely(cookie_match(next, rq_i->core_pick)))
+ continue;
+
+ trace_printk("[%d]: cookie mismatch. %s/%d/0x%lx/0x%lx\n",
+ rq_i->cpu, rq_i->core_pick->comm,
+ rq_i->core_pick->pid,
+ rq_i->core_pick->core_cookie,
+ rq_i->core->core_cookie);
+ WARN_ON_ONCE(1);
+ }
+
done:
set_next_task(rq, next);
return next;
Powered by blists - more mailing lists