Re: [patch 3/4] mempolicy: add MPOL_F_STATIC_NODES flag

[Paul Jackson <pj@sgi.com>]

David wrote:
> But without MPOL_FLAG_SHIFT it becomes 
> impossible to determine whether a user passed an invalid flag.

I don't think so.  It's not possible to detemine if exactly the low
eight bits of the 'policy' short are a valid mode, -however- that
"eight" is a spurious detail.  Remove it.

Instead of specifying that the 'policy' short consists of two bytes,
one for mode and one for flags, rather specify that the policy fields
consists of some high bits for flags, and the remaining low bits
(however many remain) for the mode.

That in turn exposes the opportunity to remove a couple of checks
for "(mpol_flags(mode) & ~MPOL_MODE_FLAGS)" being zero, and that
in turn makes it obvious that the 'flags' variable in
sys_set_mempolicy() is unused.

Consider the following patch, on top of the latest you sent me a day
ago.

---
 include/linux/mempolicy.h |   26 +++++++++++++++-----------
 mm/mempolicy.c            |    6 ------
 2 files changed, 15 insertions(+), 17 deletions(-)

--- 2.6.24-mm1.orig/include/linux/mempolicy.h	2008-02-15 00:11:10.000000000 -0800
+++ 2.6.24-mm1/include/linux/mempolicy.h	2008-02-15 01:13:51.597894429 -0800
@@ -8,6 +8,14 @@
  * Copyright 2003,2004 Andi Kleen SuSE Labs
  */
 
+/*
+ * The 'policy' field of 'struct mempolicy' has both a mode and
+ * some flags packed into it.  The flags (MPOL_F_* below) occupy
+ * the high bit positions (MPOL_MODE_FLAGS), and the mempolicy
+ * modes (the "Policies" below) are encoded in the remaining low
+ * bit positions.
+ */
+
 /* Policies */
 enum {
 	MPOL_DEFAULT,
@@ -18,16 +26,12 @@ enum {
 };
 
 /*
- * The lower MPOL_FLAG_SHIFT bits of the policy mode represent the MPOL_*
- * constants defined in the above enum.  The upper bits represent optional
- * set_mempolicy() or mbind() MPOL_F_* mode flags.
+ * Optional flags that modify nodemask numbering.
  */
-#define MPOL_FLAG_SHIFT		(8)
-#define MPOL_MODE_MASK		((1 << MPOL_FLAG_SHIFT) - 1)
-
-/* Flags for set_mempolicy */
-#define MPOL_F_STATIC_NODES	(1 << MPOL_FLAG_SHIFT)
-#define MPOL_MODE_FLAGS		(MPOL_F_STATIC_NODES)	/* legal set_mempolicy() MPOL_F_* flags */
+#define MPOL_F_RELATIVE_NODES (1<<14)		/* remapped relative to cpuset */
+#define MPOL_F_STATIC_NODES (1<<15)		/* unremapped physical masks */
+#define MPOL_MODE_FLAGS (MPOL_F_RELATIVE_NODES|MPOL_F_STATIC_NODES)
+						/* combined MPOL_F_* mask flags */
 
 /* Flags for get_mempolicy */
 #define MPOL_F_NODE	(1<<0)	/* return next IL mode instead of node mask */
@@ -130,12 +134,12 @@ static inline int mpol_equal(struct memp
 
 static inline unsigned char mpol_mode(unsigned short mode)
 {
-	return mode & MPOL_MODE_MASK;
+	return mode & ~MPOL_MODE_FLAGS;
 }
 
 static inline unsigned short mpol_flags(unsigned short mode)
 {
-	return mode & ~MPOL_MODE_MASK;
+	return mode & MPOL_MODE_FLAGS;
 }
 
 /*
--- 2.6.24-mm1.orig/mm/mempolicy.c	2008-02-15 00:18:35.000000000 -0800
+++ 2.6.24-mm1/mm/mempolicy.c	2008-02-15 01:23:53.775748002 -0800
@@ -884,8 +884,6 @@ asmlinkage long sys_mbind(unsigned long 
 
 	if (mpol_mode(mode) >= MPOL_MAX)
 		return -EINVAL;
-	if (mpol_flags(mode) & ~MPOL_MODE_FLAGS)
-		return -EINVAL;
 	err = get_nodes(&nodes, nmask, maxnode);
 	if (err)
 		return err;
@@ -898,13 +896,9 @@ asmlinkage long sys_set_mempolicy(int mo
 {
 	int err;
 	nodemask_t nodes;
-	unsigned short flags;
 
 	if (mpol_mode(mode) >= MPOL_MAX)
 		return -EINVAL;
-	if (mpol_flags(mode) & ~MPOL_MODE_FLAGS)
-		return -EINVAL;
-	flags = mpol_flags(mode) & MPOL_MODE_FLAGS;
 	err = get_nodes(&nodes, nmask, maxnode);
 	if (err)
 		return err;


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