Why is cpuset_cpus_allowed_fallback() necessary?

Why is cpuset_cpus_allowed_fallback() necessary?

[Tejun Heo]

Hello, guys.

I'm wondering why cpuset_cpus_allowed_fallback() is necessary.  This
is called from, e.g., try_to_wake_up()->select_task_rq() when none of
the cpus in ->cpus_allowed is useable.  The cpuset callback invokes
do_set_cpus_allowed() w/ the cpuset's cpus_allowed.  This was added by
the following commit,

  commit 9084bb8246ea935b98320554229e2f371f7f52fa
  Author: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
  Date:   Mon Mar 15 10:10:27 2010 +0100


  sched: Make select_fallback_rq() cpuset friendly

  Introduce cpuset_cpus_allowed_fallback() helper to fix the cpuset problems
  with select_fallback_rq(). It can be called from any context and can't use
  any cpuset locks including task_lock(). It is called when the task doesn't
  have online cpus in ->cpus_allowed but ttwu/etc must be able to find a
  suitable cpu.
  ....			  

The problem is, nothing's explaining what "the cpuset problems with
select_fallback_rq()" are.  Oleg, do you remember?  Why do we need
this?

Thanks.

-- 
tejun

Re: Why is cpuset_cpus_allowed_fallback() necessary?

[Oleg Nesterov]

Hi Tejun,

On 11/26, Tejun Heo wrote:
>
> I'm wondering why cpuset_cpus_allowed_fallback() is necessary.  This
> is called from, e.g., try_to_wake_up()->select_task_rq() when none of
> the cpus in ->cpus_allowed is useable.  The cpuset callback invokes
> do_set_cpus_allowed() w/ the cpuset's cpus_allowed.  This was added by
> the following commit,
>
>   commit 9084bb8246ea935b98320554229e2f371f7f52fa
>   Author: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>   Date:   Mon Mar 15 10:10:27 2010 +0100
>
>
>   sched: Make select_fallback_rq() cpuset friendly
>
>   Introduce cpuset_cpus_allowed_fallback() helper to fix the cpuset problems
>   with select_fallback_rq(). It can be called from any context and can't use
>   any cpuset locks including task_lock(). It is called when the task doesn't
>   have online cpus in ->cpus_allowed but ttwu/etc must be able to find a
>   suitable cpu.
>   ....
>
> The problem is, nothing's explaining what "the cpuset problems with
> select_fallback_rq()" are.

Cough. You are right, the changelog is confusing and I no can not
understand it too.

> Oleg, do you remember?  Why do we need
> this?

No, I forgot. And this code was changed after that, the fat comment in
cpuset_cpus_allowed_fallback() tried to explain the code below which
was removed.



I am starting to recall what this patch tried to do after I looked into
git history. This patch was the last (probably) change in series.

Please look at
	897f0b3c3ff40b443c84e271bef19bd6ae885195
	sched: Kill the broken and deadlockable cpuset_lock/cpuset_cpus_allowed_locked code

In particular it removes cpuset_cpus_allowed_locked() from
select_fallback_rq() because this was very wrong. IOW, this patch
simply removes the code which didn't really work

And after some other changes, this comment tried to add the supposed
behaviour back: we shouldn't simply use cpu_possible_mask, we should
consult cpuset.

Oleg.

Re: Why is cpuset_cpus_allowed_fallback() necessary?

[Tejun Heo]

Hey, Oleg.

On Tue, Nov 27, 2012 at 07:41:17PM +0100, Oleg Nesterov wrote:
> > Oleg, do you remember?  Why do we need
> > this?
> 
> No, I forgot. And this code was changed after that, the fat comment in
> cpuset_cpus_allowed_fallback() tried to explain the code below which
> was removed.
> 
> I am starting to recall what this patch tried to do after I looked into
> git history. This patch was the last (probably) change in series.
> 
> Please look at
> 	897f0b3c3ff40b443c84e271bef19bd6ae885195
> 	sched: Kill the broken and deadlockable cpuset_lock/cpuset_cpus_allowed_locked code
> 
> In particular it removes cpuset_cpus_allowed_locked() from
> select_fallback_rq() because this was very wrong. IOW, this patch
> simply removes the code which didn't really work
> 
> And after some other changes, this comment tried to add the supposed
> behaviour back: we shouldn't simply use cpu_possible_mask, we should
> consult cpuset.

Ah, okay.  If the task's affinity, which is subset of cpuset's, become
empty while the cpuset's doesn't, we still better confine the task to
the cpuset.  The code seems broken tho - it looks at CPUs which are on
the same node first before consulting cpuset, which may lead to the
task escaping cpuset.  Prolly the function need to be restructured
that it first runs as if cpuset->cpus_allowed is cpu_possible_mask and
then if that fails with the actual cpu_possible_mask.

Thanks!

-- 
tejun