From: Tejun Heo <tj@kernel.org>
To: Lai Jiangshan <eag0628@gmail.com>
Cc: Lai Jiangshan <laijs@cn.fujitsu.com>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 15/21] workqueue: remove worker_maybe_bind_and_lock()
Date: Thu, 21 Mar 2013 10:41:33 -0700 [thread overview]
Message-ID: <20130321174133.GA20500@htj.dyndns.org> (raw)
In-Reply-To: <CACvQF512Ufz-Y2HjLKOGNW8A+9epcv6yvB8szyR4S9d4Uy41Sw@mail.gmail.com>
Hello, Lai.
On Thu, Mar 21, 2013 at 07:03:30PM +0800, Lai Jiangshan wrote:
> On Thu, Mar 21, 2013 at 2:10 AM, Tejun Heo <tj@kernel.org> wrote:
> > On Wed, Mar 20, 2013 at 03:28:15AM +0800, Lai Jiangshan wrote:
> >> static struct worker *alloc_worker(void)
> >> {
> >> struct worker *worker;
> >> @@ -2326,7 +2262,8 @@ repeat:
> >> spin_unlock_irq(&wq_mayday_lock);
> >>
> >> /* migrate to the target cpu if possible */
> >> - worker_maybe_bind_and_lock(pool);
> >> + set_cpus_allowed_ptr(current, pool->attrs->cpumask);
> >> + spin_lock_irq(&pool->lock);
> >> rescuer->pool = pool;
> >
> > You can't do this. The CPU might go up between set_cpus_allowed_ptr()
> > and spin_lock_irq() and the rescuer can end up executing a work item
> > which was issued while the target CPU is up on the wrong CPU.
>
>
> Why it is wrong?
> It can be allowed if the wq has rescuer. (wq of work_on_cpu() don't has rescuer,
> it does not hurt to work_on_cpu() of something else.
>
> Actually it was allowed by recent patches.
> If a cpu is up while a rescuer do process_scheduled_workers(),
> all the later work item will be running the wrong CPU while
> the target CPU is up.(due to for_cpu_pool_workers() don't include
> active rescuer).
So,
* If the user wraps queueing and flushing inside get_online_cpus(), it
better gets executed and finishes on the target CPU.
* Similarly, it must guaranteed that any work items queued and flushed
by CPU_ONLINE or CPU_DOWN_PREP notifiers should start and finish
execution on the target CPU.
Your patch breaks the above.
> I don't want to go back to make cpuhotplug nor rescuer_theread become
> complicated.
> so I prefer to allow work item can run on wrong cpu if the wq has rescuer.
> All just needs a comments.
It is explained in the comment.
> I guess you will agaist me...... let me rewrite it .....
It's not because I'm "against" you. It's because it's buggy and we
wouldn't have any affinity guarantee with the proposed changes. If
you can maintain the guarantees and remove
worker_maybe_bind_and_lock(), please be my guest.
Thanks.
--
tejun
next prev parent reply other threads:[~2013-03-21 17:41 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-03-19 19:28 [PATCH 00/21] workqueue: cleanups and better locking for recent changes Lai Jiangshan
2013-03-19 19:28 ` [PATCH 01/21] workqueue: add missing POOL_FREEZING Lai Jiangshan
2013-03-20 17:18 ` Tejun Heo
2013-03-20 17:24 ` Tejun Heo
2013-03-19 19:28 ` [PATCH 02/21] workqueue: don't free pool->worker_idr by RCU Lai Jiangshan
2013-03-20 17:29 ` Tejun Heo
2013-03-19 19:28 ` [PATCH 03/21] workqueue: simplify current_is_workqueue_rescuer() Lai Jiangshan
2013-03-20 17:33 ` Tejun Heo
2013-03-19 19:28 ` [PATCH 04/21] workqueue: swap the two branches in pwq_adjust_max_active() to get better readability Lai Jiangshan
2013-03-20 17:39 ` Tejun Heo
2013-03-19 19:28 ` [PATCH 05/21] workqueue: kick workers in pwq_adjust_max_active() Lai Jiangshan
2013-03-20 17:56 ` [PATCH] workqueue: kick a worker " Tejun Heo
2013-03-19 19:28 ` [PATCH 06/21] workqueue: separate out pools locking into pools_mutex Lai Jiangshan
2013-03-20 17:58 ` Tejun Heo
2013-03-19 19:28 ` [PATCH 07/21] workqueue: rename wq_mutex to wqs_mutex Lai Jiangshan
2013-03-19 19:28 ` [PATCH 08/21] workqueue: rename wq->flush_mutex to wq->mutex Lai Jiangshan
2013-03-19 19:28 ` [PATCH 09/21] workqueue: use wq->mutex to protects ->nr_drainers and __WQ_DRAINING Lai Jiangshan
2013-03-19 19:28 ` [PATCH 10/21] workqueue: use rcu_read_lock_sched() instead for accessing pwq in RCU Lai Jiangshan
2013-03-20 18:01 ` Tejun Heo
2013-03-19 19:28 ` [PATCH 11/21] workqueue: also allowed wq->mutex protect for_each_pwq() Lai Jiangshan
2013-03-19 19:28 ` [PATCH 12/21] workqueue: use wq->mutex to protect saved_max_active Lai Jiangshan
2013-03-19 19:28 ` [PATCH 13/21] workqueue: remove unused pwq_lock Lai Jiangshan
2013-03-19 19:28 ` [PATCH 14/21] workqueue: add wq->freezing and remove POOL_FREEZING Lai Jiangshan
2013-03-19 19:28 ` [PATCH 15/21] workqueue: remove worker_maybe_bind_and_lock() Lai Jiangshan
2013-03-20 18:10 ` Tejun Heo
2013-03-21 11:03 ` Lai Jiangshan
2013-03-21 17:41 ` Tejun Heo [this message]
2013-03-19 19:28 ` [PATCH 16/21] workqueue: rename rebind_workers() to associate_cpu_pool() Lai Jiangshan
2013-03-19 19:28 ` [PATCH 17/21] workqueue: simplify workqueue_cpu_up_callback(CPU_ONLINE) Lai Jiangshan
2013-03-20 18:16 ` Tejun Heo
2013-03-19 19:28 ` [PATCH 18/21] workqueue: read POOL_DISASSOCIATED bit under pool->lock Lai Jiangshan
2013-03-20 18:19 ` Tejun Heo
2013-03-19 19:28 ` [PATCH 19/21] workqueue: remove @p_last_pwq from init_and_link_pwq() Lai Jiangshan
2013-03-19 19:28 ` [PATCH 20/21] workqueue: modify wq->freezing only when freezable Lai Jiangshan
2013-03-19 19:28 ` [PATCH 21/21] workqueue: avoid false negative in assert_manager_or_pool_lock() Lai Jiangshan
2013-03-20 18:22 ` Tejun Heo
2013-03-20 16:38 ` [PATCH 00/21] workqueue: cleanups and better locking for recent changes Lai Jiangshan
2013-03-20 16:40 ` Tejun Heo
2013-03-20 18:30 ` 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=20130321174133.GA20500@htj.dyndns.org \
--to=tj@kernel.org \
--cc=eag0628@gmail.com \
--cc=laijs@cn.fujitsu.com \
--cc=linux-kernel@vger.kernel.org \
/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
all inboxes | Powered by JetHome®