Messages in this thread | | | Date | Wed, 29 Feb 2012 10:17:32 +0100 | From | Ingo Molnar <> | Subject | Re: [PATCH] cpumask: fix lg_lock/br_lock. |
| |
* Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> wrote:
> Hi Andrew, > > On 02/29/2012 02:57 AM, Andrew Morton wrote: > > > On Tue, 28 Feb 2012 09:43:59 +0100 > > Ingo Molnar <mingo@elte.hu> wrote: > > > >> This patch should also probably go upstream through the > >> locking/lockdep tree? Mind sending it us once you think it's > >> ready? > > > > Oh goody, that means you own > > http://marc.info/?l=linux-kernel&m=131419353511653&w=2. > > > > > That bug got fixed sometime around Dec 2011. See commit e30e2fdf > (VFS: Fix race between CPU hotplug and lglocks)
The lglocks code is still CPU-hotplug racy AFAICS, despite the ->cpu_lock complication:
Consider a taken global lock on a CPU:
CPU#1 ... br_write_lock(vfsmount_lock);
this takes the lock of all online CPUs: say CPU#1 and CPU#2. Now CPU#3 comes online and takes the read lock:
CPU#3 br_read_lock(vfsmount_lock);
This will succeed while the br_write_lock() is still active, because CPU#1 has only taken the locks of CPU#1 and CPU#2.
Crash!
The proper fix would be for CPU-online to serialize with all known lglocks, via the notifier callback, i.e. to do something like this:
case CPU_UP_PREPARE: for_each_online_cpu(cpu) { spin_lock(&name##_cpu_lock); spin_unlock(&name##_cpu_lock); } ...
I.e. in essence do:
case CPU_UP_PREPARE: name##_global_lock_online(); name##_global_unlock_online();
Another detail I noticed, this bit:
register_hotcpu_notifier(&name##_lg_cpu_notifier); \ get_online_cpus(); \ for_each_online_cpu(i) \ cpu_set(i, name##_cpus); \ put_online_cpus(); \
could be something simpler and loop-less, like:
get_online_cpus(); cpumask_copy(name##_cpus, cpu_online_mask); register_hotcpu_notifier(&name##_lg_cpu_notifier); put_online_cpus();
Thanks,
Ingo
| |