[PATCH 2/4 v3] call_function_many: add missing ordering

[Milton Miller]

Paul McKenney's review pointed out two problems with the barriers
in the 2.6.38 update to the smp call function many code.

First, a barrier that would force the func and info members of data
to be visible before their consumption in the interrupt handler
was missing.  This can be solved by adding a smp_wmb between
setting the func and info members and setting setting the cpumask;
this will pair with the existing and required smp_rmb ordering
the cpumask read before the read of refs.  This placement avoids
the need a second smp_rmb in the interrupt handler which would
be executed on each of the N cpus executing the call request.
(I was thinking this barrier was present but was not).

Second, the previous write to refs (establishing the zero that
we the interrupt handler was testing from all cpus) was performed
by a third party cpu.  This would invoke transitivity which, as
a recient or concurrent addition to memory-barriers.txt now
explicitly states, would require a full smp_mb().

However, we know the cpumask will only be set by one cpu (the
data owner) and any preivous iteration of the mask would have
cleared by the reading cpu.  By redundantly writing refs to 0 on
the owning cpu before the smp_wmb, the write to refs will follow
the same path as the writes that set the cpumask, which in turn
allows us to keep the barrier in the interrupt handler a smp_rmb
instead of promoting it to a smp_mb (which will be be executed
by N cpus for each of the possible M elements on the list).

I moved and expanded the comment about our (ab)use of the rcu list
primitives for the concurrent walk earlier into this function.
I considered moving the first two paragraphs to the queue list
head and lock, but felt it would have been too disconected from
the code.

Cc: Paul McKinney <paulmck@xxxxxxxxxxxxxxxxxx>
Cc: stable (2.6.32 and later)
Signed-off-by: Milton Miller <miltonm@xxxxxxx>

Paul, please review this alternative to your patch at either of

http://marc.info/?l=linux-kernel&m=129662029916241&w=2
https://patchwork.kernel.org/patch/525891/

In contrast to your patch, this proposal keeps the additional
barriers in the (interrupt enabled) requester instead of the
execution interrupt, where they would be executed by all N cpus
in the system triggered concurrently for each of the N possible
elements in the list.

Index: common/kernel/smp.c
===================================================================
--- common.orig/kernel/smp.c	2011-03-15 05:21:41.000000000 -0500
+++ common/kernel/smp.c	2011-03-15 05:22:26.000000000 -0500
@@ -483,23 +483,42 @@ void smp_call_function_many(const struct
 
 	data = &__get_cpu_var(cfd_data);
 	csd_lock(&data->csd);
-	BUG_ON(atomic_read(&data->refs) || !cpumask_empty(data->cpumask));
 
-	data->csd.func = func;
-	data->csd.info = info;
-	cpumask_and(data->cpumask, mask, cpu_online_mask);
-	cpumask_clear_cpu(this_cpu, data->cpumask);
+	/* This BUG_ON verifies our reuse assertions and can be removed */
+	BUG_ON(atomic_read(&data->refs) || !cpumask_empty(data->cpumask));
 
 	/*
+	 * The global call function queue list add and delete are protected
+	 * by a lock, but the list is traversed without any lock, relying
+	 * on the rcu list add and delete to allow safe concurrent traversal.
 	 * 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.
+	 * This means a cpu might find our data block as it is being
+	 * filled out.
+	 *
+	 * We hold off the interrupt handler on the other cpu by
+	 * ordering our writes to the cpu mask vs our setting of the
+	 * refs counter.  We assert only the cpu owning the data block
+	 * will set a bit in cpumask, and each bit will only be cleared
+	 * by the subject cpu.  Each cpu must first find its bit is
+	 * set and then check that refs is set indicating the element is
+	 * ready to be processed, otherwise it must skip the entry.
+	 *
+	 * On the previous iteration refs was set to 0 by another cpu.
+	 * To avoid the use of transitivity, set the counter to 0 here
+	 * so the wmb will pair with the rmb in the interrupt handler.
 	 */
+	atomic_set(&data->refs, 0);	/* convert 3rd to 1st party write */
+
+	data->csd.func = func;
+	data->csd.info = info;
+
+	/* Ensure 0 refs is visible before mask.  Also orders func and info */
+	smp_wmb();
+
+	/* We rely on the "and" being processed before the store */
+	cpumask_and(data->cpumask, mask, cpu_online_mask);
+	cpumask_clear_cpu(this_cpu, data->cpumask);
 
 	raw_spin_lock_irqsave(&call_function.lock, flags);
 	/*
@@ -509,8 +528,9 @@ void smp_call_function_many(const struct
 	 */
 	list_add_rcu(&data->csd.list, &call_function.queue);
 	/*
-	 * We rely on the wmb() in list_add_rcu to order the writes
-	 * to func, data, and cpumask before this write to refs.
+	 * We rely on the wmb() in list_add_rcu to complete our writes
+	 * to the cpumask before this write to refs, which indicates
+	 * data is on the list and is ready to be processed.
 	 */
 	atomic_set(&data->refs, cpumask_weight(data->cpumask));
 	raw_spin_unlock_irqrestore(&call_function.lock, flags);
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/