[patch] cpusets: allow empty {cpus,mems}_allowed to be set for unpopulated cpuset

[patch] cpusets: allow empty {cpus,mems}_allowed to be set for unpopulated cpuset

[David Rientjes]

You currently cannot remove all cpus or mems from cpus_allowed or
mems_allowed of a cpuset.  We now allow both if there are no attached
tasks.

Cc: Paul Jackson <pj@sgi.com>
Cc: Christoph Lameter <clameter@engr.sgi.com>
Signed-off-by: Paul Menage <menage@google.com>
Signed-off-by: David Rientjes <rientjes@google.com>
---
 kernel/cpuset.c |   38 ++++++++++++++++++++++++++++++--------
 1 files changed, 30 insertions(+), 8 deletions(-)

diff --git a/kernel/cpuset.c b/kernel/cpuset.c
--- a/kernel/cpuset.c
+++ b/kernel/cpuset.c
@@ -822,11 +822,22 @@ static int update_cpumask(struct cpuset *cs, char *buf)
 		return -EACCES;
 
 	trialcs = *cs;
-	retval = cpulist_parse(buf, trialcs.cpus_allowed);
-	if (retval < 0)
-		return retval;
+
+	/*
+	 * We allow a cpuset's cpus_allowed to be empty; if it has attached
+	 * tasks, we'll catch it later when we validate the change and return
+	 * -ENOSPC.
+	 */
+	if (!*buf) {
+		cpus_clear(trialcs.cpus_allowed);
+	} else {
+		retval = cpulist_parse(buf, trialcs.cpus_allowed);
+		if (retval < 0)
+			return retval;
+	}
 	cpus_and(trialcs.cpus_allowed, trialcs.cpus_allowed, cpu_online_map);
-	if (cpus_empty(trialcs.cpus_allowed))
+	/* cpus_allowed cannot be empty for a cpuset with attached tasks. */
+	if (atomic_read(&cs->count) && cpus_empty(trialcs.cpus_allowed))
 		return -ENOSPC;
 	retval = validate_change(cs, &trialcs);
 	if (retval < 0)
@@ -919,16 +930,27 @@ static int update_nodemask(struct cpuset *cs, char *buf)
 		return -EACCES;
 
 	trialcs = *cs;
-	retval = nodelist_parse(buf, trialcs.mems_allowed);
-	if (retval < 0)
-		goto done;
+
+	/*
+	 * We allow a cpuset's mems_allowed to be empty; if it has attached
+	 * tasks, we'll catch it later when we validate the change and return
+	 * -ENOSPC.
+	 */
+	if (!*buf) {
+		nodes_clear(trialcs.mems_allowed);
+	} else {
+		retval = nodelist_parse(buf, trialcs.mems_allowed);
+		if (retval < 0)
+			goto done;
+	}
 	nodes_and(trialcs.mems_allowed, trialcs.mems_allowed, node_online_map);
 	oldmem = cs->mems_allowed;
 	if (nodes_equal(oldmem, trialcs.mems_allowed)) {
 		retval = 0;		/* Too easy - nothing to do */
 		goto done;
 	}
-	if (nodes_empty(trialcs.mems_allowed)) {
+	/* mems_allowed cannot be empty for a cpuset with attached tasks. */
+	if (atomic_read(&cs->count) && nodes_empty(trialcs.mems_allowed)) {
 		retval = -ENOSPC;
 		goto done;
 	}

Re: [patch] cpusets: allow empty {cpus,mems}_allowed to be set for unpopulated cpuset

[Paul Jackson]

David wrote:
> You currently cannot remove all cpus or mems from cpus_allowed or
> mems_allowed of a cpuset.  We now allow both if there are no attached
> tasks.

Why do you need this?  It adds a little more code, and changes
semantics a little bit, so I'd think it should have at least a
little bit of justfication.


+	if (!*buf) {
+		cpus_clear(trialcs.cpus_allowed);

Won't the above code fail if someone does:

	echo > /dev/cpuset/foobar/mems

Just guessing, but I'd expect buf[] to contain a newline char,
not just a zero length string, at this point.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.925.600.0401

Re: [patch] cpusets: allow empty {cpus,mems}_allowed to be set for unpopulated cpuset

[Paul Menage]

On 5/1/07, Paul Jackson <pj@sgi.com> wrote:
> Why do you need this?  It adds a little more code, and changes
> semantics a little bit, so I'd think it should have at least a
> little bit of justfication.

We have cases where we'd like to be able to clear the memory nodes
away from a (temporarily) empty cpuset without actually deleting the
directory - there's really no reason for the interface to stop people
from doing that as far as I can see. Otherwise the only way to reclaim
the node for a different sibling is to delete the cpuset.

>
>
> +       if (!*buf) {
> +               cpus_clear(trialcs.cpus_allowed);
>
> Won't the above code fail if someone does:
>
>         echo > /dev/cpuset/foobar/mems
>
> Just guessing, but I'd expect buf[] to contain a newline char,
> not just a zero length string, at this point.

Yes, but that's arguably an artefact of the user using the wrong tool
to update the cpu/node set. Doing "echo -n > /dev/cpuset/foobar/mems"
has the expected effect.

Paul

Re: [patch] cpusets: allow empty {cpus,mems}_allowed to be set for unpopulated cpuset

[Paul Jackson]

Paul M wrote:
> Otherwise the only way to reclaim
> the node for a different sibling is to delete the cpuset.

I couldn't make sense of that sentence.  Could you restate it?

> Yes, but that's arguably an artefact of the user using the wrong tool
> to update the cpu/node set. Doing "echo -n > /dev/cpuset/foobar/mems"
> has the expected effect.

While it is true that 'echo -n' works here, I think that this will
cause confusion and irritation to users.  We have gone out of our way
for years now to support newlines as optional trailing characters on
input to the various bitmask formats, and to provide the newline on
output, in order to provide a comfortable interface for use from shell
scripts and prompts.

I think it would be an annoying inconsistency to not do so here.

I'd vote for adding a couple of lines of code to handle this:

+	char *bp;

+	bp = buf;
+	while (isspace(*bp))
+		bp++;
+	if (!*bp) {
+		cpus_clear(trialcs.cpus_allowed);
+	} else {

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.925.600.0401

Re: [patch] cpusets: allow empty {cpus,mems}_allowed to be set for unpopulated cpuset

[Paul Jackson]

Paul M wrote:
> Otherwise the only way to reclaim
> the node for a different sibling is to delete the cpuset.

Ah - I just made sense of that sentence.

It means that if a particular memory node is in one cpuset, and you'd
like to have it in another cpuset instead, then with the existing
kernel code, you had to special case the situation where this memory
node was the last node in the original cpuset - deleting the cpuset
just to do this.

Yeah - I have never given much thought to moving memory nodes from one
cpuset to another.  No good reason; it just didn't happen to be a
common operation for the uses of cpusets I cared about.

I still have this niggling fear that there was something that passed my
view, years ago, that this proposed change (to allow unpopulating a
cpuset) will break.

But I'll be damned if I can remember what it was.

Ok ... if this patch passes my cpuset_test (guess I'll try that now)
then I'm ok with this patch.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.925.600.0401

Re: [patch] cpusets: allow empty {cpus,mems}_allowed to be set for unpopulated cpuset

[Paul Jackson]

Other than the detail of allowing a newline from doing:

	echo > cpus

to work, I'm ok with this patch.  It passes my cpuset_test,
and seems to allow unpopulating cpusets, as advertised.

Aha - as I was writing this, I noticed that the command:

  echo -n '' > cpus

does -not- work!  The echo command recognizes that as a write
of zero non-null bytes, and skips the write altogether.

We have to add the code to handle an input line consisting of
just a bare newline, to mean an empty mask.  Well, we don't
-have- to.  But writing a single nul byte in shell script will
challenge most shell script hackers.

-- 
                  I won't rest till it's the best ...
                  Programmer, Linux Scalability
                  Paul Jackson <pj@sgi.com> 1.925.600.0401