linux-kernel - Re: [PATCH v2 02/13] sched/fair: Consistent use of prev

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]

Message-ID: <1466618651.15275.21.camel@redhat.com>
Date:	Wed, 22 Jun 2016 14:04:11 -0400
From:	Rik van Riel <riel@...hat.com>
To:	Morten Rasmussen <morten.rasmussen@....com>, peterz@...radead.org,
	mingo@...hat.com
Cc:	dietmar.eggemann@....com, yuyang.du@...el.com,
	vincent.guittot@...aro.org, mgalbraith@...e.de,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 02/13] sched/fair: Consistent use of prev_cpu in
 wakeup path

On Wed, 2016-06-22 at 18:03 +0100, Morten Rasmussen wrote:
> In commit ac66f5477239 ("sched/numa: Introduce migrate_swap()")
> select_task_rq() got a 'cpu' argument to enable overriding of
> prev_cpu
> in special cases (NUMA task swapping). However, the
> select_task_rq_fair() helper functions: wake_affine() and
> select_idle_sibling(), still use task_cpu(p) directly to work out
> prev_cpu which leads to inconsistencies.
> 
> This patch passes prev_cpu (potentially overridden by NUMA code) into
> the helper functions to ensure prev_cpu is indeed the same cpu
> everywhere in the wakeup path.
> 
> cc: Ingo Molnar <mingo@...hat.com>
> cc: Peter Zijlstra <peterz@...radead.org>
> cc: Rik van Riel <riel@...hat.com>
> 
> Signed-off-by: Morten Rasmussen <morten.rasmussen@....com>
> ---
>  kernel/sched/fair.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/kernel/sched/fair.c b/kernel/sched/fair.c
> index c6dd8bab010c..eec8e29104f9 100644
> --- a/kernel/sched/fair.c
> +++ b/kernel/sched/fair.c
> @@ -656,7 +656,7 @@ static u64 sched_vslice(struct cfs_rq *cfs_rq,
> struct sched_entity *se)
>  }
>  
>  #ifdef CONFIG_SMP
> -static int select_idle_sibling(struct task_struct *p, int cpu);
> +static int select_idle_sibling(struct task_struct *p, int prev_cpu,
> int cpu);
>  static unsigned long task_h_load(struct task_struct *p);
>  
>  /*
> @@ -1483,7 +1483,8 @@ static void task_numa_compare(struct
> task_numa_env *env,
>  	 * Call select_idle_sibling to maybe find a better one.
>  	 */
>  	if (!cur)
> -		env->dst_cpu = select_idle_sibling(env->p, env-
> >dst_cpu);
> +		env->dst_cpu = select_idle_sibling(env->p, env-
> >src_cpu,
> +						   env->dst_cpu);

It is worth remembering that "prev" will only
ever be returned by select_idle_sibling() if
it is part of the same NUMA node as target.

That means this patch does not change behaviour
of the NUMA balancing code, since that always
migrates between nodes.

Now lets look at try_to_wake_up(). It will pass
p->wake_cpu as the argument for "prev_cpu", which
again appears to be the same CPU number as that used
by the current code.

I have no objection to your patch, but must be
overlooking something, since I cannot find a change
in behaviour that your patch would create.

-- 
All rights reversed

Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)