[PATCH] workqueue: remove the unneeded cpu_relax() in __queue_work()

[PATCH] workqueue: remove the unneeded cpu_relax() in __queue_work()

[Lai Jiangshan]

When pwq->refcnt == 0, the retrying is guaranteed to make forward-progress.
The comment above the code explains it well:

	/*
	 * pwq is determined and locked.  For unbound pools, we could have
	 * raced with pwq release and it could already be dead.  If its
	 * refcnt is zero, repeat pwq selection.  Note that pwqs never die
	 * without another pwq replacing it in the numa_pwq_tbl or while
	 * work items are executing on it, so the retrying is guaranteed to
	 * make forward-progress.
	 */

It means the cpu_relax() here is useless and sometimes misleading,
it should retry directly and make some progress rather than waste time.

Signed-off-by: Lai Jiangshan <laijs@cn.fujitsu.com>
---
 kernel/workqueue.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 23f9a2b..98b38b5 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -1368,7 +1368,6 @@ retry:
 	if (unlikely(!pwq->refcnt)) {
 		if (wq->flags & WQ_UNBOUND) {
 			spin_unlock(&pwq->pool->lock);
-			cpu_relax();
 			goto retry;
 		}
 		/* oops */
-- 
1.7.4.4

Re: [PATCH] workqueue: remove the unneeded cpu_relax() in __queue_work()

[Tejun Heo]

On Thu, May 22, 2014 at 04:44:16PM +0800, Lai Jiangshan wrote:
> When pwq->refcnt == 0, the retrying is guaranteed to make forward-progress.
> The comment above the code explains it well:
> 
> 	/*
> 	 * pwq is determined and locked.  For unbound pools, we could have
> 	 * raced with pwq release and it could already be dead.  If its
> 	 * refcnt is zero, repeat pwq selection.  Note that pwqs never die
> 	 * without another pwq replacing it in the numa_pwq_tbl or while
> 	 * work items are executing on it, so the retrying is guaranteed to
> 	 * make forward-progress.
> 	 */
> 
> It means the cpu_relax() here is useless and sometimes misleading,
> it should retry directly and make some progress rather than waste time.

cpu_relax() doesn't have much to do with guaranteeing forward
progress.  It's about giving a breather during busy wait so that the
waiting cpu doesn't busy loop claiming the same cache lines over and
over ultimately delaying the event being waited on.  If you're doing a
busy wait, you better use cpu_relax().

Thanks.

-- 
tejun

Re: [PATCH] workqueue: remove the unneeded cpu_relax() in __queue_work()

[Lai Jiangshan]

On Thu, May 22, 2014 at 9:47 PM, Tejun Heo <tj@kernel.org> wrote:
> On Thu, May 22, 2014 at 04:44:16PM +0800, Lai Jiangshan wrote:
>> When pwq->refcnt == 0, the retrying is guaranteed to make forward-progress.
>> The comment above the code explains it well:
>>
>>       /*
>>        * pwq is determined and locked.  For unbound pools, we could have
>>        * raced with pwq release and it could already be dead.  If its
>>        * refcnt is zero, repeat pwq selection.  Note that pwqs never die
>>        * without another pwq replacing it in the numa_pwq_tbl or while
>>        * work items are executing on it, so the retrying is guaranteed to
>>        * make forward-progress.
>>        */
>>
>> It means the cpu_relax() here is useless and sometimes misleading,
>> it should retry directly and make some progress rather than waste time.
>
> cpu_relax() doesn't have much to do with guaranteeing forward
> progress.  It's about giving a breather during busy wait so that the

This is not busy wait, the retry and numa_pwq_tbl() guarantee that
the retry will get a new pwq (even without cpu_relax()) as the comments says,
and the refcnt of this new pwq is very very likely non-zero and
cpu_relax() can't
increase the probability of non-zero-refcnt. cpu_relax() is useless here.

It is different from spin_lock() or some other spin code.

it is similar to the loop of __task_rq_lock() which also guarantees progress.

Thanks,
Lai

> waiting cpu doesn't busy loop claiming the same cache lines over and
> over ultimately delaying the event being waited on.  If you're doing a
> busy wait, you better use cpu_relax().
>
> Thanks.
>
> --
> tejun
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] workqueue: remove the unneeded cpu_relax() in __queue_work()

[Lai Jiangshan]

On 05/22/2014 10:21 PM, Lai Jiangshan wrote:
> On Thu, May 22, 2014 at 9:47 PM, Tejun Heo <tj@kernel.org> wrote:
>> On Thu, May 22, 2014 at 04:44:16PM +0800, Lai Jiangshan wrote:
>>> When pwq->refcnt == 0, the retrying is guaranteed to make forward-progress.
>>> The comment above the code explains it well:
>>>
>>>       /*
>>>        * pwq is determined and locked.  For unbound pools, we could have
>>>        * raced with pwq release and it could already be dead.  If its
>>>        * refcnt is zero, repeat pwq selection.  Note that pwqs never die
>>>        * without another pwq replacing it in the numa_pwq_tbl or while
>>>        * work items are executing on it, so the retrying is guaranteed to
>>>        * make forward-progress.
>>>        */
>>>
>>> It means the cpu_relax() here is useless and sometimes misleading,
>>> it should retry directly and make some progress rather than waste time.
>>
>> cpu_relax() doesn't have much to do with guaranteeing forward
>> progress.  It's about giving a breather during busy wait so that the
> 
> This is not busy wait, the retry and numa_pwq_tbl() guarantee that
> the retry will get a new pwq (even without cpu_relax()) as the comments says,
> and the refcnt of this new pwq is very very likely non-zero and
> cpu_relax() can't
> increase the probability of non-zero-refcnt. cpu_relax() is useless here.
> 
> It is different from spin_lock() or some other spin code.
> 
> it is similar to the loop of __task_rq_lock() which also guarantees progress.
> 
> Thanks,
> Lai

Ping.

Any comments?

> 
>> waiting cpu doesn't busy loop claiming the same cache lines over and
>> over ultimately delaying the event being waited on.  If you're doing a
>> busy wait, you better use cpu_relax().
>>
>> Thanks.
>>
>> --
>> tejun
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

Re: [PATCH] workqueue: remove the unneeded cpu_relax() in __queue_work()

[Tejun Heo]

On Thu, May 22, 2014 at 10:21:25PM +0800, Lai Jiangshan wrote:
> On Thu, May 22, 2014 at 9:47 PM, Tejun Heo <tj@kernel.org> wrote:
> This is not busy wait, the retry and numa_pwq_tbl() guarantee that
> the retry will get a new pwq (even without cpu_relax()) as the comments says,

Yes, *eventually*.  It's not guaranteed to succeed on the immediate
next try.  This is a busy wait.

> and the refcnt of this new pwq is very very likely non-zero and
> cpu_relax() can't
> increase the probability of non-zero-refcnt. cpu_relax() is useless here.
> 
> It is different from spin_lock() or some other spin code.
> 
> it is similar to the loop of __task_rq_lock() which also guarantees progress.

No, it's not.  __task_rq_lock() *already* sees the updated value to
use for the next time.  Here, we see the old one dead and the new one
is guaranteed to show up pretty soon but we're still busy waiting for
it.

-- 
tejun

Re: [PATCH] workqueue: remove the unneeded cpu_relax() in __queue_work()

[Lai Jiangshan]

On 05/26/2014 12:23 PM, Tejun Heo wrote:
> On Thu, May 22, 2014 at 10:21:25PM +0800, Lai Jiangshan wrote:
>> On Thu, May 22, 2014 at 9:47 PM, Tejun Heo <tj@kernel.org> wrote:
>> This is not busy wait, the retry and numa_pwq_tbl() guarantee that
>> the retry will get a new pwq (even without cpu_relax()) as the comments says,
> 
> Yes, *eventually*.  It's not guaranteed to succeed on the immediate
> next try.  This is a busy wait.



changing pwq:
	install pwq
	lock(pool->lock)
	put_pwq();
	unlock(pool->lock)

__queue_work():
	lock(pool->lock)
	test ref and find it zero;
	see the installation here;
	it is guaranteed to get the installed pwq on the immediate next try.
	unlock()
	retry.




> 
>> and the refcnt of this new pwq is very very likely non-zero and
>> cpu_relax() can't
>> increase the probability of non-zero-refcnt. cpu_relax() is useless here.
>>
>> It is different from spin_lock() or some other spin code.
>>
>> it is similar to the loop of __task_rq_lock() which also guarantees progress.
> 
> No, it's not.  __task_rq_lock() *already* sees the updated value to
> use for the next time.  Here, we see the old one dead and the new one
> is guaranteed to show up pretty soon but we're still busy waiting for
> it.
> 

Re: [PATCH] workqueue: remove the unneeded cpu_relax() in __queue_work()

[Tejun Heo]

On Mon, May 26, 2014 at 01:27:55PM +0800, Lai Jiangshan wrote:
> changing pwq:
> 	install pwq
> 	lock(pool->lock)
> 	put_pwq();
> 	unlock(pool->lock)
> 
> __queue_work():
> 	lock(pool->lock)
> 	test ref and find it zero;
> 	see the installation here;
> 	it is guaranteed to get the installed pwq on the immediate next try.
> 	unlock()
> 	retry.

The fact that pool->lock locking happens to provide enough barrier for
the above to work is an accidental implementation detail.  We
theoretically can move refcnting out of pool->lock.  Nothing
semantically guarantees that barrier to be there to interlock pwq
qinstallation and the last put.  Removing that cpu_relax() doesn't buy
us *ANYTHING* and removing that with rationale of making it go faster
would easily win pointless micro optimization award of the year.  Just
let it go.

-- 
tejun