All of lore.kernel.org
 help / color / mirror / Atom feed
* + cpuset-check-if-the-cpus-allowed-is-a-subset-of-the-cpusets-cpus-for-pf_thread_bound-threads.patch added to -mm tree
@ 2009-03-12 22:51 akpm
       [not found] ` <alpine.DEB.2.00.0903121619540.19505@chino.kir.corp.google.com>
  0 siblings, 1 reply; 3+ messages in thread
From: akpm @ 2009-03-12 22:51 UTC (permalink / raw)
  To: mm-commits; +Cc: dhaval, balbir, menage, rientjes, vatsa


The patch titled
     cpuset: check if the cpus allowed is a subset of the cpuset's cpus for PF_THREAD_BOUND threads
has been added to the -mm tree.  Its filename is
     cpuset-check-if-the-cpus-allowed-is-a-subset-of-the-cpusets-cpus-for-pf_thread_bound-threads.patch

Before you just go and hit "reply", please:
   a) Consider who else should be cc'ed
   b) Prefer to cc a suitable mailing list as well
   c) Ideally: find the original patch on the mailing list and do a
      reply-to-all to that, adding suitable additional cc's

*** Remember to use Documentation/SubmitChecklist when testing your code ***

See http://userweb.kernel.org/~akpm/stuff/added-to-mm.txt to find
out what to do about this

The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/

------------------------------------------------------
Subject: cpuset: check if the cpus allowed is a subset of the cpuset's cpus for PF_THREAD_BOUND threads
From: Dhaval Giani <dhaval@linux.vnet.ibm.com>

When a kernel thread which has been bound to a CPU is moved to a cpuset
which is a superset of the CPU it is bound to, the movement fails.

[root@llm11 cgroups]# mkdir a
[root@llm11 cgroups]# echo 3 > a/cpuset.cpus
[root@llm11 cgroups]# echo 0 > a/cpuset.mems
[root@llm11 cgroups]# echo 12 > a/tasks
[root@llm11 cgroups]# echo 12 > tasks
-bash: echo: write error: Invalid argument
[root@llm11 cgroups]# cat cpuset.cpus
0-7
[root@llm11 cgroups]#

(pid 12 is the migration thread bound to CPU 3)

Change the check to see if the cpus allowed is a subset of the cpus
allowed for that cpuset.

Signed-off-by: Dhaval Giani <dhaval@linux.vnet.ibm.com>
Srivatsa Vaddagiri <vatsa@linux.vnet.ibm.com>
Cc: Balbir Singh <balbir@in.ibm.com>
Cc: Paul Menage <menage@google.com>
Cc: David Rientjes <rientjes@google.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 kernel/cpuset.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff -puN kernel/cpuset.c~cpuset-check-if-the-cpus-allowed-is-a-subset-of-the-cpusets-cpus-for-pf_thread_bound-threads kernel/cpuset.c
--- a/kernel/cpuset.c~cpuset-check-if-the-cpus-allowed-is-a-subset-of-the-cpusets-cpus-for-pf_thread_bound-threads
+++ a/kernel/cpuset.c
@@ -1349,7 +1349,7 @@ static int cpuset_can_attach(struct cgro
 
 	if (tsk->flags & PF_THREAD_BOUND) {
 		mutex_lock(&callback_mutex);
-		if (!cpumask_equal(&tsk->cpus_allowed, cs->cpus_allowed))
+		if (!cpumask_subset(&tsk->cpus_allowed, cs->cpus_allowed))
 			ret = -EINVAL;
 		mutex_unlock(&callback_mutex);
 	}
_

Patches currently in -mm which might be from dhaval@linux.vnet.ibm.com are

origin.patch
cgroup-css-id-support-remove-rcu_read_lock-from-css_get_next.patch
cpuset-check-if-the-cpus-allowed-is-a-subset-of-the-cpusets-cpus-for-pf_thread_bound-threads.patch


^ permalink raw reply	[flat|nested] 3+ messages in thread

* [PATCH] cpuset: fix cpuset vs PF_THREAD_BOUND weirdness
       [not found]         ` <alpine.DEB.2.00.0903140233510.457@chino.kir.corp.google.com>
@ 2009-03-14 11:17           ` Peter Zijlstra
  2009-03-16 22:11             ` David Rientjes
  0 siblings, 1 reply; 3+ messages in thread
From: Peter Zijlstra @ 2009-03-14 11:17 UTC (permalink / raw)
  To: David Rientjes
  Cc: Dhaval Giani, Andrew Morton, balbir, menage, vatsa,
	Bharata B Rao, lkml, lkml

On Sat, 2009-03-14 at 02:53 -0700, David Rientjes wrote:
> On Sat, 14 Mar 2009, Dhaval Giani wrote:
> 
> > No. I do not agree. PF_THREAD_BOUND is a special case and should be
> > treated as such.
> 
> Userspace has no knowledge of PF_THREAD_BOUND tasks.

FWIW its exposed in /proc/$pid/stat and I know of user-space actually
using it.

> > There already exists an inconsistency. An example on a
> > recent-ish tip kernel.
> > 
> > [root@llm11 cpuset]# mkdir a
> > [root@llm11 cpuset]# echo 3 > a/cpuset.cpus
> > [root@llm11 cpuset]# echo 0 > a/cpuset.mems
> > [root@llm11 cpuset]# echo 12 > a/tasks
> > [root@llm11 cpuset]# cd a/
> > [root@llm11 a]# echo 2 > cpuset.cpus
> > [root@llm11 a]# cat cpuset.cpus
> > 2
> > [root@llm11 a]# taskset -pc 12
> > pid 12's current affinity list: 3
> > [root@llm11 a]#
> > 
> > As per your explanation, it is reasonable to expect that the cpu
> > affinity of pid 12 has now been set to CPU 2 but that is not the
> case.
> > 
> 
> Wrong, I said the consistency is that if a task is successfully attached 
> to a cpuset, then its set of allowed cpus has been altered to conform to 
> the cpuset's settings when it is attached.  Otherwise, it fails.
> 
> In your example, it would now be impossible to attach pid 12 to cpuset `a' 
> if it were not already a member.  The consistency exists at the time a 
> task is _attached_ to a cpuset, not because of its membership.  I think 
> this is where you're misunderstanding the long-standing behavior of 
> cpusets.

David, I'm not sure what you're arguing.

Letting a kernel thread in a subset, but then not letting it back out
again seems really weird to me, esp. since PF_THREAD_BOUND is a fairly
recent thing.

I do feel we need to address this issue because its terribly
counter-intuitive.

Furthermore, I do think Dhaval's patch is on the right path by changing
can_attach to check for a subset and attach to ignore the error on
PF_THREAD_BOUND.

However, I don't think its quite enough, we should furthermore fail
update_cpumask(), when it contains such a PF_THREAD_BOUND task and we're
removing the cpu its bound to from the mask, with -EBUSY.

So let me propose the following patch:

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
---
Index: linux-2.6/kernel/cpuset.c
===================================================================
--- linux-2.6.orig/kernel/cpuset.c
+++ linux-2.6/kernel/cpuset.c
@@ -884,10 +884,11 @@ static int cpuset_test_cpumask(struct ta
  * We don't need to re-check for the cgroup/cpuset membership, since we're
  * holding cgroup_lock() at this point.
  */
-static void cpuset_change_cpumask(struct task_struct *tsk,
-				  struct cgroup_scanner *scan)
+static int cpuset_change_cpumask(struct task_struct *tsk,
+				 struct cgroup_scanner *scan)
 {
 	set_cpus_allowed_ptr(tsk, ((cgroup_cs(scan->cg))->cpus_allowed));
+	return 0;
 }
 
 /**
@@ -911,9 +912,39 @@ static void update_tasks_cpumask(struct 
 	scan.test_task = cpuset_test_cpumask;
 	scan.process_task = cpuset_change_cpumask;
 	scan.heap = heap;
+
 	cgroup_scan_tasks(&scan);
 }
 
+static int cpuset_test_thread(struct task_struct *tsk,
+			      struct cgroup_scanner *scan)
+{
+	return tsk->flags & PF_THREAD_BOUND;
+}
+
+static int cpuset_validate_thread(struct task_struct *tsk,
+				  struct cgroup_scanner *scan)
+{
+	struct cpumask *new_mask = scan->data;
+
+	if (!cpumask_subset(&tsk->cpus_allowed, new_mask))
+		return -EBUSY;
+
+	return 0;
+}
+
+static int validate_cpumask(struct cpuset *cs, struct cpumask *new_mask)
+{
+	struct cgroup_scanner scan;
+
+	scan.cg = cs->css.cgroup;
+	scan.test_task = cpuset_test_thread;
+	scan.process_task = cpuset_validate_thread;
+	scan.data = new_mask;
+
+	return cgroup_scan_tasks(&scan);
+}
+
 /**
  * update_cpumask - update the cpus_allowed mask of a cpuset and all tasks in it
  * @cs: the cpuset to consider
@@ -954,6 +985,10 @@ static int update_cpumask(struct cpuset 
 	if (cpumask_equal(cs->cpus_allowed, trialcs->cpus_allowed))
 		return 0;
 
+	retval = validate_cpumask(cs, trailcs->cpus_allowed);
+	if (retval < 0)
+		return retval;
+
 	retval = heap_init(&heap, PAGE_SIZE, GFP_KERNEL, NULL);
 	if (retval)
 		return retval;
@@ -1362,7 +1397,7 @@ static int cpuset_can_attach(struct cgro
 
 	if (tsk->flags & PF_THREAD_BOUND) {
 		mutex_lock(&callback_mutex);
-		if (!cpumask_equal(&tsk->cpus_allowed, cs->cpus_allowed))
+		if (!cpumask_subset(&tsk->cpus_allowed, cs->cpus_allowed))
 			ret = -EINVAL;
 		mutex_unlock(&callback_mutex);
 	}
@@ -1388,7 +1423,12 @@ static void cpuset_attach(struct cgroup_
 		mutex_unlock(&callback_mutex);
 	}
 	err = set_cpus_allowed_ptr(tsk, cpus_attach);
-	if (err)
+	/*
+	 * In cpuset_can_attach we confirmed that a PF_THREAD_BOUND's CPUs
+	 * are a subset of the cpuset's. In this case set_cpus_allowed_ptr
+	 * will fail. We are fine with it and ignore that failure.
+	 */
+	if (err & !(tsk->flags & PF_THREAD_BOUND))
 		return;
 
 	from = oldcs->mems_allowed;
Index: linux-2.6/include/linux/cgroup.h
===================================================================
--- linux-2.6.orig/include/linux/cgroup.h
+++ linux-2.6/include/linux/cgroup.h
@@ -316,9 +316,10 @@ struct cftype {
 struct cgroup_scanner {
 	struct cgroup *cg;
 	int (*test_task)(struct task_struct *p, struct cgroup_scanner *scan);
-	void (*process_task)(struct task_struct *p,
+	int (*process_task)(struct task_struct *p,
 			struct cgroup_scanner *scan);
 	struct ptr_heap *heap;
+	struct void *data;
 };
 
 /* Add a new file to the given cgroup directory. Should only be
Index: linux-2.6/kernel/cgroup.c
===================================================================
--- linux-2.6.orig/kernel/cgroup.c
+++ linux-2.6/kernel/cgroup.c
@@ -1980,6 +1980,7 @@ int cgroup_scan_tasks(struct cgroup_scan
 	}
 	cgroup_iter_end(scan->cg, &it);
 
+	retval = 0;
 	if (heap->size) {
 		for (i = 0; i < heap->size; i++) {
 			struct task_struct *q = heap->ptrs[i];
@@ -1988,8 +1989,10 @@ int cgroup_scan_tasks(struct cgroup_scan
 				latest_task = q;
 			}
 			/* Process the task per the caller's callback */
-			scan->process_task(q, scan);
+			retval = scan->process_task(q, scan);
 			put_task_struct(q);
+			if (retval < 0)
+				break;
 		}
 		/*
 		 * If we had to process any tasks at all, scan again
@@ -2002,7 +2005,7 @@ int cgroup_scan_tasks(struct cgroup_scan
 	}
 	if (heap == &tmp_heap)
 		heap_free(&tmp_heap);
-	return 0;
+	return retval;
 }
 
 /*



^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] cpuset: fix cpuset vs PF_THREAD_BOUND weirdness
  2009-03-14 11:17           ` [PATCH] cpuset: fix cpuset vs PF_THREAD_BOUND weirdness Peter Zijlstra
@ 2009-03-16 22:11             ` David Rientjes
  0 siblings, 0 replies; 3+ messages in thread
From: David Rientjes @ 2009-03-16 22:11 UTC (permalink / raw)
  To: Peter Zijlstra
  Cc: Dhaval Giani, Andrew Morton, balbir, menage, vatsa,
	Bharata B Rao, lkml, lkml

On Sat, 14 Mar 2009, Peter Zijlstra wrote:

> David, I'm not sure what you're arguing.
> 
> Letting a kernel thread in a subset, but then not letting it back out
> again seems really weird to me, esp. since PF_THREAD_BOUND is a fairly
> recent thing.
> 

The addition of PF_THREAD_BOUND impacted cpusets because, for the first 
time, it was possible that setting the new set of allowed cpus wouldn't 
succeed (cpusets already deals with the hotplug exception).

That introduced a new requirement: that set_cpus_allowed_ptr() succeeds 
for the attachment of a task to the cpuset changes.  The cpu affinity of 
a task is changed to match that of the cpuset anytime it is moved and then 
imposed a superset for subsequent sched_setaffinity() calls from 
userspace.  Your change would do the latter but neglect the former if it 
doesn't succeed; I'm not immediately opposed to that, but I wasn't 
satisfied with the additional inconsistency that the earlier patch added 
(i.e. changing a cpuset's cpumask that now is disjoint from threads with a 
bound cpu).  So to be clear, I'm not disagreeing with resolving a 
perceived inconsistency with cpuset movement, but I am disagreeing with an 
incomplete solution.

Your solution appears to be more complete, although I have not tested it.  
So thanks for looking at this particular issue.

Let me propose my own patch, however, that would be less intrusive and, if 
anything, more logical.  It would also not require a change to the cgroup 
interface for attachment callbacks.

This patch simply denies PF_THREAD_BOUND tasks from ever moving out of the 
root cpuset.  These kthreads do not require mempolicies that disallow 
access to certain nodes and their cpumask can never be changed; thus, 
cpusets isn't applicable to these threads.  They will forever reside in 
the root cpuset so the interface with userspace remains consistent (i.e. 
`cat tasks' for the root cpuset will still work).

Unless a use-case can be shown for a single PF_THREAD_BOUND task where 
manipulating its cpuset attachment or mempolicy matters, I think this is 
the most appropriate solution.

Signed-off-by: David Rientjes <rientjes@google.com>
---
diff --git a/kernel/cpuset.c b/kernel/cpuset.c
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -1360,12 +1360,16 @@ static int cpuset_can_attach(struct cgroup_subsys *ss,
 	if (cpumask_empty(cs->cpus_allowed) || nodes_empty(cs->mems_allowed))
 		return -ENOSPC;
 
-	if (tsk->flags & PF_THREAD_BOUND) {
-		mutex_lock(&callback_mutex);
-		if (!cpumask_equal(&tsk->cpus_allowed, cs->cpus_allowed))
-			ret = -EINVAL;
-		mutex_unlock(&callback_mutex);
-	}
+	/*
+	 * Kthreads bound to specific cpus cannot be moved to a new cpuset; we
+	 * cannot change their cpu affinity and isolating such threads by their
+	 * set of allowed nodes is unnecessary.  Thus, cpusets are not
+	 * applicable for such threads.  This prevents checking for success of
+	 * set_cpus_allowed_ptr() on all attached tasks before cpus_allowed may
+	 * be changed.
+	 */
+	if (tsk->flags & PF_THREAD_BOUND)
+		return -EINVAL;
 
 	return ret < 0 ? ret : security_task_setscheduler(tsk, 0, NULL);
 }

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2009-03-16 22:13 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-03-12 22:51 + cpuset-check-if-the-cpus-allowed-is-a-subset-of-the-cpusets-cpus-for-pf_thread_bound-threads.patch added to -mm tree akpm
     [not found] ` <alpine.DEB.2.00.0903121619540.19505@chino.kir.corp.google.com>
     [not found]   ` <20090313134635.GE17927@linux.vnet.ibm.com>
     [not found]     ` <alpine.DEB.2.00.0903131143040.17621@chino.kir.corp.google.com>
     [not found]       ` <20090314092345.GA3339@linux.vnet.ibm.com>
     [not found]         ` <alpine.DEB.2.00.0903140233510.457@chino.kir.corp.google.com>
2009-03-14 11:17           ` [PATCH] cpuset: fix cpuset vs PF_THREAD_BOUND weirdness Peter Zijlstra
2009-03-16 22:11             ` David Rientjes

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.