workqueues: turn queue_work() into the "barrier" for work->func()

To clarify, I will be happy with the "no, we don't need this" comment.

But let's suppose we have

	int VAR;

	void work_func(struct work_struct *work)
	{
		if (VAR)
			do_something();
	}

and we are doing

	VAR = 1;
	queue_work(work);

I think the caller of queue_work() has all rights to expect that the next invocation of work_func() must see "VAR == 1", but this is not true if the work is already pending.

run_workqueue:

work_clear_pending(work)
clear_bit(WORK_STRUCT_PENDING) // no mb()

call work_func()
if (VAR)

it is possible that CPU reads VAR before before it clears _PENDING, and queue_work() "infiltrates" in between and fails. So we can miss an event.

I don't know if we really have such a code in kernel, and even if we have perhaps we should fix it and do not touch workqueues. But perhaps the current behaviour is a bit too subtle in this respect.

For example, atkbd_event_work() happens to work correctly, but only because it does mb() implicitly.

The patch merely adds mb() after work_clear_pending(work), another side already has the mb semantics implied by test_and_set_bit().

@@ -291,6 +291,12 @@ static void run_workqueue(struct cpu_wor
 
 		BUG_ON(get_wq_data(work) != cwq);
 		work_clear_pending(work);
+		/*
+		 * Ensure that either the concurrent queue_work() succeeds,
+		 * or work->func() sees all the preceding memory changes.
+		 */
+		smp_mb__after_clear_bit();
+
 		lock_map_acquire(&cwq->wq->lockdep_map);
 		lock_map_acquire(&lockdep_map);
 		f(work);

As you said, queue_work() does test_and_set_bit() which implies smp_mb() either side of the function, so you're half way there, and run_workqueue() calls spin_unlock_irq() just before calling work_clear_pending()... So might it make sense to move the work_clear_pending() into locked section? Or would that require an smp_mb__before_clear_bit()?

David

This can't really help, afaics. We still need mb() between clear_bit(_PENDING) and LOAD(VAR). Because unlock() is the "one way" barrier, LOAD(VAR) can leak into the critical section, and it can be re-ordered with clear_bit() inside the critical section.

Oleg.