linux-kernel - Re: [PATCH 5/5] sched: use the old min_vruntime when normalizing on dequeue

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-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sat, 20 Nov 2010 11:55:11 +0100
From:	Peter Zijlstra <peterz@...radead.org>
To:	John Stultz <john.stultz@...aro.org>
Cc:	lkml <linux-kernel@...r.kernel.org>, Dima Zavin <dima@...roid.com>,
	Ingo Molnar <mingo@...e.hu>,
	Arve Hjønnevåg <arve@...roid.com>
Subject: Re: [PATCH 5/5] sched: use the old min_vruntime when normalizing
 on dequeue

On Fri, 2010-11-19 at 18:08 -0800, John Stultz wrote:
> From: Dima Zavin <dima@...roid.com>
> 
> After pulling the thread off the run-queue during a cgroup change,
> the cfs_rq.min_vruntime gets recalculated. The dequeued thread's vruntime
> then gets normalized to this new value. This can then lead to the thread
> getting an unfair boost in the new group if the vruntime of the next
> task in the old run-queue was way further ahead.
> 
> CC: Ingo Molnar <mingo@...e.hu>
> CC: Peter Zijlstra <peterz@...radead.org>
> Cc: Arve Hjønnevåg <arve@...roid.com>
> Signed-off-by: Dima Zavin <dima@...roid.com>
> Signed-off-by: John Stultz <john.stultz@...aro.org>
> ---
>  kernel/sched_fair.c |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/sched_fair.c b/kernel/sched_fair.c
> index f4f6a83..72f19ad 100644
> --- a/kernel/sched_fair.c
> +++ b/kernel/sched_fair.c
> @@ -802,6 +802,8 @@ static void clear_buddies(struct cfs_rq *cfs_rq, struct sched_entity *se)
>  static void
>  dequeue_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags)
>  {
> +	u64 min_vruntime;
> +
>  	/*
>  	 * Update run-time statistics of the 'current'.
>  	 */
> @@ -826,6 +828,8 @@ dequeue_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags)
>  	if (se != cfs_rq->curr)
>  		__dequeue_entity(cfs_rq, se);
>  	account_entity_dequeue(cfs_rq, se);
> +
> +	min_vruntime = cfs_rq->min_vruntime;
>  	update_min_vruntime(cfs_rq);
>  
>  	/*
> @@ -834,7 +838,7 @@ dequeue_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags)
>  	 * movement in our normalized position.
>  	 */
>  	if (!(flags & DEQUEUE_SLEEP))
> -		se->vruntime -= cfs_rq->min_vruntime;
> +		se->vruntime -= min_vruntime;
>  }

Right, so assuming the reasoning is right (my brain still needs to wake
up) the patch is weird, by not simply move the code bock up and avoid
the whole extra variable like so?

---
 kernel/sched_fair.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/kernel/sched_fair.c b/kernel/sched_fair.c
index d35f464..dfa28ef 100644
--- a/kernel/sched_fair.c
+++ b/kernel/sched_fair.c
@@ -1003,8 +1003,6 @@ dequeue_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags)
 	se->on_rq = 0;
 	update_cfs_load(cfs_rq, 0);
 	account_entity_dequeue(cfs_rq, se);
-	update_min_vruntime(cfs_rq);
-	update_cfs_shares(cfs_rq, 0);
 
 	/*
 	 * Normalize the entity after updating the min_vruntime because the
@@ -1013,6 +1011,9 @@ dequeue_entity(struct cfs_rq *cfs_rq, struct sched_entity *se, int flags)
 	 */
 	if (!(flags & DEQUEUE_SLEEP))
 		se->vruntime -= cfs_rq->min_vruntime;
+
+	update_min_vruntime(cfs_rq);
+	update_cfs_shares(cfs_rq, 0);
 }
 
 /*

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/