Re: [PATCH v2 02/13] sched/fair: Consistent use of prev_cpu in wakeup path

[Rik van Riel]

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@xxxxxxxxxx>
> cc: Peter Zijlstra <peterz@xxxxxxxxxxxxx>
> cc: Rik van Riel <riel@xxxxxxxxxx>
> 
> Signed-off-by: Morten Rasmussen <morten.rasmussen@xxxxxxx>
> ---
> Â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

Attachment: signature.asc
Description: This is a digitally signed message part