[PATCH] workqueue: Guarantee work function memory ordering

[PATCH] workqueue: Guarantee work function memory ordering

[Peter Hurley]

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 <peter@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();
+
 	lock_map_acquire_read(&pwq->wq->lockdep_map);
 	lock_map_acquire(&lockdep_map);
 	trace_workqueue_execute_start(work);
-- 
1.8.1.2

Re: [PATCH] workqueue: Guarantee work function memory ordering

[Tejun Heo]

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 <peter@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?

Thanks.

-- 
tejun

Re: [PATCH] workqueue: Guarantee work function memory ordering

[Peter Hurley]

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 <peter@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

Re: [PATCH] workqueue: Guarantee work function memory ordering

[Tejun Heo]

Hello, Peter.

On Sat, Feb 22, 2014 at 10:05:53AM -0500, Peter Hurley wrote:
> 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?

It should be fine.  It's more like I just didn't like the idea of it.
Workqueue execution path isn't really light enough for an addition of
single smp_rmb() per work item execution makes any noticeable
difference.  No harm in measuring perf delta but I'm highly skeptical
that there will be any difference which can be measured.

Thanks.

-- 
tejun