cpuset: attach_task() vs sched_setaffinity() race?

cpuset: attach_task() vs sched_setaffinity() race?

[Oleg Nesterov]

After the brief look at kernel/cpuset.c, it seems that attach_task() should
guarantee that the task can't use CPUs outside of cpuset->cpus_allowed.

But this looks racy wrt sched_setaffinity() which does

	cpus_allowed = cpuset_cpus_allowed(p);
	// callback_mutex is free
	set_cpus_allowed(p);

What if attach_task()->set_cpus_allowed() happens in between?


Another question: update_cpumask(cs) does nothing with the tasks attached to
that cpuset, why? It may take a while before the task actually migrates to the
new CPU.

Thanks,

Oleg.

cpusets vs cpu-hotplug interaction is broken?

[Oleg Nesterov]

(cpu-hotplug experts cc'ed)

On 08/25, Oleg Nesterov wrote:
>
> After the brief look at kernel/cpuset.c, it seems that attach_task() should
> guarantee that the task can't use CPUs outside of cpuset->cpus_allowed.
> 
> But this looks racy wrt sched_setaffinity() which does
> 
> 	cpus_allowed = cpuset_cpus_allowed(p);
> 	// callback_mutex is free
> 	set_cpus_allowed(p);
> 
> What if attach_task()->set_cpus_allowed() happens in between?

Actually, I think there is another problem, and cpuset_cpus_allowed() is
just broken wrt CONFIG_HOTPLUG_CPU.

Suppose that CONFIG_CPUSETS is true, but we don't use cpusets. In that
case all tasks in system belong to the top_cpuset (btw, why cpuset_init()
sets init_task.cpuset? this was already done by cpuset_init_early()), and
we should have the same behaviour as without CONFIG_CPUSETS.

By default, all tasks have ->cpus_allowed = CPU_MASK_ALL inherited from
kernel_init(). This means that the task can use the new CPU right after
cpu_up().

Now let's suppose that some task does sched_setaffinity(0, CPU_MASK_ALL).
In that case, cpuset_cpus_allowed() sets ->cpus_allowed = cpu_online_map,
and I think this is just wrong. Now that task doesn't see the new CPUs.

Of course, we have the similar problem with cpusets other than top_cpuset.

In short, unless I missed something, top_cpuset.cpus_allowed should be
cpu_possible_map, guarantee_online_cpus() shouldn't mix "allowed" and
"online" masks.

Oleg.

Re: cpusets vs cpu-hotplug interaction is broken?

[Gautham R Shenoy]

On Tue, Aug 28, 2007 at 05:48:53PM +0400, Oleg Nesterov wrote:
> (cpu-hotplug experts cc'ed)
> 
> On 08/25, Oleg Nesterov wrote:
> >
> > After the brief look at kernel/cpuset.c, it seems that attach_task() should
> > guarantee that the task can't use CPUs outside of cpuset->cpus_allowed.
> > 
> > But this looks racy wrt sched_setaffinity() which does
> > 
> > 	cpus_allowed = cpuset_cpus_allowed(p);
> > 	// callback_mutex is free
> > 	set_cpus_allowed(p);
> > 
> > What if attach_task()->set_cpus_allowed() happens in between?
> 
> Actually, I think there is another problem, and cpuset_cpus_allowed() is
> just broken wrt CONFIG_HOTPLUG_CPU.
> 
> Suppose that CONFIG_CPUSETS is true, but we don't use cpusets. In that
> case all tasks in system belong to the top_cpuset (btw, why cpuset_init()
> sets init_task.cpuset? this was already done by cpuset_init_early()), and
> we should have the same behaviour as without CONFIG_CPUSETS.
> 
> By default, all tasks have ->cpus_allowed = CPU_MASK_ALL inherited from
> kernel_init(). This means that the task can use the new CPU right after
> cpu_up().
> 
> Now let's suppose that some task does sched_setaffinity(0, CPU_MASK_ALL).
> In that case, cpuset_cpus_allowed() sets ->cpus_allowed = cpu_online_map,
> and I think this is just wrong. Now that task doesn't see the new CPUs.
> 

Good point! 

A task's cpu_allowed mask can contain cpus which are offline.
And if those cpus exist in the intersection of the task's requested mask
and cpuset's cpu mask, why should we unset the offlined cpus from that 
intersection? Either way the task is not going to run on the cpus while
they are in the offlined state.  And on cpu_up, if the cpu is present in
the task's allowed mask, it can run on that cpu, which is a good thing.

The two users of cpuset_cpus_allowed - sched_setaffinity and pdflush
don't seem to require the online cpu information.

Paul, is there any particular reason why we need guarentee_online_cpus
to be called in cpuset_cpus_allowed ? 

Thanks and Regards
gautham.

-- 
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"

Re: cpusets vs cpu-hotplug interaction is broken?

[Oleg Nesterov]

On 08/29, Gautham R Shenoy wrote:
>
> On Tue, Aug 28, 2007 at 05:48:53PM +0400, Oleg Nesterov wrote:
> > (cpu-hotplug experts cc'ed)
> > 
> > On 08/25, Oleg Nesterov wrote:
> > >
> > > After the brief look at kernel/cpuset.c, it seems that attach_task() should
> > > guarantee that the task can't use CPUs outside of cpuset->cpus_allowed.
> > > 
> > > But this looks racy wrt sched_setaffinity() which does
> > > 
> > > 	cpus_allowed = cpuset_cpus_allowed(p);
> > > 	// callback_mutex is free
> > > 	set_cpus_allowed(p);
> > > 
> > > What if attach_task()->set_cpus_allowed() happens in between?
> > 
> > Actually, I think there is another problem, and cpuset_cpus_allowed() is
> > just broken wrt CONFIG_HOTPLUG_CPU.
> > 
> > Suppose that CONFIG_CPUSETS is true, but we don't use cpusets. In that
> > case all tasks in system belong to the top_cpuset (btw, why cpuset_init()
> > sets init_task.cpuset? this was already done by cpuset_init_early()), and
> > we should have the same behaviour as without CONFIG_CPUSETS.
> > 
> > By default, all tasks have ->cpus_allowed = CPU_MASK_ALL inherited from
> > kernel_init(). This means that the task can use the new CPU right after
> > cpu_up().
> > 
> > Now let's suppose that some task does sched_setaffinity(0, CPU_MASK_ALL).
> > In that case, cpuset_cpus_allowed() sets ->cpus_allowed = cpu_online_map,
> > and I think this is just wrong. Now that task doesn't see the new CPUs.
> > 
> 
> Good point! 
> 
> A task's cpu_allowed mask can contain cpus which are offline.
> And if those cpus exist in the intersection of the task's requested mask
> and cpuset's cpu mask, why should we unset the offlined cpus from that 
> intersection? Either way the task is not going to run on the cpus while
> they are in the offlined state.  And on cpu_up, if the cpu is present in
> the task's allowed mask, it can run on that cpu, which is a good thing.
> 
> The two users of cpuset_cpus_allowed - sched_setaffinity and pdflush
> don't seem to require the online cpu information.
> 
> Paul, is there any particular reason why we need guarentee_online_cpus
> to be called in cpuset_cpus_allowed ? 

Note also that cpuset_cpus_allowed()->guarentee_online_cpus() easily allows
the task to escape its ->cpuset, sched_setaffinity(cpumask_of_cpu(OFFLINE_CPU))
is enough.

Another note, http://marc.info/?t=118823976000002 really needs something
like guarentee_online_cpus(). However even in this case we shouldn't afaics
"and" cpuset->cpus_allowed with cpu_online_map.

Oleg.

Re: cpusets vs cpu-hotplug interaction is broken?

[Gautham R Shenoy]

On Wed, Aug 29, 2007 at 02:52:04PM +0400, Oleg Nesterov wrote:
> On 08/29, Gautham R Shenoy wrote:
> >
> > On Tue, Aug 28, 2007 at 05:48:53PM +0400, Oleg Nesterov wrote:
> > > (cpu-hotplug experts cc'ed)
> > > 
> > > On 08/25, Oleg Nesterov wrote:
> > > >
> > > > After the brief look at kernel/cpuset.c, it seems that attach_task() should
> > > > guarantee that the task can't use CPUs outside of cpuset->cpus_allowed.
> > > > 
> > > > But this looks racy wrt sched_setaffinity() which does
> > > > 
> > > > 	cpus_allowed = cpuset_cpus_allowed(p);
> > > > 	// callback_mutex is free
> > > > 	set_cpus_allowed(p);
> > > > 
> > > > What if attach_task()->set_cpus_allowed() happens in between?
> > > 
> > > Actually, I think there is another problem, and cpuset_cpus_allowed() is
> > > just broken wrt CONFIG_HOTPLUG_CPU.
> > > 
> > > Suppose that CONFIG_CPUSETS is true, but we don't use cpusets. In that
> > > case all tasks in system belong to the top_cpuset (btw, why cpuset_init()
> > > sets init_task.cpuset? this was already done by cpuset_init_early()), and
> > > we should have the same behaviour as without CONFIG_CPUSETS.
> > > 
> > > By default, all tasks have ->cpus_allowed = CPU_MASK_ALL inherited from
> > > kernel_init(). This means that the task can use the new CPU right after
> > > cpu_up().
> > > 
> > > Now let's suppose that some task does sched_setaffinity(0, CPU_MASK_ALL).
> > > In that case, cpuset_cpus_allowed() sets ->cpus_allowed = cpu_online_map,
> > > and I think this is just wrong. Now that task doesn't see the new CPUs.
> > > 
> > 
> > Good point! 
> > 
> > A task's cpu_allowed mask can contain cpus which are offline.
> > And if those cpus exist in the intersection of the task's requested mask
> > and cpuset's cpu mask, why should we unset the offlined cpus from that 
> > intersection? Either way the task is not going to run on the cpus while
> > they are in the offlined state.  And on cpu_up, if the cpu is present in
> > the task's allowed mask, it can run on that cpu, which is a good thing.
> > 
> > The two users of cpuset_cpus_allowed - sched_setaffinity and pdflush
> > don't seem to require the online cpu information.
> > 
> > Paul, is there any particular reason why we need guarentee_online_cpus
> > to be called in cpuset_cpus_allowed ? 
> 
> Note also that cpuset_cpus_allowed()->guarentee_online_cpus() easily allows
> the task to escape its ->cpuset, sched_setaffinity(cpumask_of_cpu(OFFLINE_CPU))
> is enough.

Well, the comment for cpuset_cpus_allowed() says

/* 
 * Description: Returns the cpumask_t cpus_allowed of the cpuset
 * attached to the specified @tsk.  Guaranteed to return some non-empty
 * subset of cpu_online_map, even if this means going outside the
 * tasks cpuset.             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ 
 **/^^^^^^^^^^^^^

Since this behaviour has been documented, I presume there is a reason
behind it. 

So either we're incorrectly using cpuset_cpus_allowed in
sched_setaffinity or we're missing something subtle :)

Thanks and Regards
gautham.
-- 
Gautham R Shenoy
Linux Technology Center
IBM India.
"Freedom comes with a price tag of responsibility, which is still a bargain,
because Freedom is priceless!"