Regression: Re: [patch -mm 2/4] mempolicy: create mempolicy_operations structure

Regression: Re: [patch -mm 2/4] mempolicy: create mempolicy_operations structure

[Lee Schermerhorn]

The subject patch causes a regression in the numactl package mempolicy
regression tests.

I'm using the numactl-1.0.2 package with the patches available at:

http://free.linux.hp.com/~lts/Patches/Numactl/numactl-1.0.2-patches-080226.tar.gz

The numastat and regress patches in that tarball are necessary for
recent kernels, else the tests will report false failures.

The following patch fixes the regression.

Lee

----------------------------------------------------

Against:  2.6.25-rc3-mm1 atop the following patches, which Andrew
has already added to the -mm tree:

[patch 1/6] mempolicy: convert MPOL constants to enum
[patch 2/6] mempolicy: support optional mode flags
[patch 3/6] mempolicy: add MPOL_F_STATIC_NODES flag
[patch 4/6] mempolicy: add bitmap_onto() and bitmap_fold() operations
[patch 5/6] mempolicy: add MPOL_F_RELATIVE_NODES flag
[patch 6/6] mempolicy: update NUMA memory policy documentation
[patch -mm 1/4] mempolicy: move rebind functions
[patch -mm 2/4] mempolicy: create mempolicy_operations structure
[patch -mm 3/4] mempolicy: small header file cleanup

Specifically this patch fixes problems introduced by the rework
of mpol_new() in patch 2/4 in the second series.  As a result,
we're no longer accepting NULL/empty nodemask with MPOL_PREFERRED
as "local" allocation.  This breaks the numactl regression tests.

"numactl --localalloc" <some command>" fails with invalid argument.

This patch fixes the regression by again treating NULL/empty nodemask
with MPOL_PREFERRED as "local allocation", and special casing this
condition, as needed, in mpol_new(), mpol_new_preferred() and 
mpol_rebind_preferred().

It also appears that the patch series listed above required a non-empty
nodemask with MPOL_DEFAULT.  However, I didn't test that.  With this
patch, MPOL_DEFAULT effectively ignores the nodemask--empty or not.
This is a change in behavior that I have argued against, but the
regression tests don't test this, so I'm not going to attempt to address
it with this patch.

Note:  I have no tests to test the 'STATIC_NODES and 'RELATIVE_NODES
behavior to ensure that this patch doesn't regress those.

Signed-off-by: Lee Schermerhorn <lee.schermerhorn@hp.com>

 mm/mempolicy.c |   54 ++++++++++++++++++++++++++++++++++++------------------
 1 file changed, 36 insertions(+), 18 deletions(-)

Index: linux-2.6.25-rc3-mm1/mm/mempolicy.c
===================================================================
--- linux-2.6.25-rc3-mm1.orig/mm/mempolicy.c	2008-03-07 15:22:01.000000000 -0500
+++ linux-2.6.25-rc3-mm1/mm/mempolicy.c	2008-03-07 15:37:43.000000000 -0500
@@ -158,9 +158,12 @@ static int mpol_new_interleave(struct me
 
 static int mpol_new_preferred(struct mempolicy *pol, const nodemask_t *nodes)
 {
-	if (nodes_empty(*nodes))
-		return -EINVAL;
-	pol->v.preferred_node = first_node(*nodes);
+	if (!nodes)
+		pol->v.preferred_node = -1;	/* local allocation */
+	else if (nodes_empty(*nodes))
+		return -EINVAL;			/*  no allowed nodes */
+	else
+		pol->v.preferred_node = first_node(*nodes);
 	return 0;
 }
 
@@ -178,34 +181,43 @@ static struct mempolicy *mpol_new(unsign
 {
 	struct mempolicy *policy;
 	nodemask_t cpuset_context_nmask;
+	int localalloc = 0;
 	int ret;
 
 	pr_debug("setting mode %d flags %d nodes[0] %lx\n",
 		 mode, flags, nodes ? nodes_addr(*nodes)[0] : -1);
 
-	if (nodes && nodes_empty(*nodes) && mode != MPOL_PREFERRED)
-		return ERR_PTR(-EINVAL);
 	if (mode == MPOL_DEFAULT)
 		return NULL;
+	if (!nodes || nodes_empty(*nodes)) {
+		if (mode != MPOL_PREFERRED)
+			return ERR_PTR(-EINVAL);
+		localalloc = 1;	/* special case:  no mode flags */
+	}
 	policy = kmem_cache_alloc(policy_cache, GFP_KERNEL);
 	if (!policy)
 		return ERR_PTR(-ENOMEM);
 	atomic_set(&policy->refcnt, 1);
-	cpuset_update_task_memory_state();
-	if (flags & MPOL_F_RELATIVE_NODES)
-		mpol_relative_nodemask(&cpuset_context_nmask, nodes,
-				       &cpuset_current_mems_allowed);
-	else
-		nodes_and(cpuset_context_nmask, *nodes,
-			  cpuset_current_mems_allowed);
 	policy->policy = mode;
-	policy->flags = flags;
-	if (mpol_store_user_nodemask(policy))
-		policy->w.user_nodemask = *nodes;
-	else
-		policy->w.cpuset_mems_allowed = cpuset_mems_allowed(current);
 
-	ret = mpol_ops[mode].create(policy, &cpuset_context_nmask);
+	if (!localalloc) {
+		policy->flags = flags;
+		cpuset_update_task_memory_state();
+		if (flags & MPOL_F_RELATIVE_NODES)
+			mpol_relative_nodemask(&cpuset_context_nmask, nodes,
+					       &cpuset_current_mems_allowed);
+		else
+			nodes_and(cpuset_context_nmask, *nodes,
+				  cpuset_current_mems_allowed);
+		if (mpol_store_user_nodemask(policy))
+			policy->w.user_nodemask = *nodes;
+		else
+			policy->w.cpuset_mems_allowed =
+						cpuset_mems_allowed(current);
+	}
+
+	ret = mpol_ops[mode].create(policy,
+				localalloc ? NULL : &cpuset_context_nmask);
 	if (ret < 0) {
 		kmem_cache_free(policy_cache, policy);
 		return ERR_PTR(ret);
@@ -247,6 +259,10 @@ static void mpol_rebind_preferred(struct
 {
 	nodemask_t tmp;
 
+	/*
+	 * check 'STATIC_NODES first, as preferred_node == -1 may be
+	 * a temporary, "fallback" state for this policy.
+	 */
 	if (pol->flags & MPOL_F_STATIC_NODES) {
 		int node = first_node(pol->w.user_nodemask);
 
@@ -254,6 +270,8 @@ static void mpol_rebind_preferred(struct
 			pol->v.preferred_node = node;
 		else
 			pol->v.preferred_node = -1;
+	} else if (pol->v.preferred_node == -1) {
+		return;	/* no remap required for explicit local alloc */
 	} else if (pol->flags & MPOL_F_RELATIVE_NODES) {
 		mpol_relative_nodemask(&tmp, &pol->w.user_nodemask, nodes);
 		pol->v.preferred_node = first_node(tmp);


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Regression: Re: [patch -mm 2/4] mempolicy: create mempolicy_operations structure

[David Rientjes]

On Fri, 7 Mar 2008, Lee Schermerhorn wrote:

> It also appears that the patch series listed above required a non-empty
> nodemask with MPOL_DEFAULT.  However, I didn't test that.  With this
> patch, MPOL_DEFAULT effectively ignores the nodemask--empty or not.
> This is a change in behavior that I have argued against, but the
> regression tests don't test this, so I'm not going to attempt to address
> it with this patch.
> 

Excuse me, but there was significant discussion about this on LKML and I 
eventually did force MPOL_DEFAULT to require a non-empty nodemask 
specifically because of your demand that it should.  It didn't originally 
require this in my patchset, and now you're removing the exact same 
requirement that you demanded.

You said on February 13:

	1) we've discussed the issue of returning EINVAL for non-empty
	nodemasks with MPOL_DEFAULT.  By removing this restriction, we run
	the risk of breaking applications if we should ever want to define
	a semantic to non-empty node mask for MPOL_DEFAULT.

If you want to remove this requirement now (please get agreement from 
Paul) and are sure of your position, you'll at least need an update to 
Documentation/vm/numa-memory-policy.txt.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Regression: Re: [patch -mm 2/4] mempolicy: create mempolicy_operations structure

[Paul Jackson]

David wrote:
> If you want to remove this requirement now (please get agreement from Paul)

I'm ducking and running for cover ;).

Personally, I'm slightly in favor of not requiring the empty mask,
as I always that that empty mask check was a couple lines of non-
essential logic.  However I'm slightly in favor of not changing
this detail from what it has been for years, which would mean we
still checked for the empty mask.  And no doubt, if someone cares
to examine the record closely enough they will find where I took a
third position as well.

But I can't see where it actually matters enough to write home about.

So I'll quit writing, and agree to most anything.

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Regression: Re: [patch -mm 2/4] mempolicy: create mempolicy_operations structure

[Lee Schermerhorn]

On Fri, 2008-03-07 at 13:48 -0800, David Rientjes wrote: 
> On Fri, 7 Mar 2008, Lee Schermerhorn wrote:
> 
> > It also appears that the patch series listed above required a non-empty
> > nodemask with MPOL_DEFAULT.  However, I didn't test that.  With this
> > patch, MPOL_DEFAULT effectively ignores the nodemask--empty or not.
> > This is a change in behavior that I have argued against, but the
> > regression tests don't test this, so I'm not going to attempt to address
> > it with this patch.
> > 
> 
> Excuse me, but there was significant discussion about this on LKML and I 
> eventually did force MPOL_DEFAULT to require a non-empty nodemask 
> specifically because of your demand that it should.  It didn't originally 
> require this in my patchset, and now you're removing the exact same 
> requirement that you demanded.
> 
> You said on February 13:
> 
> 	1) we've discussed the issue of returning EINVAL for non-empty
> 	nodemasks with MPOL_DEFAULT.  By removing this restriction, we run
> 	the risk of breaking applications if we should ever want to define
> 	a semantic to non-empty node mask for MPOL_DEFAULT.
> 
> If you want to remove this requirement now (please get agreement from 
> Paul) and are sure of your position, you'll at least need an update to 
> Documentation/vm/numa-memory-policy.txt.

Excuse me.  I thought that the discussion--my position, anyway--was
about preserving existing behavior for MPOL_DEFAULT which is to require
an EMPTY [or NULL--same effect] nodemask.  Not a NON-EMPTY one.  See:
http://www.kernel.org/doc/man-pages/online/pages/man2/set_mempolicy.2.html
It does appear that your patches now require a non-empty nodemask.  This
was intentional?

Is it, then, the case that our disagreement was based on the fact that
you thought I was advocating a non-empty nodemask with MPOL_DEFAULT?  No
wonder you said it didn't make sense. 

Since we can't seem to understand each other with ~English prose, I've
attached a little test program that demonstrates the behavior that I
expect.   This is not to belabor the point; just an attempt to establish
understanding.

Note:  in the subject patch, I didn't enforce this behavior because your
patch didn't [it enforced just the opposite], and I've pretty much given
up.  Although I prefer current behavior [before your series], if we
change it, we will need to change the man pages to remove the error
condition for non-empty nodemasks with MPOL_DEFAULT.

Later,
Lee


/*
 * test error returns for set_mempolicy(MPOL_DEFAULT, nodemask, maxnodes) for
 * null, empty and non-empty nodemasks.
 *
 * requires libnuma
 */
#include <sys/types.h>

#include <errno.h>
#include <numaif.h>
#include <numa.h>
#include <stdarg.h>
#include <stdlib.h>
#include <stdio.h>
#include <string.h>
#include <unistd.h>

void results(int ret, int ierr, int expected)
{
	if (ret) {
		printf("\tResults:   %s [%d]\n", strerror(ierr), ierr);
	} else {
		printf("\tResults:   No Error [0]\n");
	}
	printf("\tExpected:  %s [%d]\n",
			expected ? strerror(expected) : "No Error", expected);
}

int main(int argc, char *argv[])
{
	unsigned long nodemask;	/* hack:  single long word mask */
	int maxnodes = 4;	/* arbitrary max <= 8 * sizeof(nodemask) */
	int ret;

	printf("\n1: testing set_mempolicy(MPOL_DEFAULT, ...) with NULL nodemask:\n");
	ret = set_mempolicy(MPOL_DEFAULT, NULL, maxnodes);
	results(ret, errno, 0);	/* expect success */

	printf("\n2: testing set_mempolicy(MPOL_DEFAULT, ...) with non-NULL, "
			"but empty, nodemask:\n");
	nodemask = 0UL;
	ret = set_mempolicy(MPOL_DEFAULT, &nodemask, maxnodes);
	results(ret, errno, 0);	/* expect success */

	printf("\n2: testing set_mempolicy(MPOL_DEFAULT, ...) with non-NULL, "
			"non-empty nodemask:\n");
	nodemask = 1UL;
	ret = set_mempolicy(MPOL_DEFAULT, &nodemask, maxnodes);
	results(ret, errno, EINVAL);	/* expect EINVAL */

}




--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Regression: Re: [patch -mm 2/4] mempolicy: create mempolicy_operations structure

[David Rientjes]

On Sat, 8 Mar 2008, Lee Schermerhorn wrote:

> > Excuse me, but there was significant discussion about this on LKML and I 
> > eventually did force MPOL_DEFAULT to require a non-empty nodemask 

Correction: s/non-empty/empty

> > specifically because of your demand that it should.  It didn't originally 
> > require this in my patchset, and now you're removing the exact same 
> > requirement that you demanded.
> > 
> > You said on February 13:
> > 
> > 	1) we've discussed the issue of returning EINVAL for non-empty
> > 	nodemasks with MPOL_DEFAULT.  By removing this restriction, we run
> > 	the risk of breaking applications if we should ever want to define
> > 	a semantic to non-empty node mask for MPOL_DEFAULT.
> > 
> > If you want to remove this requirement now (please get agreement from 
> > Paul) and are sure of your position, you'll at least need an update to 
> > Documentation/vm/numa-memory-policy.txt.
> 
> Excuse me.  I thought that the discussion--my position, anyway--was
> about preserving existing behavior for MPOL_DEFAULT which is to require
> an EMPTY [or NULL--same effect] nodemask.  Not a NON-EMPTY one.  See:
> http://www.kernel.org/doc/man-pages/online/pages/man2/set_mempolicy.2.html
> It does appear that your patches now require a non-empty nodemask.  This
> was intentional?
> 

The first and second set did not have this requirement, but the third set 
does (not currently in -mm), so I've changed it back.  Hopefully there's 
no confusion and we can settle on a solution without continuously 
revisiting the topic.

My position was originally to allow any type of nodemask to be passed with 
MPOL_DEFAULT since its not used.  You asked for strict argument checking 
and so after some debate I changed it to require an empty nodemask mainly 
because I didn't want the patchset to stall on such a minor point.  But in 
your regression fix, you expressed the desire once again to allow it to 
accept any nodemask because the testsuite does not check for it.

So if you'd like to do that, I'd encourage you to submit it as a separate 
patch and open it up for review.

What is currently in -mm and what I will be posting shortly is the updated 
regression fix.  All of these patches require that MPOL_DEFAULT include a 
NULL pointer or empty nodemask passed via the two syscalls.

> Note:  in the subject patch, I didn't enforce this behavior because your
> patch didn't [it enforced just the opposite], and I've pretty much given
> up.  Although I prefer current behavior [before your series], if we
> change it, we will need to change the man pages to remove the error
> condition for non-empty nodemasks with MPOL_DEFAULT.
> 

With my patches it still requires a NULL pointer or empty nodemask and 
I've updated Documentation/vm/numa_memory_policy.txt to explicitly say its 
an error if a non-empty nodemask is passed.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Regression: Re: [patch -mm 2/4] mempolicy: create mempolicy_operations structure

[Lee Schermerhorn]

On Sat, 2008-03-08 at 14:09 -0800, David Rientjes wrote: 
> On Sat, 8 Mar 2008, Lee Schermerhorn wrote:
> 
> > > Excuse me, but there was significant discussion about this on LKML and I 
> > > eventually did force MPOL_DEFAULT to require a non-empty nodemask 
> 
> Correction: s/non-empty/empty

That makes more sense.  I agree.  more below... 
> 
> > > specifically because of your demand that it should.  It didn't originally 
> > > require this in my patchset, and now you're removing the exact same 
> > > requirement that you demanded.
> > > 
> > > You said on February 13:
> > > 
> > > 	1) we've discussed the issue of returning EINVAL for non-empty
> > > 	nodemasks with MPOL_DEFAULT.  By removing this restriction, we run
> > > 	the risk of breaking applications if we should ever want to define
> > > 	a semantic to non-empty node mask for MPOL_DEFAULT.
> > > 
> > > If you want to remove this requirement now (please get agreement from 
> > > Paul) and are sure of your position, you'll at least need an update to 
> > > Documentation/vm/numa-memory-policy.txt.
> > 
> > Excuse me.  I thought that the discussion--my position, anyway--was
> > about preserving existing behavior for MPOL_DEFAULT which is to require
> > an EMPTY [or NULL--same effect] nodemask.  Not a NON-EMPTY one.  See:
> > http://www.kernel.org/doc/man-pages/online/pages/man2/set_mempolicy.2.html
> > It does appear that your patches now require a non-empty nodemask.  This
> > was intentional?
> > 
> 
> The first and second set did not have this requirement, but the third set 
> does (not currently in -mm), so I've changed it back.  Hopefully there's 
> no confusion and we can settle on a solution without continuously 
> revisiting the topic.
> 
> My position was originally to allow any type of nodemask to be passed with 
> MPOL_DEFAULT since its not used.  You asked for strict argument checking 
> and so after some debate I changed it to require an empty nodemask mainly 
> because I didn't want the patchset to stall on such a minor point.  But in 
> your regression fix, you expressed the desire once again to allow it to 
> accept any nodemask because the testsuite does not check for it.

Not a desire.  Just that when I fixed the MPOL_PREFERRED with empty node
mask regression, I also fixed mpol_new() not to require a non-empty
nodemask with MPOL_DEFAULT.  I didn't go the extra step to require an
empty one.  I'm tiring of the subject, as I think you are, and didn't
want to argue it anymore.  So, I was willing to "cave" on that point.

> 
> So if you'd like to do that, I'd encourage you to submit it as a separate 
> patch and open it up for review.

No, I'm quite happy if, after your patches, the APIs retain the previous
behavior w/rt nodemask error checking.

> 
> What is currently in -mm and what I will be posting shortly is the updated 
> regression fix.  All of these patches require that MPOL_DEFAULT include a 
> NULL pointer or empty nodemask passed via the two syscalls.
> 
> > Note:  in the subject patch, I didn't enforce this behavior because your
> > patch didn't [it enforced just the opposite], and I've pretty much given
> > up.  Although I prefer current behavior [before your series], if we
> > change it, we will need to change the man pages to remove the error
> > condition for non-empty nodemasks with MPOL_DEFAULT.
> > 
> 
> With my patches it still requires a NULL pointer or empty nodemask and 
> I've updated Documentation/vm/numa_memory_policy.txt to explicitly say its 
> an error if a non-empty nodemask is passed.

Good.

Do you intend for your patch entitled "[patch -mm v2] mempolicy:
disallow static or relative flags for local preferred mode" to replace
the patch that I sent in to repair the regression?  Looks that way.
I'll replace it in my tree and retest.

Lee




--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH] Mempolicy: fix parsing of tmpfs mpol mount option

[Lee Schermerhorn]

Against:  2.6.25-rc5-mm1 atop:
+ mempolicy-disallow-static-or-relative-flags-for-local-preferred-mode
+ mempolicy-support-optional-mode-flags-fix [Hugh]

Parsing of new mode flags in the tmpfs mpol mount option is
slightly broken:

Setting a valid flag works OK:
	#mount -o remount,mpol=bind=static:1-2 /dev/shm
	#mount
	...
	tmpfs on /dev/shm type tmpfs (rw,mpol=bind=static:1-2)
	...

However, we can't remove them or change them, once we've
set a valid flag:

	#mount -o remount,mpol=bind:1-2 /dev/shm
	#mount
	...
	tmpfs on /dev/shm type tmpfs (rw,mpol=bind:1-2)
	...

It SAYS it removed it, but that's just a copy of the input
string.  If we now try to set it to a different flag, we
get:

	#mount -o remount,mpol=bind=relative:1-2 /dev/shm
	mount: /dev/shm not mounted already, or bad option

And on the console, we see:
	tmpfs: Bad value 'bind' for mount option 'mpol'
	                      ^ lost remainder of string

Furthermore, bogus flags are accepted with out error.
Granted, they are a no-op:

	#mount -o remount,mpol=interleave=foo:0-3 /dev/shm
	#mount
	...
	tmpfs on /dev/shm type tmpfs (rw,mpol=interleave=foo:0-3)

Again, that's just a copy of the input string shown by the
mount command.

This patch fixes the behavior by pre-zeroing the flags so that
only one of the mutually exclusive flags can be set at one time.
It also reports an error when an unrecognized flag is specified.

The check for both flags being set is removed because it can't
happen with this implementation.  If we ever want to support
multiple non-exclusive flags, this area will need rework and we
will need to check that any mutually exclusive flags aren't
specified.

Signed-off-by:  Lee Schermerhorn <lee.schermerhorn@hp.com>

 mm/shmem.c |   16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

Index: linux-2.6.25-rc5-mm1/mm/shmem.c
===================================================================
--- linux-2.6.25-rc5-mm1.orig/mm/shmem.c	2008-03-12 14:15:27.000000000 -0400
+++ linux-2.6.25-rc5-mm1/mm/shmem.c	2008-03-12 14:18:09.000000000 -0400
@@ -1128,20 +1128,26 @@ static int shmem_parse_mpol(char *value,
 			*policy_nodes = node_states[N_HIGH_MEMORY];
 		err = 0;
 	}
+
+	*mode_flags = 0;
 	if (flags) {
+		/*
+		 * Currently, we only support two mutually exclusive
+		 * mode flags.
+		 */
 		if (!strcmp(flags, "static"))
 			*mode_flags |= MPOL_F_STATIC_NODES;
-		if (!strcmp(flags, "relative"))
+		else if (!strcmp(flags, "relative"))
 			*mode_flags |= MPOL_F_RELATIVE_NODES;
-
-		if ((*mode_flags & MPOL_F_STATIC_NODES) &&
-		    (*mode_flags & MPOL_F_RELATIVE_NODES))
-			err = 1;
+		else
+			err = 1;	/* unrecognized flag */
 	}
 out:
 	/* Restore string for error message */
 	if (nodelist)
 		*--nodelist = ':';
+	if (flags)
+		*--flags = '=';
 	return err;
 }
 


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>