On 02/22/2014 09:40 AM, Tejun Heo wrote:
On Sat, Feb 22, 2014 at 07:11:51AM -0500, Peter Hurley wrote:
Users of the workqueue api may assume the workqueue provides a
memory ordering guarantee for re-queued work items; ie., that
if a work item is not queue-able then the previously queued
work instance is not running and so any memory operations
which occur before queuing the work will be visible to the work
function.
For example, code of the form:
add new data to work on
queue work
assumes that this latest data is acted upon, either by the
newly queued instance (if it could be queued) or by the not-yet-
running instance (if a new instance could not be queued).
Provide this implicit memory ordering guarantee; prevent
speculative loads in the work function from occurring before
the current work instance is marked not pending (and thus has
started). This, in turn, guarantees that stores occurring before
schedule_work/queue_work are visible to either the not-yet-running
work instance (if new work could not be queued) or that new work
is queued (and thus ensuring the new data is acted upon).
Note that preventing early stores is unnecessary because no
conclusion can be reached about the state of the work function
from outside the work function by ordering early stores after
clearing PENDING (other than testing PENDING).
Signed-off-by: Peter Hurley <pe...@hurleysoftware.com>
---
kernel/workqueue.c | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 82ef9f3..a4b241d 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -2201,6 +2201,16 @@ __acquires(&pool->lock)
spin_unlock_irq(&pool->lock);
+ /*
+ * Paired with the implied mb of test_and_set_bit(PENDING).
+ * Guarantees speculative loads which occur in the work item
+ * function do not complete before PENDING is cleared in
+ * set_work_pool_and_clear_pending() above. In turn, this
+ * ensures that stores are either visible to the not-yet-
+ * running work instance or a new instance is queueable.
+ */
+ smp_rmb();
Wouldn't it make more sense to have the above right after
clear_pending?
I didn't want to affect any optimization that the cpu might
accomplish regarding the unlock. In fact, I considered using it
right before calling through worker->current_func.
Given your concerns about the performance impact, maybe we should
ask Fengguang to run this change through his automated test suites
to find out what the perf delta is?
Regards,
Peter Hurley
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
