From: Lai Jiangshan <laijs@cn.fujitsu.com>
To: Tejun Heo <tj@kernel.org>
Cc: linux-kernel@vger.kernel.org, axboe@kernel.dk, jmoyer@redhat.com,
zab@redhat.com
Subject: Re: [PATCH 12/31] workqueue: update synchronization rules on workqueue->pwqs
Date: Sun, 10 Mar 2013 18:09:28 +0800 [thread overview]
Message-ID: <513C5BD8.9050909@cn.fujitsu.com> (raw)
In-Reply-To: <1362194662-2344-13-git-send-email-tj@kernel.org>
On 02/03/13 11:24, Tejun Heo wrote:
> Make workqueue->pwqs protected by workqueue_lock for writes and
> sched-RCU protected for reads. Lockdep assertions are added to
> for_each_pwq() and first_pwq() and all their users are converted to
> either hold workqueue_lock or disable preemption/irq.
>
> alloc_and_link_pwqs() is updated to use list_add_tail_rcu() for
> consistency which isn't strictly necessary as the workqueue isn't
> visible. destroy_workqueue() isn't updated to sched-RCU release pwqs.
> This is okay as the workqueue should have on users left by that point.
>
> The locking is superflous at this point. This is to help
> implementation of unbound pools/pwqs with custom attributes.
>
> This patch doesn't introduce any behavior changes.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> ---
> kernel/workqueue.c | 85 +++++++++++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 68 insertions(+), 17 deletions(-)
>
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 02f51b8..ff51c59 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -42,6 +42,7 @@
> #include <linux/lockdep.h>
> #include <linux/idr.h>
> #include <linux/hashtable.h>
> +#include <linux/rculist.h>
>
> #include "workqueue_internal.h"
>
> @@ -118,6 +119,8 @@ enum {
> * F: wq->flush_mutex protected.
> *
> * W: workqueue_lock protected.
> + *
> + * R: workqueue_lock protected for writes. Sched-RCU protected for reads.
> */
>
> /* struct worker is defined in workqueue_internal.h */
> @@ -169,7 +172,7 @@ struct pool_workqueue {
> int nr_active; /* L: nr of active works */
> int max_active; /* L: max active works */
> struct list_head delayed_works; /* L: delayed works */
> - struct list_head pwqs_node; /* I: node on wq->pwqs */
> + struct list_head pwqs_node; /* R: node on wq->pwqs */
> struct list_head mayday_node; /* W: node on wq->maydays */
> } __aligned(1 << WORK_STRUCT_FLAG_BITS);
>
> @@ -189,7 +192,7 @@ struct wq_flusher {
> struct workqueue_struct {
> unsigned int flags; /* W: WQ_* flags */
> struct pool_workqueue __percpu *cpu_pwqs; /* I: per-cpu pwq's */
> - struct list_head pwqs; /* I: all pwqs of this wq */
> + struct list_head pwqs; /* R: all pwqs of this wq */
> struct list_head list; /* W: list of all workqueues */
>
> struct mutex flush_mutex; /* protects wq flushing */
> @@ -227,6 +230,11 @@ EXPORT_SYMBOL_GPL(system_freezable_wq);
> #define CREATE_TRACE_POINTS
> #include <trace/events/workqueue.h>
>
> +#define assert_rcu_or_wq_lock() \
> + rcu_lockdep_assert(rcu_read_lock_sched_held() || \
> + lockdep_is_held(&workqueue_lock), \
> + "sched RCU or workqueue lock should be held")
> +
> #define for_each_std_worker_pool(pool, cpu) \
> for ((pool) = &std_worker_pools(cpu)[0]; \
> (pool) < &std_worker_pools(cpu)[NR_STD_WORKER_POOLS]; (pool)++)
> @@ -282,9 +290,16 @@ static inline int __next_wq_cpu(int cpu, const struct cpumask *mask,
> * for_each_pwq - iterate through all pool_workqueues of the specified workqueue
> * @pwq: iteration cursor
> * @wq: the target workqueue
> + *
> + * This must be called either with workqueue_lock held or sched RCU read
> + * locked. If the pwq needs to be used beyond the locking in effect, the
> + * caller is responsible for guaranteeing that the pwq stays online.
> + *
> + * The if clause exists only for the lockdep assertion and can be ignored.
> */
> #define for_each_pwq(pwq, wq) \
> - list_for_each_entry((pwq), &(wq)->pwqs, pwqs_node)
> + list_for_each_entry_rcu((pwq), &(wq)->pwqs, pwqs_node) \
> + if (({ assert_rcu_or_wq_lock(); true; }))
Aware this:
if (somecondition)
for_each_pwq(pwq, wq)
one_statement;q
else
xxxxx;
for_each_pwq() will eat the else.
To avoid this, you can use:
#define for_each_pwq(pwq, wq) \
list_for_each_entry_rcu((pwq), &(wq)->pwqs, pwqs_node) \
if (({ assert_rcu_or_wq_lock(); false; })) { } \
else
The same for for_each_pool() in later patch.
>
> #ifdef CONFIG_DEBUG_OBJECTS_WORK
>
> @@ -463,9 +478,19 @@ static struct worker_pool *get_std_worker_pool(int cpu, bool highpri)
> return &pools[highpri];
> }
>
> +/**
> + * first_pwq - return the first pool_workqueue of the specified workqueue
> + * @wq: the target workqueue
> + *
> + * This must be called either with workqueue_lock held or sched RCU read
> + * locked. If the pwq needs to be used beyond the locking in effect, the
> + * caller is responsible for guaranteeing that the pwq stays online.
> + */
> static struct pool_workqueue *first_pwq(struct workqueue_struct *wq)
> {
> - return list_first_entry(&wq->pwqs, struct pool_workqueue, pwqs_node);
> + assert_rcu_or_wq_lock();
> + return list_first_or_null_rcu(&wq->pwqs, struct pool_workqueue,
> + pwqs_node);
> }
>
> static unsigned int work_color_to_flags(int color)
> @@ -2488,10 +2513,12 @@ static bool flush_workqueue_prep_pwqs(struct workqueue_struct *wq,
> atomic_set(&wq->nr_pwqs_to_flush, 1);
> }
>
> + local_irq_disable();
> +
> for_each_pwq(pwq, wq) {
> struct worker_pool *pool = pwq->pool;
>
> - spin_lock_irq(&pool->lock);
> + spin_lock(&pool->lock);
>
> if (flush_color >= 0) {
> WARN_ON_ONCE(pwq->flush_color != -1);
> @@ -2508,9 +2535,11 @@ static bool flush_workqueue_prep_pwqs(struct workqueue_struct *wq,
> pwq->work_color = work_color;
> }
>
> - spin_unlock_irq(&pool->lock);
> + spin_unlock(&pool->lock);
> }
>
> + local_irq_enable();
> +
> if (flush_color >= 0 && atomic_dec_and_test(&wq->nr_pwqs_to_flush))
> complete(&wq->first_flusher->done);
>
> @@ -2701,12 +2730,14 @@ void drain_workqueue(struct workqueue_struct *wq)
> reflush:
> flush_workqueue(wq);
>
> + local_irq_disable();
> +
> for_each_pwq(pwq, wq) {
> bool drained;
>
> - spin_lock_irq(&pwq->pool->lock);
> + spin_lock(&pwq->pool->lock);
> drained = !pwq->nr_active && list_empty(&pwq->delayed_works);
> - spin_unlock_irq(&pwq->pool->lock);
> + spin_unlock(&pwq->pool->lock);
>
> if (drained)
> continue;
> @@ -2715,13 +2746,17 @@ reflush:
> (flush_cnt % 100 == 0 && flush_cnt <= 1000))
> pr_warn("workqueue %s: flush on destruction isn't complete after %u tries\n",
> wq->name, flush_cnt);
> +
> + local_irq_enable();
> goto reflush;
> }
>
> - spin_lock_irq(&workqueue_lock);
> + spin_lock(&workqueue_lock);
> if (!--wq->nr_drainers)
> wq->flags &= ~WQ_DRAINING;
> - spin_unlock_irq(&workqueue_lock);
> + spin_unlock(&workqueue_lock);
> +
> + local_irq_enable();
> }
> EXPORT_SYMBOL_GPL(drain_workqueue);
>
> @@ -3087,7 +3122,7 @@ static int alloc_and_link_pwqs(struct workqueue_struct *wq)
> per_cpu_ptr(wq->cpu_pwqs, cpu);
>
> pwq->pool = get_std_worker_pool(cpu, highpri);
> - list_add_tail(&pwq->pwqs_node, &wq->pwqs);
> + list_add_tail_rcu(&pwq->pwqs_node, &wq->pwqs);
> }
> } else {
> struct pool_workqueue *pwq;
> @@ -3097,7 +3132,7 @@ static int alloc_and_link_pwqs(struct workqueue_struct *wq)
> return -ENOMEM;
>
> pwq->pool = get_std_worker_pool(WORK_CPU_UNBOUND, highpri);
> - list_add_tail(&pwq->pwqs_node, &wq->pwqs);
> + list_add_tail_rcu(&pwq->pwqs_node, &wq->pwqs);
> }
>
> return 0;
> @@ -3174,6 +3209,7 @@ struct workqueue_struct *__alloc_workqueue_key(const char *fmt,
> if (alloc_and_link_pwqs(wq) < 0)
> goto err;
>
> + local_irq_disable();
> for_each_pwq(pwq, wq) {
> BUG_ON((unsigned long)pwq & WORK_STRUCT_FLAG_MASK);
> pwq->wq = wq;
> @@ -3182,6 +3218,7 @@ struct workqueue_struct *__alloc_workqueue_key(const char *fmt,
> INIT_LIST_HEAD(&pwq->delayed_works);
> INIT_LIST_HEAD(&pwq->mayday_node);
> }
> + local_irq_enable();
>
> if (flags & WQ_RESCUER) {
> struct worker *rescuer;
> @@ -3239,24 +3276,32 @@ void destroy_workqueue(struct workqueue_struct *wq)
> /* drain it before proceeding with destruction */
> drain_workqueue(wq);
>
> + spin_lock_irq(&workqueue_lock);
> +
> /* sanity checks */
> for_each_pwq(pwq, wq) {
> int i;
>
> - for (i = 0; i < WORK_NR_COLORS; i++)
> - if (WARN_ON(pwq->nr_in_flight[i]))
> + for (i = 0; i < WORK_NR_COLORS; i++) {
> + if (WARN_ON(pwq->nr_in_flight[i])) {
> + spin_unlock_irq(&workqueue_lock);
> return;
> + }
> + }
> +
> if (WARN_ON(pwq->nr_active) ||
> - WARN_ON(!list_empty(&pwq->delayed_works)))
> + WARN_ON(!list_empty(&pwq->delayed_works))) {
> + spin_unlock_irq(&workqueue_lock);
> return;
> + }
> }
>
> /*
> * wq list is used to freeze wq, remove from list after
> * flushing is complete in case freeze races us.
> */
> - spin_lock_irq(&workqueue_lock);
> list_del(&wq->list);
> +
> spin_unlock_irq(&workqueue_lock);
>
> if (wq->flags & WQ_RESCUER) {
> @@ -3340,13 +3385,19 @@ EXPORT_SYMBOL_GPL(workqueue_set_max_active);
> bool workqueue_congested(int cpu, struct workqueue_struct *wq)
> {
> struct pool_workqueue *pwq;
> + bool ret;
> +
> + preempt_disable();
>
> if (!(wq->flags & WQ_UNBOUND))
> pwq = per_cpu_ptr(wq->cpu_pwqs, cpu);
> else
> pwq = first_pwq(wq);
>
> - return !list_empty(&pwq->delayed_works);
> + ret = !list_empty(&pwq->delayed_works);
> + preempt_enable();
> +
> + return ret;
> }
> EXPORT_SYMBOL_GPL(workqueue_congested);
>
next prev parent reply other threads:[~2013-03-10 10:07 UTC|newest]
Thread overview: 77+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-03-02 3:23 [PATCHSET wq/for-3.10-tmp] workqueue: implement workqueue with custom worker attributes Tejun Heo
2013-03-02 3:23 ` [PATCH 01/31] workqueue: make sanity checks less punshing using WARN_ON[_ONCE]()s Tejun Heo
2013-03-02 3:23 ` [PATCH 02/31] workqueue: make workqueue_lock irq-safe Tejun Heo
2013-03-02 3:23 ` [PATCH 03/31] workqueue: introduce kmem_cache for pool_workqueues Tejun Heo
2013-03-02 3:23 ` [PATCH 04/31] workqueue: add workqueue_struct->pwqs list Tejun Heo
2013-03-02 3:23 ` [PATCH 05/31] workqueue: replace for_each_pwq_cpu() with for_each_pwq() Tejun Heo
2013-03-02 3:23 ` [PATCH 06/31] workqueue: introduce for_each_pool() Tejun Heo
2013-03-02 3:23 ` [PATCH 07/31] workqueue: restructure pool / pool_workqueue iterations in freeze/thaw functions Tejun Heo
2013-03-10 10:09 ` Lai Jiangshan
2013-03-10 12:34 ` Tejun Heo
2013-03-02 3:23 ` [PATCH 08/31] workqueue: add wokrqueue_struct->maydays list to replace mayday cpu iterators Tejun Heo
2013-03-02 3:24 ` [PATCH 09/31] workqueue: consistently use int for @cpu variables Tejun Heo
2013-03-02 3:24 ` [PATCH 10/31] workqueue: remove workqueue_struct->pool_wq.single Tejun Heo
2013-03-02 3:24 ` [PATCH 11/31] workqueue: replace get_pwq() with explicit per_cpu_ptr() accesses and first_pwq() Tejun Heo
2013-03-02 3:24 ` [PATCH 12/31] workqueue: update synchronization rules on workqueue->pwqs Tejun Heo
2013-03-10 10:09 ` Lai Jiangshan [this message]
2013-03-10 12:38 ` Tejun Heo
2013-03-12 18:20 ` [PATCH v2 " Tejun Heo
2013-03-02 3:24 ` [PATCH 13/31] workqueue: update synchronization rules on worker_pool_idr Tejun Heo
2013-03-12 18:20 ` [PATCH v2 " Tejun Heo
2013-03-02 3:24 ` [PATCH 14/31] workqueue: replace POOL_MANAGING_WORKERS flag with worker_pool->manager_mutex Tejun Heo
2013-03-10 10:09 ` Lai Jiangshan
2013-03-10 12:46 ` Tejun Heo
2013-03-12 18:19 ` [PATCH v2 " Tejun Heo
2013-03-02 3:24 ` [PATCH 15/31] workqueue: separate out init_worker_pool() from init_workqueues() Tejun Heo
2013-03-02 3:24 ` [PATCH 16/31] workqueue: introduce workqueue_attrs Tejun Heo
2013-03-04 18:37 ` [PATCH v2 " Tejun Heo
2013-03-05 22:29 ` Ryan Mallon
2013-03-05 22:33 ` Tejun Heo
2013-03-05 22:34 ` Tejun Heo
2013-03-05 22:40 ` Ryan Mallon
2013-03-05 22:44 ` Tejun Heo
2013-03-05 23:20 ` Ryan Mallon
2013-03-05 23:28 ` Tejun Heo
2013-03-02 3:24 ` [PATCH 17/31] workqueue: implement attribute-based unbound worker_pool management Tejun Heo
2013-03-10 10:08 ` Lai Jiangshan
2013-03-10 12:58 ` Tejun Heo
2013-03-10 18:36 ` Tejun Heo
2013-03-12 18:21 ` [PATCH v2 " Tejun Heo
2013-03-02 3:24 ` [PATCH 18/31] workqueue: remove unbound_std_worker_pools[] and related helpers Tejun Heo
2013-03-02 3:24 ` [PATCH 19/31] workqueue: drop "std" from cpu_std_worker_pools and for_each_std_worker_pool() Tejun Heo
2013-03-02 3:24 ` [PATCH 20/31] workqueue: add pool ID to the names of unbound kworkers Tejun Heo
2013-03-02 3:24 ` [PATCH 21/31] workqueue: drop WQ_RESCUER and test workqueue->rescuer for NULL instead Tejun Heo
2013-03-02 3:24 ` [PATCH 22/31] workqueue: restructure __alloc_workqueue_key() Tejun Heo
2013-03-02 3:24 ` [PATCH 23/31] workqueue: implement get/put_pwq() Tejun Heo
2013-03-02 3:24 ` [PATCH 24/31] workqueue: prepare flush_workqueue() for dynamic creation and destrucion of unbound pool_workqueues Tejun Heo
2013-03-02 3:24 ` [PATCH 25/31] workqueue: perform non-reentrancy test when queueing to unbound workqueues too Tejun Heo
2013-03-02 3:24 ` [PATCH 26/31] workqueue: implement apply_workqueue_attrs() Tejun Heo
2013-03-02 3:24 ` [PATCH 27/31] workqueue: make it clear that WQ_DRAINING is an internal flag Tejun Heo
2013-03-02 3:24 ` [PATCH 28/31] workqueue: reject increasing max_active for ordered workqueues Tejun Heo
2013-03-04 18:30 ` [PATCH UPDATED 28/31] workqueue: reject adjusting max_active or applying attrs to " Tejun Heo
2013-03-02 3:24 ` [PATCH 29/31] cpumask: implement cpumask_parse() Tejun Heo
2013-03-02 3:24 ` [PATCH 30/31] driver/base: implement subsys_virtual_register() Tejun Heo
2013-03-02 18:17 ` Greg Kroah-Hartman
2013-03-02 20:26 ` Tejun Heo
2013-03-03 6:42 ` Kay Sievers
2013-03-05 20:43 ` Tejun Heo
2013-03-07 23:31 ` Greg Kroah-Hartman
2013-03-08 0:04 ` Kay Sievers
2013-03-10 11:57 ` Tejun Heo
2013-03-10 16:45 ` Greg Kroah-Hartman
2013-03-10 17:00 ` Kay Sievers
2013-03-10 17:24 ` Greg Kroah-Hartman
2013-03-10 17:50 ` Kay Sievers
2013-03-10 18:34 ` Tejun Heo
2013-03-12 18:40 ` Tejun Heo
2013-03-02 3:24 ` [PATCH 31/31] workqueue: implement sysfs interface for workqueues Tejun Heo
2013-03-04 18:30 ` [PATCH v2 " Tejun Heo
2013-03-05 20:41 ` [PATCHSET wq/for-3.10-tmp] workqueue: implement workqueue with custom worker attributes Tejun Heo
2013-03-10 10:34 ` Lai Jiangshan
2013-03-10 12:01 ` Tejun Heo
2013-03-11 15:24 ` Tejun Heo
2013-03-11 15:40 ` Lai Jiangshan
2013-03-11 15:42 ` Lai Jiangshan
2013-03-11 15:43 ` Tejun Heo
2013-03-12 18:10 ` Tejun Heo
2013-03-12 18:34 ` Tejun Heo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=513C5BD8.9050909@cn.fujitsu.com \
--to=laijs@cn.fujitsu.com \
--cc=axboe@kernel.dk \
--cc=jmoyer@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=tj@kernel.org \
--cc=zab@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox