linuxppc-dev.lists.ozlabs.org archive mirror
 help / color / mirror / Atom feed
From: Anton Vorontsov <anton.vorontsov@linaro.org>
To: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Mike Frysinger <vapier@gentoo.org>,
	user-mode-linux-devel@lists.sourceforge.net,
	linux-sh@vger.kernel.org, Richard Weinberger <richard@nod.at>,
	linuxppc-dev@lists.ozlabs.org, Oleg Nesterov <oleg@redhat.com>,
	linux-kernel@vger.kernel.org, linux-mm@kvack.org,
	Paul Mundt <lethal@linux-sh.org>,
	John Stultz <john.stultz@linaro.org>,
	KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>,
	Russell King <linux@arm.linux.org.uk>,
	Andrew Morton <akpm@linux-foundation.org>,
	uclinux-dist-devel@blackfin.uclinux.org,
	linux-arm-kernel@lists.infradead.org
Subject: [PATCH v2.1 01/10] cpu: Introduce clear_tasks_mm_cpumask() helper
Date: Sat, 24 Mar 2012 20:43:16 +0400	[thread overview]
Message-ID: <20120324164316.GB3640@lizard> (raw)
In-Reply-To: <1332593021.16159.27.camel@twins>

Many architctures clear tasks' mm_cpumask like this:

	read_lock(&tasklist_lock);
	for_each_process(p) {
		if (p->mm)
			cpumask_clear_cpu(cpu, mm_cpumask(p->mm));
	}
	read_unlock(&tasklist_lock);

The code above has several problems, such as:

1. Working with task->mm w/o getting mm or grabing the task lock is
   dangerous as ->mm might disappear (exit_mm() assigns NULL under
   task_lock(), so tasklist lock is not enough).

2. Checking for process->mm is not enough because process' main
   thread may exit or detach its mm via use_mm(), but other threads
   may still have a valid mm.

This patch implements a small helper function that does things
correctly, i.e.:

1. We take the task's lock while whe handle its mm (we can't use
   get_task_mm()/mmput() pair as mmput() might sleep);

2. To catch exited main thread case, we use find_lock_task_mm(),
   which walks up all threads and returns an appropriate task
   (with task lock held).

Also, Per Peter Zijlstra's idea, now we don't grab tasklist_lock in
the new helper, instead we take the rcu read lock. We can do this
because the function is called after the cpu is taken down and marked
offline, so no new tasks will get this cpu set in their mm mask.

Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
---

On Sat, Mar 24, 2012 at 01:43:41PM +0100, Peter Zijlstra wrote:
> On Sat, 2012-03-24 at 14:27 +0400, Anton Vorontsov wrote:
> > +       read_unlock(&tasklist_lock);
> > +} 
> 
> Why bother with the tasklist_lock at all anymore, afaict you could use
> rcu_read_lock() here. This all is called after the cpu is taken down and
> marked offline, so its not like new tasks will ever get this cpu set in
> their mm mask.

I guess you're right. Well, this would make the code a bit fragile,
but a comment might help.

How about this version?

 include/linux/cpu.h |    1 +
 kernel/cpu.c        |   26 ++++++++++++++++++++++++++
 2 files changed, 27 insertions(+)

diff --git a/include/linux/cpu.h b/include/linux/cpu.h
index 1f65875..941e865 100644
--- a/include/linux/cpu.h
+++ b/include/linux/cpu.h
@@ -171,6 +171,7 @@ extern void put_online_cpus(void);
 #define hotcpu_notifier(fn, pri)	cpu_notifier(fn, pri)
 #define register_hotcpu_notifier(nb)	register_cpu_notifier(nb)
 #define unregister_hotcpu_notifier(nb)	unregister_cpu_notifier(nb)
+void clear_tasks_mm_cpumask(int cpu);
 int cpu_down(unsigned int cpu);
 
 #ifdef CONFIG_ARCH_CPU_PROBE_RELEASE
diff --git a/kernel/cpu.c b/kernel/cpu.c
index 2060c6e..ecdf499 100644
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -10,6 +10,8 @@
 #include <linux/sched.h>
 #include <linux/unistd.h>
 #include <linux/cpu.h>
+#include <linux/oom.h>
+#include <linux/rcupdate.h>
 #include <linux/export.h>
 #include <linux/kthread.h>
 #include <linux/stop_machine.h>
@@ -171,6 +173,30 @@ void __ref unregister_cpu_notifier(struct notifier_block *nb)
 }
 EXPORT_SYMBOL(unregister_cpu_notifier);
 
+void clear_tasks_mm_cpumask(int cpu)
+{
+	struct task_struct *p;
+
+	/*
+	 * This function is called after the cpu is taken down and marked
+	 * offline, so its not like new tasks will ever get this cpu set in
+	 * their mm mask. -- Peter Zijlstra
+	 * Thus, we may use rcu_read_lock() here, instead of grabbing
+	 * full-fledged tasklist_lock.
+	 */
+	rcu_read_lock();
+	for_each_process(p) {
+		struct task_struct *t;
+
+		t = find_lock_task_mm(p);
+		if (!t)
+			continue;
+		cpumask_clear_cpu(cpu, mm_cpumask(t->mm));
+		task_unlock(t);
+	}
+	rcu_read_unlock();
+}
+
 static inline void check_for_tasks(int cpu)
 {
 	struct task_struct *p;
-- 
1.7.9.2

  reply	other threads:[~2012-03-24 16:44 UTC|newest]

Thread overview: 27+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-03-24 10:26 [PATCH v2 0/10] Fixes for common mistakes w/ for_each_process and task->mm Anton Vorontsov
2012-03-24 10:27 ` [PATCH 01/10] cpu: Introduce clear_tasks_mm_cpumask() helper Anton Vorontsov
2012-03-24 12:43   ` Peter Zijlstra
2012-03-24 16:43     ` Anton Vorontsov [this message]
2012-03-25 17:42       ` [PATCH v2.1 " Oleg Nesterov
2012-03-26  7:59         ` Peter Zijlstra
2012-03-26 17:04           ` Oleg Nesterov
2012-03-26 17:23             ` Peter Zijlstra
2012-03-28  0:01         ` Benjamin Herrenschmidt
2012-03-24 10:28 ` [PATCH 02/10] arm: Use clear_tasks_mm_cpumask() Anton Vorontsov
2012-03-24 10:28 ` [PATCH 03/10] powerpc: " Anton Vorontsov
2012-03-24 10:28 ` [PATCH 04/10] sh: " Anton Vorontsov
2012-03-24 10:29 ` [PATCH 05/10] blackfin: A couple of task->mm handling fixes Anton Vorontsov
2012-03-24 10:30 ` [PATCH 06/10] blackfin: Fix possible deadlock in decode_address() Anton Vorontsov
2012-03-24 10:30 ` [PATCH 07/10] um: Should hold tasklist_lock while traversing processes Anton Vorontsov
2012-03-24 11:12   ` Richard Weinberger
2012-03-24 12:48   ` Peter Zijlstra
2012-03-24 16:43     ` Anton Vorontsov
2012-03-24 10:30 ` [PATCH 08/10] um: Fix possible race on task->mm Anton Vorontsov
2012-03-24 11:12   ` Richard Weinberger
2012-03-24 10:31 ` [PATCH 09/10] um: Properly check all process' threads for a live mm Anton Vorontsov
2012-03-24 11:12   ` Richard Weinberger
2012-03-24 10:31 ` [PATCH 10/10] oom: Make find_lock_task_mm() sparse-aware Anton Vorontsov
2012-03-24 12:52   ` Peter Zijlstra
2012-03-24 16:21     ` Anton Vorontsov
2012-03-24 16:43       ` Peter Zijlstra
2012-03-28  7:20     ` David Rientjes

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=20120324164316.GB3640@lizard \
    --to=anton.vorontsov@linaro.org \
    --cc=a.p.zijlstra@chello.nl \
    --cc=akpm@linux-foundation.org \
    --cc=john.stultz@linaro.org \
    --cc=kosaki.motohiro@jp.fujitsu.com \
    --cc=lethal@linux-sh.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-sh@vger.kernel.org \
    --cc=linux@arm.linux.org.uk \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=oleg@redhat.com \
    --cc=richard@nod.at \
    --cc=uclinux-dist-devel@blackfin.uclinux.org \
    --cc=user-mode-linux-devel@lists.sourceforge.net \
    --cc=vapier@gentoo.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;
as well as URLs for NNTP newsgroup(s).