Messages in this thread Patch in this message | | | Subject | call_function_many: fix list delete vs add race | From | Milton Miller <> | Date | Fri, 28 Jan 2011 18:20:14 -0600 |
| |
Peter pointed out there was nothing preventing the list_del_rcu in smp_call_function_interrupt from running before the list_add_rcu in smp_call_function_many. Fix this by not setting refs until we have put the entry on the list. We can use the lock acquire and release instead of a wmb.
Reported-by: Peter Zijlstra <peterz@infradead.org> Signed-off-by: Milton Miller <miltonm@bga.com> Cc: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> ---
I tried to force this race with a udelay before the lock & list_add and by mixing all 64 online cpus with just 3 random cpus in the mask, but was unsuccessful. Still, it seems to be a valid race, and the fix is a simple change to the current code.
Index: common/kernel/smp.c =================================================================== --- common.orig/kernel/smp.c 2011-01-28 16:23:15.000000000 -0600 +++ common/kernel/smp.c 2011-01-28 16:55:01.000000000 -0600 @@ -491,15 +491,17 @@ void smp_call_function_many(const struct cpumask_clear_cpu(this_cpu, data->cpumask); /* - * To ensure the interrupt handler gets an complete view - * we order the cpumask and refs writes and order the read - * of them in the interrupt handler. In addition we may - * only clear our own cpu bit from the mask. + * We reuse the call function data without waiting for any grace + * period after some other cpu removes it from the global queue. + * This means a cpu might find our data block as it is writen. + * The interrupt handler waits until it sees refs filled out + * while its cpu mask bit is set; here we may only clear our + * own cpu mask bit, and must wait to set refs until we are sure + * previous writes are complete and we have obtained the lock to + * add the element to the queue. We use the acquire and release + * of the lock as a wmb() -- acquire prevents write moving up and + * release requires old writes are visible. */ - smp_wmb(); - - atomic_set(&data->refs, cpumask_weight(data->cpumask)); - raw_spin_lock_irqsave(&call_function.lock, flags); /* * Place entry at the _HEAD_ of the list, so that any cpu still @@ -509,6 +511,8 @@ void smp_call_function_many(const struct list_add_rcu(&data->csd.list, &call_function.queue); raw_spin_unlock_irqrestore(&call_function.lock, flags); + atomic_set(&data->refs, cpumask_weight(data->cpumask)); + /* * Make the list addition visible before sending the ipi. * (IPIs must obey or appear to obey normal Linux cache
| |