Re: [PATCH 1/2 V3] workqueue: substitute POOL_FREEZING with __WQ_FREEZING
On 04/23/2014 04:46 AM, Tejun Heo wrote: > Hello, > > On Tue, Apr 22, 2014 at 09:47:47AM +0800, Lai Jiangshan wrote: >> Testing workqueue_freezing requires wq_pool_mutex held. >> Although almost-all pwq_adjust_max_active() are called with wq_pool_mutex >> held, >> except workqueue_set_max_active(). But I hope pwq_adjust_max_active() >> don't require the heavy wq_pool_mutex. > > No it doesn't require wq_pool_mutex to be held. All it requires is > that the changed state is visible on the subsequent > pwq_adjust_max_active() invocatino which is already trivially > guaranteed. > Good! I understood! Could you respin the patch? I'm afraid I can't explain it well in the comments. For me, I always prefer locks for non-performance critical path, locks help review, I believe your comment will do so. Thanks, Lai -- 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 1/2 V3] workqueue: substitute POOL_FREEZING with __WQ_FREEZING
Hello, On Tue, Apr 22, 2014 at 09:47:47AM +0800, Lai Jiangshan wrote: > Testing workqueue_freezing requires wq_pool_mutex held. > Although almost-all pwq_adjust_max_active() are called with wq_pool_mutex > held, > except workqueue_set_max_active(). But I hope pwq_adjust_max_active() > don't require the heavy wq_pool_mutex. No it doesn't require wq_pool_mutex to be held. All it requires is that the changed state is visible on the subsequent pwq_adjust_max_active() invocatino which is already trivially guaranteed. 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 1/2 V3] workqueue: substitute POOL_FREEZING with __WQ_FREEZING
On 04/22/2014 06:20 AM, Tejun Heo wrote: > On Mon, Apr 21, 2014 at 07:59:20PM +0800, Lai Jiangshan wrote: >> Only workqueues have freezable or freezing attribution/state, not worker >> pools. >> But POOL_FREEZING adds a suspicious state and makes reviewers confused. >> >> And it causes freeze_workqueues_begin() and thaw_workqueues() much >> complicated, >> they need to travel all the pools besides wqs. >> >> Since freezable is workqueue instance's attribution, and freezing >> is workqueue instance's state, so we introduce __WQ_FREEZING >> to wq->flags instead and remove POOL_FREEZING. >> >> It is different from POOL_FREEZING, POOL_FREEZING is simply set >> all over the world(all pools), while __WQ_FREEZING is only set for freezable >> wq. >> freeze_workqueues_begin()/thaw_workqueues() skip to handle non-freezable wqs >> and don't touch the non-freezable wqs' flags. > > I was about to apply the patch and have updated the patch description. > > While freezing takes place globally, its execution is per-workqueue; > however, the current implementation makes use of the per-worker_pool > POOL_FREEZING flag. While it's not broken, the flag makes the code > more confusing and complicates freeze_workqueues_begin() and > thaw_workqueues() by requiring them to walk through all pools. > > Since freezable is a workqueue's attribute, and freezing is a > workqueue's state, let's introduce __WQ_FREEZING to wq->flags instead > and remove POOL_FREEZING. > > It is different from POOL_FREEZING in that __WQ_FREEZING is only set > for freezable workqueues while POOL_FREEZING is set globally over all > pools. freeze_workqueues_begin() and thaw_workqueues() now skip > non-freezable workqueues. > > But looking at the patch, why do we need __WQ_FREEZING at all? We > should be able to test workqueue_freezing in pwq_adjust_max_active(), > right? The only requirement there would be that Testing workqueue_freezing requires wq_pool_mutex held. Although almost-all pwq_adjust_max_active() are called with wq_pool_mutex held, except workqueue_set_max_active(). But I hope pwq_adjust_max_active() don't require the heavy wq_pool_mutex. > pwq_adjust_max_active() is invoked at least once after > workqueue_freezing is changed, which is already guaranteed. > > Thanks. > -- 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 1/2 V3] workqueue: substitute POOL_FREEZING with __WQ_FREEZING
On Mon, Apr 21, 2014 at 07:59:20PM +0800, Lai Jiangshan wrote: > Only workqueues have freezable or freezing attribution/state, not worker > pools. > But POOL_FREEZING adds a suspicious state and makes reviewers confused. > > And it causes freeze_workqueues_begin() and thaw_workqueues() much > complicated, > they need to travel all the pools besides wqs. > > Since freezable is workqueue instance's attribution, and freezing > is workqueue instance's state, so we introduce __WQ_FREEZING > to wq->flags instead and remove POOL_FREEZING. > > It is different from POOL_FREEZING, POOL_FREEZING is simply set > all over the world(all pools), while __WQ_FREEZING is only set for freezable > wq. > freeze_workqueues_begin()/thaw_workqueues() skip to handle non-freezable wqs > and don't touch the non-freezable wqs' flags. I was about to apply the patch and have updated the patch description. While freezing takes place globally, its execution is per-workqueue; however, the current implementation makes use of the per-worker_pool POOL_FREEZING flag. While it's not broken, the flag makes the code more confusing and complicates freeze_workqueues_begin() and thaw_workqueues() by requiring them to walk through all pools. Since freezable is a workqueue's attribute, and freezing is a workqueue's state, let's introduce __WQ_FREEZING to wq->flags instead and remove POOL_FREEZING. It is different from POOL_FREEZING in that __WQ_FREEZING is only set for freezable workqueues while POOL_FREEZING is set globally over all pools. freeze_workqueues_begin() and thaw_workqueues() now skip non-freezable workqueues. But looking at the patch, why do we need __WQ_FREEZING at all? We should be able to test workqueue_freezing in pwq_adjust_max_active(), right? The only requirement there would be that pwq_adjust_max_active(0 is invoked at least once after workqueue_freezing is changed, which is already guaranteed. 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 1/2 V3] workqueue: substitute POOL_FREEZING with __WQ_FREEZING
Only workqueues have freezable or freezing attribution/state, not worker pools.
But POOL_FREEZING adds a suspicious state and makes reviewers confused.
And it causes freeze_workqueues_begin() and thaw_workqueues() much complicated,
they need to travel all the pools besides wqs.
Since freezable is workqueue instance's attribution, and freezing
is workqueue instance's state, so we introduce __WQ_FREEZING
to wq->flags instead and remove POOL_FREEZING.
It is different from POOL_FREEZING, POOL_FREEZING is simply set
all over the world(all pools), while __WQ_FREEZING is only set for freezable wq.
freeze_workqueues_begin()/thaw_workqueues() skip to handle non-freezable wqs
and don't touch the non-freezable wqs' flags.
Signed-off-by: Lai Jiangshan
---
include/linux/workqueue.h |1 +
kernel/workqueue.c| 38 --
2 files changed, 13 insertions(+), 26 deletions(-)
diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 1b22c42..6bf353e 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -322,6 +322,7 @@ enum {
__WQ_DRAINING = 1 << 16, /* internal: workqueue is draining */
__WQ_ORDERED= 1 << 17, /* internal: workqueue is ordered */
+ __WQ_FREEZING = 1 << 18, /* internel: workqueue is freezing */
WQ_MAX_ACTIVE = 512,/* I like 512, better ideas? */
WQ_MAX_UNBOUND_PER_CPU = 4, /* 4 * #cpus for unbound wq */
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index c3f076f..beca98b 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -70,7 +70,6 @@ enum {
*/
POOL_MANAGE_WORKERS = 1 << 0, /* need to manage workers */
POOL_DISASSOCIATED = 1 << 2, /* cpu can't serve workers */
- POOL_FREEZING = 1 << 3, /* freeze in progress */
/* worker flags */
WORKER_STARTED = 1 << 0, /* started */
@@ -3662,9 +3661,6 @@ static struct worker_pool *get_unbound_pool(const struct
workqueue_attrs *attrs)
if (!pool || init_worker_pool(pool) < 0)
goto fail;
- if (workqueue_freezing)
- pool->flags |= POOL_FREEZING;
-
lockdep_set_subclass(&pool->lock, 1); /* see put_pwq() */
copy_workqueue_attrs(pool->attrs, attrs);
@@ -3762,7 +3758,7 @@ static void pwq_adjust_max_active(struct pool_workqueue
*pwq)
struct workqueue_struct *wq = pwq->wq;
bool freezable = wq->flags & WQ_FREEZABLE;
- /* for @wq->saved_max_active */
+ /* for @wq->saved_max_active and @wq->flags */
lockdep_assert_held(&wq->mutex);
/* fast exit for non-freezable wqs */
@@ -3771,7 +3767,7 @@ static void pwq_adjust_max_active(struct pool_workqueue
*pwq)
spin_lock_irq(&pwq->pool->lock);
- if (!freezable || !(pwq->pool->flags & POOL_FREEZING)) {
+ if (!freezable || !(wq->flags & __WQ_FREEZING)) {
pwq->max_active = wq->saved_max_active;
while (!list_empty(&pwq->delayed_works) &&
@@ -4277,6 +4273,8 @@ struct workqueue_struct *__alloc_workqueue_key(const char
*fmt,
mutex_lock(&wq_pool_mutex);
mutex_lock(&wq->mutex);
+ if ((wq->flags & WQ_FREEZABLE) && workqueue_freezing)
+ wq->flags |= __WQ_FREEZING;
for_each_pwq(pwq, wq)
pwq_adjust_max_active(pwq);
mutex_unlock(&wq->mutex);
@@ -4883,26 +4881,20 @@ EXPORT_SYMBOL_GPL(work_on_cpu);
*/
void freeze_workqueues_begin(void)
{
- struct worker_pool *pool;
struct workqueue_struct *wq;
struct pool_workqueue *pwq;
- int pi;
mutex_lock(&wq_pool_mutex);
WARN_ON_ONCE(workqueue_freezing);
workqueue_freezing = true;
- /* set FREEZING */
- for_each_pool(pool, pi) {
- spin_lock_irq(&pool->lock);
- WARN_ON_ONCE(pool->flags & POOL_FREEZING);
- pool->flags |= POOL_FREEZING;
- spin_unlock_irq(&pool->lock);
- }
-
list_for_each_entry(wq, &workqueues, list) {
+ if (!(wq->flags & WQ_FREEZABLE))
+ continue;
mutex_lock(&wq->mutex);
+ WARN_ON_ONCE(wq->flags & __WQ_FREEZING);
+ wq->flags |= __WQ_FREEZING;
for_each_pwq(pwq, wq)
pwq_adjust_max_active(pwq);
mutex_unlock(&wq->mutex);
@@ -4970,25 +4962,19 @@ void thaw_workqueues(void)
{
struct workqueue_struct *wq;
struct pool_workqueue *pwq;
- struct worker_pool *pool;
- int pi;
mutex_lock(&wq_pool_mutex);
if (!workqueue_freezing)
goto out_unlock;
- /* clear FREEZING */
- for_each_pool(pool, pi) {
- spin_lock_irq(&pool->lock);
- WARN_ON_ONCE(!(pool->flags & POOL_FREEZING));
- pool->flags &= ~POOL_FREEZI
