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

[Peter Hurley <peter@hurleysoftware.com>]

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