workqueue: simplify is-work-item-queued-here test
Currently, determining whether a work item is queued on a locked pool
involves somewhat convoluted memory barrier dancing. It goes like the
following.
* When a work item is queued on a pool, work->data is updated before
work->entry is linked to the pending list with a wmb() inbetween.
* When trying to determine whether a work item is currently queued on
a pool pointed to by work->data, it locks the pool and looks at
work->entry. If work->entry is linked, we then do rmb() and then
check whether work->data points to the current pool.
This works because, work->data can only point to a pool if it
currently is or were on the pool and,
* If it currently is on the pool, the tests would obviously succeed.
* It it left the pool, its work->entry was cleared under pool->lock,
so if we're seeing non-empty work->entry, it has to be from the work
item being linked on another pool. Because work->data is updated
before work->entry is linked with wmb() inbetween, work->data update
from another pool is guaranteed to be visible if we do rmb() after
seeing non-empty work->entry. So, we either see empty work->entry
or we see updated work->data pointin to another pool.
While this works, it's convoluted, to put it mildly. With recent
updates, it's now guaranteed that work->data points to cwq only while
the work item is queued and that updating work->data to point to cwq
or back to pool is done under pool->lock, so we can simply test
whether work->data points to cwq which is associated with the
currently locked pool instead of the convoluted memory barrier
dancing.
This patch replaces the memory barrier based "are you still here,
really?" test with much simpler "does work->data points to me?" test -
if work->data points to a cwq which is associated with the currently
locked pool, the work item is guaranteed to be queued on the pool as
work->data can start and stop pointing to such cwq only under
pool->lock and the start and stop coincide with queue and dequeue.
tj: Rewrote the comments and description.
Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
Signed-off-by: Tejun Heo <tj@kernel.org>
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 1a442c3..251f009 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1068,6 +1068,7 @@
unsigned long *flags)
{
struct worker_pool *pool;
+ struct cpu_workqueue_struct *cwq;
local_irq_save(*flags);
@@ -1097,14 +1098,17 @@
goto fail;
spin_lock(&pool->lock);
- if (!list_empty(&work->entry)) {
- /*
- * This work is queued, but perhaps we locked the wrong
- * pool. In that case we must see the new value after
- * rmb(), see insert_work()->wmb().
- */
- smp_rmb();
- if (pool == get_work_pool(work)) {
+ /*
+ * work->data is guaranteed to point to cwq only while the work
+ * item is queued on cwq->wq, and both updating work->data to point
+ * to cwq on queueing and to pool on dequeueing are done under
+ * cwq->pool->lock. This in turn guarantees that, if work->data
+ * points to cwq which is associated with a locked pool, the work
+ * item is currently queued on that pool.
+ */
+ cwq = get_work_cwq(work);
+ if (cwq) {
+ if (cwq->pool == pool) {
debug_work_deactivate(work);
/*
@@ -1159,13 +1163,6 @@
/* we own @work, set data and link */
set_work_cwq(work, cwq, extra_flags);
-
- /*
- * Ensure that we get the right work->data if we see the
- * result of list_add() below, see try_to_grab_pending().
- */
- smp_wmb();
-
list_add_tail(&work->entry, head);
/*
@@ -2799,15 +2796,10 @@
return false;
spin_lock_irq(&pool->lock);
- if (!list_empty(&work->entry)) {
- /*
- * See the comment near try_to_grab_pending()->smp_rmb().
- * If it was re-queued to a different pool under us, we
- * are not going to wait.
- */
- smp_rmb();
- cwq = get_work_cwq(work);
- if (unlikely(!cwq || pool != cwq->pool))
+ /* see the comment in try_to_grab_pending() with the same code */
+ cwq = get_work_cwq(work);
+ if (cwq) {
+ if (unlikely(cwq->pool != pool))
goto already_gone;
} else {
worker = find_worker_executing_work(pool, work);