Messages in this thread |  | | | Date | Thu, 30 Jul 2009 19:51:08 +0200 | | From | Oleg Nesterov <> | | Subject | Re: [PATCH] cpusets: fix deadlock with cpu_down()->cpuset_lock() |
| |
On 07/30, Lai Jiangshan wrote: > > Oleg Nesterov wrote: > > On 07/29, Oleg Nesterov wrote: > >> I strongly believe the bug does exist, but this patch needs the review > >> from maintainers. > > > > Yes... > > > >> IOW, with this patch migration_call(CPU_DEAD) runs without callback_mutex, > >> but kernel/cpuset.c always takes get_online_cpus() before callback_mutex. > > > > Oh. I'm afraid this is not an option. > > > > callback_mutex should nest under cgroup_mutex, but cpu hotplu pathes > > take cgroup_mutex under cpu_hotplug->lock. Lockdep won't be happy. > > > > Oleg. > > > > We have made great effort to remove get_online_cpus() from cgroup_mutex > critical region.
Agreed.
> We can migrate the owner of callback_mutex in migration_call(CPU_DEAD) > at first(and then take callback_mutex and migrate others).
Not sure I understand how can we do this. Even if we know the owner of callback_mutex, if we can migrate it safely without callback_mutex why we can't migrate other tasks without this lock?
In any case this doesn't look like a clean solution, imho. But I hardly understand what cpuset is, can't suggest something clever.
I don't really understand why guarantee_online_cpus() needs this mutex, and I don' understand why it have to check cs->parent.
update_cpumask() doesn't allow to set ->cpus_allowed which does not intersect with cpu_online_mask (unless cs is empty). This means that guarantee_online_cpus()->cpumask_intersects() == T is only possible when we are called from cpu_down() path, right? But can't we just return cpu_online_mask in this case? I mean,
static void guarantee_online_cpus(const struct cpuset *cs, struct cpumask *pmask) { if (cpumask_intersects(cs->cpus_allowed, cpu_online_mask)) cpumask_and(pmask, cs->cpus_allowed, cpu_online_mask); else /* !!!!!!!!!!!!!!!!!!!! * cpuset_track_online_cpus(CPU_DEAD)->scan_for_empty_cpusets() * will fix this. */ cpumask_copy(pmask, cpu_online_mask); } Most probably I missed something, never looked in cpuset.c before.
Oleg.
|  |