Re: [PATCH] workqueue: remove the unneeded cpu_relax() in __queue_work()
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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()
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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()
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 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. > -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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()
On Thu, May 22, 2014 at 10:21:25PM +0800, Lai Jiangshan wrote: > On Thu, May 22, 2014 at 9:47 PM, Tejun Heo 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@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()
On 05/22/2014 10:21 PM, Lai Jiangshan wrote: > On Thu, May 22, 2014 at 9:47 PM, Tejun Heo 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 majord...@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 majord...@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 majord...@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()
On 05/22/2014 10:21 PM, Lai Jiangshan wrote: On Thu, May 22, 2014 at 9:47 PM, Tejun Heo t...@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 majord...@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 majord...@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 majord...@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()
On Thu, May 22, 2014 at 10:21:25PM +0800, Lai Jiangshan wrote: On Thu, May 22, 2014 at 9:47 PM, Tejun Heo t...@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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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()
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 t...@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. -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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()
On Thu, May 22, 2014 at 9:47 PM, Tejun Heo 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 majord...@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 majord...@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()
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH] workqueue: remove the unneeded cpu_relax() in __queue_work()
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
---
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(>pool->lock);
- cpu_relax();
goto retry;
}
/* oops */
--
1.7.4.4
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH] workqueue: remove the unneeded cpu_relax() in __queue_work()
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 la...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@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()
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@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()
On Thu, May 22, 2014 at 9:47 PM, Tejun Heo t...@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 majord...@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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
