LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Srivatsa Vaddagiri <vatsa@in.ibm.com>
To: Paul Jackson <pj@sgi.com>
Cc: menage@google.com, akpm@linux-foundation.org, dev@sw.ru,
	xemul@sw.ru, serue@us.ibm.com, ebiederm@xmission.com,
	haveblue@us.ibm.com, svaidy@linux.vnet.ibm.com,
	balbir@in.ibm.com, ckrm-tech@lists.sourceforge.net,
	linux-kernel@vger.kernel.org, rohitseth@google.com,
	mbligh@google.com, containers@lists.osdl.org, devel@openvz.org
Subject: Re: [PATCH 0/9] Containers (V9): Generic Process Containers
Date: Mon, 30 Apr 2007 22:42:25 +0530	[thread overview]
Message-ID: <20070430171225.GH24350@in.ibm.com> (raw)
In-Reply-To: <20070429023721.53f249b9.pj@sgi.com>

On Sun, Apr 29, 2007 at 02:37:21AM -0700, Paul Jackson wrote:
> It builds and boots and mounts the cpuset file system ok.
> But trying to write the 'mems' file hangs the system hard.

Basically we are attempting a read_lock(&tasklist_lock) in
container_task_count() after taking write_lock_irq(&tasklist_lock) in
update_nodemask()!

This patch seems to fix the prb for me:


Fix write_lock() followed by read_lock() bug by introducing a 2nd
argument to be passed into container_task_count. Other choice is to
introduce a lock and unlocked versions of container_task_count() ..

Signed-off-by : Srivatsa Vaddagiri <vatsa@in.ibm.com>


---


diff -puN include/linux/container.h~lock_fix include/linux/container.h
--- linux-2.6.21-rc7/include/linux/container.h~lock_fix	2007-04-30 21:56:10.000000000 +0530
+++ linux-2.6.21-rc7-vatsa/include/linux/container.h	2007-04-30 21:56:32.000000000 +0530
@@ -164,7 +164,7 @@ int container_is_removed(const struct co
 
 int container_path(const struct container *cont, char *buf, int buflen);
 
-int container_task_count(const struct container *cont);
+int container_task_count(const struct container *cont, int take_lock);
 
 /* Return true if the container is a descendant of the current container */
 int container_is_descendant(const struct container *cont);
diff -puN kernel/container.c~lock_fix kernel/container.c
--- linux-2.6.21-rc7/kernel/container.c~lock_fix	2007-04-30 21:56:10.000000000 +0530
+++ linux-2.6.21-rc7-vatsa/kernel/container.c	2007-04-30 22:06:45.000000000 +0530
@@ -1193,21 +1193,25 @@ int container_add_files(struct container
 /* Count the number of tasks in a container. Could be made more
  * time-efficient but less space-efficient with more linked lists
  * running through each container and the css_group structures
- * that referenced it. */
+ * that referenced it. 'take_lock' indicates whether caller has locked
+ * tasklist or not.
+ */
 
-int container_task_count(const struct container *cont) {
+int container_task_count(const struct container *cont, int take_lock) {
 	int count = 0;
 	struct task_struct *g, *p;
 	struct container_subsys_state *css;
 	int subsys_id;
 	get_first_subsys(cont, &css, &subsys_id);
 
-	read_lock(&tasklist_lock);
+	if (take_lock)
+		read_lock(&tasklist_lock);
 	do_each_thread(g, p) {
 		if (task_subsys_state(p, subsys_id) == css)
 			count ++;
 	} while_each_thread(g, p);
-	read_unlock(&tasklist_lock);
+	if (take_lock)
+		read_unlock(&tasklist_lock);
 	return count;
 }
 
@@ -1311,7 +1315,7 @@ static int container_tasks_open(struct i
 	 * caller from the case that the additional container users didn't
 	 * show up until sometime later on.
 	 */
-	npids = container_task_count(cont);
+	npids = container_task_count(cont, 1);
 	pidarray = kmalloc(npids * sizeof(pid_t), GFP_KERNEL);
 	if (!pidarray)
 		goto err1;
diff -puN kernel/cpuset.c~lock_fix kernel/cpuset.c
--- linux-2.6.21-rc7/kernel/cpuset.c~lock_fix	2007-04-30 21:56:10.000000000 +0530
+++ linux-2.6.21-rc7-vatsa/kernel/cpuset.c	2007-04-30 21:59:25.000000000 +0530
@@ -631,13 +631,13 @@ static int update_nodemask(struct cpuset
 	 * enough mmarray[] w/o using GFP_ATOMIC.
 	 */
 	while (1) {
-		ntasks = container_task_count(cs->css.container);  /* guess */
+		ntasks = container_task_count(cs->css.container, 1); /* guess */
 		ntasks += fudge;
 		mmarray = kmalloc(ntasks * sizeof(*mmarray), GFP_KERNEL);
 		if (!mmarray)
 			goto done;
 		write_lock_irq(&tasklist_lock);		/* block fork */
-		if (container_task_count(cs->css.container) <= ntasks)
+		if (container_task_count(cs->css.container, 0) <= ntasks)
 			break;				/* got enough */
 		write_unlock_irq(&tasklist_lock);	/* try again */
 		kfree(mmarray);
diff -puN kernel/container_debug.c~lock_fix kernel/container_debug.c
--- linux-2.6.21-rc7/kernel/container_debug.c~lock_fix	2007-04-30 21:58:54.000000000 +0530
+++ linux-2.6.21-rc7-vatsa/kernel/container_debug.c	2007-04-30 21:59:04.000000000 +0530
@@ -34,7 +34,7 @@ static u64 taskcount_read(struct contain
 {
 	u64 count;
 	container_lock();
-	count = container_task_count(cont);
+	count = container_task_count(cont, 1);
 	container_unlock();
 	return count;
 }
_


-- 
Regards,
vatsa

  reply	other threads:[~2007-04-30 17:04 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-04-27 10:46 menage
2007-04-27 10:46 ` [PATCH 1/9] Containers (V9): Basic container framework menage
2007-04-29  3:12   ` [ckrm-tech] " Paul Jackson
2007-05-01 17:40   ` Balbir Singh
2007-05-01 17:46     ` Paul Menage
2007-05-01 18:40     ` [ckrm-tech] " Paul Jackson
2007-05-02  3:44       ` Balbir Singh
2007-05-02  6:12         ` Paul Jackson
2007-05-10  4:09           ` Balbir Singh
2007-05-10  4:47             ` Paul Jackson
2007-05-10  4:49               ` Balbir Singh
2007-04-27 10:46 ` [PATCH 2/9] Containers (V9): Example CPU accounting subsystem menage
2007-05-01 17:52   ` Balbir Singh
2007-04-27 10:46 ` [PATCH 3/9] Containers (V9): Add tasks file interface menage
2007-05-01 18:12   ` Balbir Singh
2007-05-01 20:37     ` [ckrm-tech] " Paul Menage
2007-05-02  3:25       ` Srivatsa Vaddagiri
2007-05-02  3:25         ` Paul Menage
2007-05-02  3:46           ` Srivatsa Vaddagiri
2007-05-08 14:51           ` Balbir Singh
2007-05-10 21:21             ` Paul Menage
2007-05-11  2:31               ` Balbir Singh
2007-05-02  3:58       ` Balbir Singh
2007-04-27 10:46 ` [PATCH 4/9] Containers (V9): Add fork/exit hooks menage
2007-04-27 10:46 ` [PATCH 5/9] Containers (V9): Add container_clone() interface menage
2007-04-27 10:46 ` [PATCH 6/9] Containers (V9): Add procfs interface menage
2007-04-27 10:46 ` [PATCH 7/9] Containers (V9): Make cpusets a client of containers menage
2007-04-27 10:46 ` [PATCH 8/9] Containers (V9): Share css_group arrays between tasks with same container memberships menage
2007-04-27 10:46 ` [PATCH 9/9] Containers (V9): Simple debug info subsystem menage
2007-04-29  1:47 ` [PATCH 0/9] Containers (V9): Generic Process Containers Paul Jackson
2007-04-29  9:37 ` Paul Jackson
2007-04-30 17:12   ` Srivatsa Vaddagiri [this message]
2007-04-30 17:09     ` Paul Menage
2007-04-30 18:06       ` Srivatsa Vaddagiri
2007-04-30 17:23     ` Christoph Hellwig
2007-04-30 18:16       ` [ckrm-tech] " Srivatsa Vaddagiri

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=20070430171225.GH24350@in.ibm.com \
    --to=vatsa@in.ibm.com \
    --cc=akpm@linux-foundation.org \
    --cc=balbir@in.ibm.com \
    --cc=ckrm-tech@lists.sourceforge.net \
    --cc=containers@lists.osdl.org \
    --cc=dev@sw.ru \
    --cc=devel@openvz.org \
    --cc=ebiederm@xmission.com \
    --cc=haveblue@us.ibm.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mbligh@google.com \
    --cc=menage@google.com \
    --cc=pj@sgi.com \
    --cc=rohitseth@google.com \
    --cc=serue@us.ibm.com \
    --cc=svaidy@linux.vnet.ibm.com \
    --cc=xemul@sw.ru \
    --subject='Re: [PATCH 0/9] Containers (V9): Generic Process Containers' \
    /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

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).