[PATCH 0/6] Mempolicy: additional cleanups

[PATCH 0/6] Mempolicy: additional cleanups

[Lee Schermerhorn]

Here is a series of proposed memory policy cleanup patches, mostly
in the 'mpol' mount option parsing function 'mpol_parse_str()'.  I
came across these cleanup opportunities reviewing and testing
Kosaki Motohiro's 5 patch tmpfs series from 16mar.  This series applies
atop Kosaki-san's series.

Patch 5 of the series is more of a bug fix to get_mempolicy() discovered
while testing the other patches.

[PATCH 1/6] Mempolicy: Don't call mpol_set_nodemask() when no_context

[Lee Schermerhorn]

Atop Kosaki Motohiro's mpol_parse_str() cleanup series.

No need to call mpol_set_nodemask() when we have no context for
the mempolicy.  This can occur when we're parsing a tmpfs 'mpol'
mount option.  Just save the raw nodemask in the mempolicy's
w.user_nodemask member for use when a tmpfs/shmem file is
created.  mpol_shared_policy_init() will "contextualize" the
policy for the new file based on the creating task's context.

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

 mm/mempolicy.c |    9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

Index: linux-2.6.34-rc1-mmotm-100311-1313/mm/mempolicy.c
===================================================================
--- linux-2.6.34-rc1-mmotm-100311-1313.orig/mm/mempolicy.c	2010-03-19 09:03:14.000000000 -0400
+++ linux-2.6.34-rc1-mmotm-100311-1313/mm/mempolicy.c	2010-03-19 09:03:17.000000000 -0400
@@ -2245,7 +2245,10 @@ int mpol_parse_str(char *str, struct mem
 	if (IS_ERR(new))
 		goto out;
 
-	{
+	if (no_context) {
+		/* save for contextualization */
+		new->w.user_nodemask = nodes;
+	} else {
 		int ret;
 		NODEMASK_SCRATCH(scratch);
 		if (scratch) {
@@ -2261,10 +2264,6 @@ int mpol_parse_str(char *str, struct mem
 		}
 	}
 	err = 0;
-	if (no_context) {
-		/* save for contextualization */
-		new->w.user_nodemask = nodes;
-	}
 
 out:
 	/* Restore string for error message */

Re: [PATCH 1/6] Mempolicy: Don't call mpol_set_nodemask() when no_context

[Christoph Lameter]

Just use i instead of mode? Local variables typically have short names.
"mode" sounds like a parameter. But its just style so ignore my comments
if you want.

--
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: [PATCH 1/6] Mempolicy: Don't call mpol_set_nodemask() when no_context

[Lee Schermerhorn]

On Mon, 2010-03-22 at 09:40 -0500, Christoph Lameter wrote:
> Just use i instead of mode? Local variables typically have short names.
> "mode" sounds like a parameter. 

That was probably my thinking when I used 'i' for the loop variable back
when I replaced the 'if-elseif' skip chain with the for loop to reuse
the [then] "policy_types[]" array from mpol_to_str().  But, I then went
and assigned it to the more meaningful 'mode' to carry around the rest
of function.  Looking at it this time around, seemed like I didn't need
2 vars, and outside of the loop, I thought 'mode' would be
better--reinforcing the nomenclature.

> But its just
>  style so ignore my comments
> if you want.
> 

Aw, would I do that?  But, let's wait to see if anyone else weighs in.
I'm not wedded to either one, except for the rationale, such as it is,
mentioned above.

Thanks for the review.

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 2/6] Mempolicy: Lose unnecessary loop variable in mpol_parse_str()

[Lee Schermerhorn]

We don't really need the extra variable 'i' in mpol_parse_str().
The only use is as the the loop variable.  Then, it's assigned
to 'mode'.  Just use mode, and loose the 'uninitialized_var()'
macro.

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

 mm/mempolicy.c |   10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

Index: linux-2.6.34-rc1-mmotm-100311-1313/mm/mempolicy.c
===================================================================
--- linux-2.6.34-rc1-mmotm-100311-1313.orig/mm/mempolicy.c	2010-03-19 09:03:17.000000000 -0400
+++ linux-2.6.34-rc1-mmotm-100311-1313/mm/mempolicy.c	2010-03-19 09:03:21.000000000 -0400
@@ -2154,12 +2154,11 @@ static const char * const policy_types[]
 int mpol_parse_str(char *str, struct mempolicy **mpol, int no_context)
 {
 	struct mempolicy *new = NULL;
-	unsigned short uninitialized_var(mode);
+	unsigned short mode;
 	unsigned short uninitialized_var(mode_flags);
 	nodemask_t nodes;
 	char *nodelist = strchr(str, ':');
 	char *flags = strchr(str, '=');
-	int i;
 	int err = 1;
 
 	if (nodelist) {
@@ -2175,13 +2174,12 @@ int mpol_parse_str(char *str, struct mem
 	if (flags)
 		*flags++ = '\0';	/* terminate mode string */
 
-	for (i = 0; i <= MPOL_LOCAL; i++) {
-		if (!strcmp(str, policy_types[i])) {
-			mode = i;
+	for (mode = 0; mode <= MPOL_LOCAL; mode++) {
+		if (!strcmp(str, policy_types[mode])) {
 			break;
 		}
 	}
-	if (i > MPOL_LOCAL)
+	if (mode > MPOL_LOCAL)
 		goto out;
 
 	switch (mode) {

[PATCH 3/6] Mempolicy: rename policy_types and cleanup initialization

[Lee Schermerhorn]

Rename 'policy_types[]' to 'policy_modes[]' to better match the
array contents.

Use designated intializer syntax for policy_modes[].

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

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

Index: linux-2.6.34-rc1-mmotm-100311-1313/mm/mempolicy.c
===================================================================
--- linux-2.6.34-rc1-mmotm-100311-1313.orig/mm/mempolicy.c	2010-03-19 09:03:21.000000000 -0400
+++ linux-2.6.34-rc1-mmotm-100311-1313/mm/mempolicy.c	2010-03-19 12:22:17.000000000 -0400
@@ -2127,9 +2127,15 @@ void numa_default_policy(void)
  * "local" is pseudo-policy:  MPOL_PREFERRED with MPOL_F_LOCAL flag
  * Used only for mpol_parse_str() and mpol_to_str()
  */
-#define MPOL_LOCAL (MPOL_INTERLEAVE + 1)
-static const char * const policy_types[] =
-	{ "default", "prefer", "bind", "interleave", "local" };
+#define MPOL_LOCAL MPOL_MAX
+static const char * const policy_modes[] =
+{
+ 	[MPOL_DEFAULT]    = "default",
+ 	[MPOL_PREFERRED]  = "prefer",
+	[MPOL_BIND]       = "bind",
+	[MPOL_INTERLEAVE] = "interleave",
+	[MPOL_LOCAL]      = "local"
+};
 
 
 #ifdef CONFIG_TMPFS
@@ -2175,7 +2181,7 @@ int mpol_parse_str(char *str, struct mem
 		*flags++ = '\0';	/* terminate mode string */
 
 	for (mode = 0; mode <= MPOL_LOCAL; mode++) {
-		if (!strcmp(str, policy_types[mode])) {
+		if (!strcmp(str, policy_modes[mode])) {
 			break;
 		}
 	}
@@ -2330,11 +2336,11 @@ int mpol_to_str(char *buffer, int maxlen
 		BUG();
 	}
 
-	l = strlen(policy_types[mode]);
+	l = strlen(policy_modes[mode]);
 	if (buffer + maxlen < p + l + 1)
 		return -ENOSPC;
 
-	strcpy(p, policy_types[mode]);
+	strcpy(p, policy_modes[mode]);
 	p += l;
 
 	if (flags & MPOL_MODE_FLAGS) {

Re: [PATCH 3/6] Mempolicy: rename policy_types and cleanup initialization

[Christoph Lameter]

On Fri, 19 Mar 2010, Lee Schermerhorn wrote:

> Rename 'policy_types[]' to 'policy_modes[]' to better match the
> array contents.

Reviewed-by: Christoph Lameter <cl@linux-foundation.org>

Small nitpick: MPOL_MAX should be called MPOL_NR to follow vmstat.h and
mmzones.h's way of naming the n+1st element.

--
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: [PATCH 3/6] Mempolicy: rename policy_types and cleanup initialization

[Lee Schermerhorn]

On Mon, 2010-03-22 at 09:43 -0500, Christoph Lameter wrote:
> On Fri, 19 Mar 2010, Lee Schermerhorn wrote:
> 
> > Rename 'policy_types[]' to 'policy_modes[]' to better match the
> > array contents.
> 
> Reviewed-by: Christoph Lameter <cl@linux-foundation.org>
> 
> Small nitpick: MPOL_MAX should be called MPOL_NR to follow vmstat.h and
> mmzones.h's way of naming the n+1st element.

The 'MPOL_MAX' has been there since David R [wasn't it?] created the
enum.  The current name shows up in user space numaif.h from the numactl
package [2.0.3] as a #define of MPOL_MAX to MPOL_INTERLEAVE .  I suppose
we could #define it to MPOL_NR to avoid the possibility of application
breakage if the enum ever makes it to the user space header.   I checked
numactl sources and the only use of MPOL_MAX in the package is one of
the test programs.  Don't know about end user apps out there in the
wild, tho'.

David:  what do you think?

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 4/6] Mempolicy: factor mpol_shared_policy_init() return paths

[Lee Schermerhorn]

Factor out duplicate put/frees in mpol_shared_policy_init() to
a common return path.

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

 mm/mempolicy.c |   16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

Index: linux-2.6.34-rc1-mmotm-100311-1313/mm/mempolicy.c
===================================================================
--- linux-2.6.34-rc1-mmotm-100311-1313.orig/mm/mempolicy.c	2010-03-19 09:03:22.000000000 -0400
+++ linux-2.6.34-rc1-mmotm-100311-1313/mm/mempolicy.c	2010-03-19 09:06:09.000000000 -0400
@@ -2001,26 +2001,22 @@ void mpol_shared_policy_init(struct shar
 			return;
 		/* contextualize the tmpfs mount point mempolicy */
 		new = mpol_new(mpol->mode, mpol->flags, &mpol->w.user_nodemask);
-		if (IS_ERR(new)) {
-			mpol_put(mpol);	/* drop our ref on sb mpol */
-			NODEMASK_SCRATCH_FREE(scratch);
-			return;		/* no valid nodemask intersection */
-		}
+		if (IS_ERR(new))
+			goto put_free; /* no valid nodemask intersection */
 
 		task_lock(current);
 		ret = mpol_set_nodemask(new, &mpol->w.user_nodemask, scratch);
 		task_unlock(current);
 		mpol_put(mpol);	/* drop our ref on sb mpol */
-		if (ret) {
-			NODEMASK_SCRATCH_FREE(scratch);
-			mpol_put(new);
-			return;
-		}
+		if (ret)
+			goto put_free;
 
 		/* Create pseudo-vma that contains just the policy */
 		memset(&pvma, 0, sizeof(struct vm_area_struct));
 		pvma.vm_end = TASK_SIZE;	/* policy covers entire file */
 		mpol_set_shared_policy(sp, &pvma, new); /* adds ref */
+
+put_free:
 		mpol_put(new);			/* drop initial ref */
 		NODEMASK_SCRATCH_FREE(scratch);
 	}

Re: [PATCH 4/6] Mempolicy: factor mpol_shared_policy_init() return paths

[Christoph Lameter]

Reviewed-by: Christoph Lameter <cl@linux-foundation.org>


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

[BUGFIX][PATCH 5/6] Mempolicy: fix get_mempolicy() for relative and static nodes

[Lee Schermerhorn]

Discovered while testing other mempolicy changes:

get_mempolicy() does not handle static/relative mode flags correctly.
Return the value that the user specified so that it can be restored
via set_mempolicy() if desired.

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

 mm/mempolicy.c |   10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Index: linux-2.6.34-rc1-mmotm-100311-1313/mm/mempolicy.c
===================================================================
--- linux-2.6.34-rc1-mmotm-100311-1313.orig/mm/mempolicy.c	2010-03-19 09:06:09.000000000 -0400
+++ linux-2.6.34-rc1-mmotm-100311-1313/mm/mempolicy.c	2010-03-19 09:23:29.000000000 -0400
@@ -806,9 +806,13 @@ static long do_get_mempolicy(int *policy
 
 	err = 0;
 	if (nmask) {
-		task_lock(current);
-		get_policy_nodemask(pol, nmask);
-		task_unlock(current);
+		if (mpol_store_user_nodemask(pol)) {
+			*nmask = pol->w.user_nodemask;
+		} else {
+			task_lock(current);
+			get_policy_nodemask(pol, nmask);
+			task_unlock(current);
+		}
 	}
 
  out:

[PATCH 6/6] Mempolicy: document cpuset interaction with tmpfs mpol mount option

[Lee Schermerhorn]

Update Documentation/filesystems/tmpfs.txt to describe the
interaction of tmpfs mount option memory policy with tasks'
cpuset mems_allowed.

Note:  the mount(8) man page [in the util-linux-ng package]
requires similiar updates.

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

 Documentation/filesystems/tmpfs.txt |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

Index: linux-2.6.34-rc1-mmotm-100311-1313/Documentation/filesystems/tmpfs.txt
===================================================================
--- linux-2.6.34-rc1-mmotm-100311-1313.orig/Documentation/filesystems/tmpfs.txt	2010-03-19 09:06:15.000000000 -0400
+++ linux-2.6.34-rc1-mmotm-100311-1313/Documentation/filesystems/tmpfs.txt	2010-03-19 11:22:37.000000000 -0400
@@ -94,11 +94,19 @@ NodeList format is a comma-separated lis
 a range being two hyphen-separated decimal numbers, the smallest and
 largest node numbers in the range.  For example, mpol=bind:0-3,5,7,9-15
 
+A memory policy with a valid NodeList will be saved, as specified, for
+use at file creation time.  When a task allocates a file in the file
+system, the mount option memory policy will be applied with a NodeList,
+if any, modified by the calling task's cpuset constraints
+[See Documentation/cgroups/cpusets.txt] and any optional flags, listed
+below.  If the resulting NodeLists is the empty set, the effective memory
+policy for the file will revert to "default" policy.
+
 NUMA memory allocation policies have optional flags that can be used in
 conjunction with their modes.  These optional flags can be specified
 when tmpfs is mounted by appending them to the mode before the NodeList.
 See Documentation/vm/numa_memory_policy.txt for a list of all available
-memory allocation policy mode flags.
+memory allocation policy mode flags and their effect on memory policy.
 
 	=static		is equivalent to	MPOL_F_STATIC_NODES
 	=relative	is equivalent to	MPOL_F_RELATIVE_NODES