Memory Policy Cleanups and Enhancements

PATCHRFC 1/5 Memory Policy: fix reference counting

Against 2.6.23-rc3-mm1

This patch proposes fixes to the reference counting of memory policy in the page allocation paths and in show_numa_map().

Shared policy lookup [shmem] has always added a reference to the policy, but this was never unrefed after page allocation or after formatting the numa map data.

Default system policy should not require additional ref counting, nor should the current task's task policy. However, show_numa_map() calls get_vma_policy() to examine what may be [likely is] another task's policy. The latter case needs protection against freeing of the policy.

This patch adds a reference count to a mempolicy returned by get_vma_policy() when the policy is a vma policy or another task's mempolicy. Again, shared policy is already reference counted on lookup. A matching "unref" [__mpol_free()] is performed in alloc_page_vma() for shared and vma policies, and in show_numa_map() for shared and another task's mempolicy. We can call __mpol_free() directly, saving an admittedly inexpensive inline NULL test, because we know we have a non-NULL policy.

Handling policy ref counts for hugepages is a bit tricker. huge_zonelist() returns a zone list that might come from a shared or vma 'BIND policy. In this case, we should hold the reference until after the huge page allocation in dequeue_hugepage(). The patch modifies huge_zonelist() to return a pointer to the mempolicy if it needs to be unref'd after allocation.

Signed-off-by Lee Schermerhorn <[email protected]>

 include/linux/mempolicy.h |    4 +-
 mm/hugetlb.c              |    4 +-
 mm/mempolicy.c            |   79 ++++++++++++++++++++++++++++++++++++++++------
 3 files changed, 75 insertions(+), 12 deletions(-)

Index: Linux/mm/mempolicy.c
===================================================================

mm/mempolicy.c +13 -1

@@ -1083,21 +1083,37 @@ asmlinkage long compat_sys_mbind(compat_
 
 #endif
 
-/* Return effective policy for a VMA */
+/*
+ * get_vma_policy(@task, @vma, @addr)
+ * @task - task for fallback if vma policy == default
+ * @vma   - virtual memory area whose policy is sought
+ * @addr  - address in @vma for shared policy lookup
+ *
+ * Returns effective policy for a VMA at specified address.
+ * Falls back to @task or system default policy, as necessary.
+ * Returned policy has extra reference count if shared, vma,
+ * or some other task's policy [show_numa_maps() can pass
+ * @task != current].  It is the caller's responsibility to
+ * free the reference in these cases.
+ */
 static struct mempolicy * get_vma_policy(struct task_struct *task,
 		struct vm_area_struct *vma, unsigned long addr)

mm/mempolicy.c +3 -0

+		} else if (vma->vm_policy &&
 				vma->vm_policy->policy != MPOL_DEFAULT)
 			pol = vma->vm_policy;
 	}
 	if (!pol)
 		pol = &default_policy;
+	else if (!shared_pol && pol != current->mempolicy)
+		mpol_get(pol);	/* vma or other task's policy */
 	return pol;
 }
 

@@ -1213,19 +1229,45 @@ static inline unsigned interleave_nid(st
 }
 
 #ifdef CONFIG_HUGETLBFS

mm/mempolicy.c +8 -1

+ * If it is also a policy for which get_vma_policy() returns an extra
+ * reference, we must hold that reference until after allocation.
+ * In that case, return policy via @mpol so hugetlb allocation can drop
+ * the reference.  For non-'BIND referenced policies, we can/do drop the
+ * reference here, so the caller doesn't need to know about the special case
+ * for default and current task policy.
+ */
 struct zonelist *huge_zonelist(struct vm_area_struct *vma, unsigned long addr,
-							gfp_t gfp_flags)
+				gfp_t gfp_flags, struct mempolicy **mpol)

mm/mempolicy.c +31 -4

 		return NODE_DATA(nid)->node_zonelists + gfp_zone(gfp_flags);
 	}
-	return zonelist_policy(GFP_HIGHUSER, pol);
+
+	zl = zonelist_policy(GFP_HIGHUSER, pol);
+	if (unlikely(pol != &default_policy && pol != current->mempolicy)) {
+		if (pol->policy != MPOL_BIND)
+			__mpol_free(pol);	/* finished with pol */
+		else
+			*mpol = pol;	/* unref needed after allocation */
+	}
+	return zl;
 }
 #endif
 

@@ -1270,6 +1312,7 @@ struct page *
 alloc_page_vma(gfp_t gfp, struct vm_area_struct *vma, unsigned long addr)
 {
 	struct mempolicy *pol = get_vma_policy(current, vma, addr);
+	struct zonelist *zl;
 
 	cpuset_update_task_memory_state();
 

@@ -1279,7 +1322,19 @@ alloc_page_vma(gfp_t gfp, struct vm_area
 		nid = interleave_nid(pol, vma, addr, PAGE_SHIFT);
 		return alloc_page_interleave(gfp, 0, nid);
 	}
-	return __alloc_pages(gfp, 0, zonelist_policy(gfp, pol));
+	zl = zonelist_policy(gfp, pol);
+	if (pol != &default_policy && pol != current->mempolicy) {
+		/*
+		 * slow path: ref counted policy -- shared or vma
+		 */
+		struct page *page =  __alloc_pages(gfp, 0, zl);
+		__mpol_free(pol);
+		return page;
+	}
+	/*
+	 * fast path:  default or task policy
+	 */
+	return __alloc_pages(gfp, 0, zl);
 }
 
 /**

@@ -1878,6 +1933,7 @@ int show_numa_map(struct seq_file *m, vo
 	struct numa_maps *md;
 	struct file *file = vma->vm_file;
 	struct mm_struct *mm = vma->vm_mm;
+	struct mempolicy *pol;
 	int n;
 	char buffer[50];
 

@@ -1888,8 +1944,13 @@ int show_numa_map(struct seq_file *m, vo
 	if (!md)
 		return 0;
 
-	mpol_to_str(buffer, sizeof(buffer),
-			    get_vma_policy(priv->task, vma, vma->vm_start));
+	pol = get_vma_policy(priv->task, vma, vma->vm_start);
+	mpol_to_str(buffer, sizeof(buffer), pol);
+	/*
+	 * unref shared or other task's mempolicy
+	 */
+	if (pol != &default_policy && pol != current->mempolicy)
+		__mpol_free(pol);
 
 	seq_printf(m, "%08lx %s", vma->vm_start, buffer);
 

Index: Linux/include/linux/mempolicy.h
===================================================================

include/linux/mempolicy.h +2 -2

@@ -150,7 +150,7 @@ extern void mpol_fix_fork_child_flag(str
 
 extern struct mempolicy default_policy;
 extern struct zonelist *huge_zonelist(struct vm_area_struct *vma,
-		unsigned long addr, gfp_t gfp_flags);
+		unsigned long addr, gfp_t gfp_flags, struct mempolicy **mpol);
 extern unsigned slab_node(struct mempolicy *policy);
 
 extern enum zone_type policy_zone;

@@ -238,7 +238,7 @@ static inline void mpol_fix_fork_child_f
 }
 
 static inline struct zonelist *huge_zonelist(struct vm_area_struct *vma,
-		unsigned long addr, gfp_t gfp_flags)
+		unsigned long addr, gfp_t gfp_flags, struct mempolicy **mpol)
 {
 	return NODE_DATA(0)->node_zonelists + gfp_zone(gfp_flags);
 }

Index: Linux/mm/hugetlb.c
===================================================================

mm/hugetlb.c +2 -1

@@ -71,8 +71,9 @@ static struct page *dequeue_huge_page(st
 {
 	int nid;
 	struct page *page = NULL;
+	struct mempolicy *mpol;
 	struct zonelist *zonelist = huge_zonelist(vma, address,
-						htlb_alloc_mask);
+					htlb_alloc_mask, &mpol);
 	struct zone **z;
 
 	for (z = zonelist->zones; *z; z++) {

You know this stuff better than I do. Take suggestions here with a large grain of salt.

Quoted 24 lines

Index: Linux/mm/mempolicy.c
===================================================================

mm/mempolicy.c +13 -1

@@ -1083,21 +1083,37 @@ asmlinkage long compat_sys_mbind(compat_
 
 #endif
 
-/* Return effective policy for a VMA */
+/*
+ * get_vma_policy(@task, @vma, @addr)
+ * @task - task for fallback if vma policy == default
+ * @vma   - virtual memory area whose policy is sought
+ * @addr  - address in @vma for shared policy lookup
+ *
+ * Returns effective policy for a VMA at specified address.
+ * Falls back to @task or system default policy, as necessary.
+ * Returned policy has extra reference count if shared, vma,
+ * or some other task's policy [show_numa_maps() can pass
+ * @task != current].  It is the caller's responsibility to
+ * free the reference in these cases.
+ */
 static struct mempolicy * get_vma_policy(struct task_struct *task,
 		struct vm_area_struct *vma, unsigned long addr)

mm/mempolicy.c asmlinkage long compat_sys_mbind(compat_ · line 1108

Quoted 10 lines

mm/mempolicy.c +3 -2

 {
 	struct mempolicy *pol = task->mempolicy;
+	int shared_pol = 0;
 
 	if (vma) {
-		if (vma->vm_ops && vma->vm_ops->get_policy)
+		if (vma->vm_ops && vma->vm_ops->get_policy) {
 			pol = vma->vm_ops->get_policy(vma, addr);
-		else if (vma->vm_policy &&
+			shared_pol = 1;	/* if pol non-NULL, that is */

What do you mean here by "pol non-NULL, that is". Where do you check that vm_ops->get_policy() returned a non-NULL value?

Should the comment be

/* Policy if set is shared, check later */

and rename the variable to check_shared_pol?

Quoted 15 lines

mm/mempolicy.c +3 -0

+		} else if (vma->vm_policy &&
 				vma->vm_policy->policy != MPOL_DEFAULT)
 			pol = vma->vm_policy;
 	}
 	if (!pol)
 		pol = &default_policy;
+	else if (!shared_pol && pol != current->mempolicy)
+		mpol_get(pol);	/* vma or other task's policy */
 	return pol;
 }
 

@@ -1213,19 +1229,45 @@ static inline unsigned interleave_nid(st
 }
 

mm/mempolicy.c static inline unsigned interleave_nid(st · line 1240

Quoted 11 lines

mm/mempolicy.c +9 -1

 #ifdef CONFIG_HUGETLBFS
-/* Return a zonelist suitable for a huge page allocation. */
+/*
+ * huge_zonelist(@vma, @addr, @gfp_flags, @mpol)
+ * @vma = virtual memory area whose policy is sought
+ * @addr = address in @vma for shared policy lookup and interleave policy
+ * @gfp_flags = for requested zone
+ * @mpol = pointer to mempolicy pointer for reference counted 'BIND policy
+ *
+ * Returns a zonelist suitable for a huge page allocation.
+ * If the effective policy is 'BIND, returns pointer to policy's zonelist.

This comment here becomes redundant if applied on top of one-zonelist as you suggest you will be doing later. The zonelist returned for MPOL_BIND is the nodes zonelist but it is filtered based on a nodemask.

Quoted 19 lines

mm/mempolicy.c +10 -1

+ * If it is also a policy for which get_vma_policy() returns an extra
+ * reference, we must hold that reference until after allocation.
+ * In that case, return policy via @mpol so hugetlb allocation can drop
+ * the reference.  For non-'BIND referenced policies, we can/do drop the
+ * reference here, so the caller doesn't need to know about the special case
+ * for default and current task policy.
+ */
 struct zonelist *huge_zonelist(struct vm_area_struct *vma, unsigned long addr,
-							gfp_t gfp_flags)
+				gfp_t gfp_flags, struct mempolicy **mpol)
 {
 	struct mempolicy *pol = get_vma_policy(current, vma, addr);
+	struct zonelist *zl;
 
+	*mpol = NULL;		/* probably no unref needed */
 	if (pol->policy == MPOL_INTERLEAVE) {
 		unsigned nid;
 

mm/mempolicy.c static inline unsigned interleave_nid(st · line 1259

Quoted 2 lines

mm/mempolicy.c +1 -0

 		nid = interleave_nid(pol, vma, addr, HPAGE_SHIFT);
+		__mpol_free(pol);

So, __mpol_free() here acts as a put on the get_vma_policy() right? Either that needs commenting or __mpol_free() needs to be renamed to __mpol_put() assuming that when the count reaches 0, it really gets free.

Quoted 2 lines

You know this stuff better than I do. Take suggestions here with a large grain of salt.

Your comments are on the mark. See responses below.

Quoted 2 lines · Lee Schermerhorn

<patch description snipped>

Quoted 38 lines

Quoted 24 lines

Index: Linux/mm/mempolicy.c
===================================================================

mm/mempolicy.c +13 -1

@@ -1083,21 +1083,37 @@ asmlinkage long compat_sys_mbind(compat_
 
 #endif
 
-/* Return effective policy for a VMA */
+/*
+ * get_vma_policy(@task, @vma, @addr)
+ * @task - task for fallback if vma policy == default
+ * @vma   - virtual memory area whose policy is sought
+ * @addr  - address in @vma for shared policy lookup
+ *
+ * Returns effective policy for a VMA at specified address.
+ * Falls back to @task or system default policy, as necessary.
+ * Returned policy has extra reference count if shared, vma,
+ * or some other task's policy [show_numa_maps() can pass
+ * @task != current].  It is the caller's responsibility to
+ * free the reference in these cases.
+ */
 static struct mempolicy * get_vma_policy(struct task_struct *task,
 		struct vm_area_struct *vma, unsigned long addr)

mm/mempolicy.c asmlinkage long compat_sys_mbind(compat_ · line 1108

Quoted 10 lines

mm/mempolicy.c +3 -2

 {
 	struct mempolicy *pol = task->mempolicy;
+	int shared_pol = 0;
 
 	if (vma) {
-		if (vma->vm_ops && vma->vm_ops->get_policy)
+		if (vma->vm_ops && vma->vm_ops->get_policy) {
 			pol = vma->vm_ops->get_policy(vma, addr);
-		else if (vma->vm_policy &&
+			shared_pol = 1;	/* if pol non-NULL, that is */

What do you mean here by "pol non-NULL, that is". Where do you check that vm_ops->get_policy() returned a non-NULL value?

Quoted 5 lines

Should the comment be

/* Policy if set is shared, check later */

and rename the variable to check_shared_pol?

You interpret my cryptic comment correctly. However, your suggested fix doesn't quite capture my way of looking at it. Would it work for you if I change it to: /* if pol non-NULL, add ref below */ ??? That fits in 80 columns ;-)!

Quoted 15 lines

Quoted 15 lines

mm/mempolicy.c +3 -0

+		} else if (vma->vm_policy &&
 				vma->vm_policy->policy != MPOL_DEFAULT)
 			pol = vma->vm_policy;
 	}
 	if (!pol)
 		pol = &default_policy;
+	else if (!shared_pol && pol != current->mempolicy)
+		mpol_get(pol);	/* vma or other task's policy */
 	return pol;
 }
 

@@ -1213,19 +1229,45 @@ static inline unsigned interleave_nid(st
 }
 

Quoted 15 lines

mm/mempolicy.c static inline unsigned interleave_nid(st · line 1240

Quoted 11 lines

mm/mempolicy.c +9 -1

 #ifdef CONFIG_HUGETLBFS
-/* Return a zonelist suitable for a huge page allocation. */
+/*
+ * huge_zonelist(@vma, @addr, @gfp_flags, @mpol)
+ * @vma = virtual memory area whose policy is sought
+ * @addr = address in @vma for shared policy lookup and interleave policy
+ * @gfp_flags = for requested zone
+ * @mpol = pointer to mempolicy pointer for reference counted 'BIND policy
+ *
+ * Returns a zonelist suitable for a huge page allocation.
+ * If the effective policy is 'BIND, returns pointer to policy's zonelist.

This comment here becomes redundant if applied on top of one-zonelist as you suggest you will be doing later. The zonelist returned for MPOL_BIND is the nodes zonelist but it is filtered based on a nodemask.

Agreed. When I get around to rebasing atop your patches [under the assumption they'll hit the mm tree before these] I'll fix this up. For now, I've added myself a 'TODO' comment.

Note, however, that unless we take a copy of the policy's nodemask, we'll still need to hold the reference over the allocation, I think. Haven't looked that closely, yet.

Quoted 19 lines

Quoted 19 lines

mm/mempolicy.c +10 -1

+ * If it is also a policy for which get_vma_policy() returns an extra
+ * reference, we must hold that reference until after allocation.
+ * In that case, return policy via @mpol so hugetlb allocation can drop
+ * the reference.  For non-'BIND referenced policies, we can/do drop the
+ * reference here, so the caller doesn't need to know about the special case
+ * for default and current task policy.
+ */
 struct zonelist *huge_zonelist(struct vm_area_struct *vma, unsigned long addr,
-							gfp_t gfp_flags)
+				gfp_t gfp_flags, struct mempolicy **mpol)
 {
 	struct mempolicy *pol = get_vma_policy(current, vma, addr);
+	struct zonelist *zl;
 
+	*mpol = NULL;		/* probably no unref needed */
 	if (pol->policy == MPOL_INTERLEAVE) {
 		unsigned nid;
 

Quoted 7 lines

mm/mempolicy.c static inline unsigned interleave_nid(st · line 1259

Quoted 2 lines

mm/mempolicy.c +1 -0

 		nid = interleave_nid(pol, vma, addr, HPAGE_SHIFT);
+		__mpol_free(pol);

So, __mpol_free() here acts as a put on the get_vma_policy() right? Either that needs commenting or __mpol_free() needs to be renamed to __mpol_put() assuming that when the count reaches 0, it really gets free.

Yes, the '__' version of mpol_free() takes a non-NULL policy pointer and decrements the reference. [w/o the '__', a NULL policy pointer is a no-op.] If the resulting count is zero, the policy structure, and any attached zonelist [or nodemask, if we make those remote, as discussed in
Cambridge] is freed. The 'free notation is Andi's original naming.
For now, rather than change that throughout the code, I'll comment this instance.

PATCH/RFC 2/5 Use MPOL_PREFERRED for system-wide default policy

Against: 2.6.23-rc3-mm1

V1 -> V2:
+ restore BUG()s in switch(policy) default cases -- per Christoph
+ eliminate unneeded re-init of struct mempolicy policy member before freeing

Currently, when one specifies MPOL_DEFAULT via a NUMA memory policy API [set_mempolicy(), mbind() and internal versions], the kernel simply installs a NULL struct mempolicy pointer in the appropriate context: task policy, vma policy, or shared policy. This causes any use of that policy to "fall back" to the next most specific policy scope. The only use of MPOL_DEFAULT to mean "local allocation" is in the system default policy.

There is another, "preferred" way to specify local allocation via the APIs. That is using the MPOL_PREFERRED policy mode with an empty nodemask. Internally, the empty nodemask gets converted to a preferred_node id of '-1'. All internal usage of MPOL_PREFERRED will convert the '-1' to the id of the node local to the cpu where the allocation occurs.

System default policy, except during boot, is hard-coded to "local allocation". By using the MPOL_PREFERRED mode with a negative value of preferred node for system default policy, MPOL_DEFAULT will never occur in the 'policy' member of a struct mempolicy. Thus, we can remove all checks for MPOL_DEFAULT when converting policy to a node id/zonelist in the allocation paths.

In slab_node() return local node id when policy pointer is NULL. No need to set a pol value to take the switch default. Replace switch default with BUG()--i.e., shouldn't happen.

With this patch MPOL_DEFAULT is only used in the APIs, including internal calls to do_set_mempolicy() and in the display of policy in /proc/<pid>/numa_maps. It always means "fall back" to the the next most specific policy scope. This simplifies the description of memory policies quite a bit, with no visible change in behavior. This patch updates Documentation to reflect this change.

Tested with set_mempolicy() using numactl with memtoy, and tested mbind() with memtoy. All seems to work "as expected".

Signed-off-by Lee Schermerhorn <[email protected]>

 Documentation/vm/numa_memory_policy.txt |   70 ++++++++++++--------------------
 mm/mempolicy.c                          |   31 ++++++--------
 2 files changed, 41 insertions(+), 60 deletions(-)

Index: Linux/mm/mempolicy.c
===================================================================

mm/mempolicy.c +0 -0

@@ -105,9 +105,13 @@ static struct kmem_cache *sn_cache;
    policied. */
 enum zone_type policy_zone = 0;
 

mm/mempolicy.c +7 -15

 	policy = kmem_cache_alloc(policy_cache, GFP_KERNEL);
 	if (!policy)
 		return ERR_PTR(-ENOMEM);

@@ -493,8 +498,6 @@ static void get_zonemask(struct mempolic
 			node_set(zone_to_nid(p->v.zonelist->zones[i]),
 				*nodes);
 		break;
-	case MPOL_DEFAULT:
-		break;
 	case MPOL_INTERLEAVE:
 		*nodes = p->v.nodes;
 		break;

@@ -1106,8 +1109,7 @@ static struct mempolicy * get_vma_policy
 		if (vma->vm_ops && vma->vm_ops->get_policy) {
 			pol = vma->vm_ops->get_policy(vma, addr);
 			shared_pol = 1;	/* if pol non-NULL, that is */
-		} else if (vma->vm_policy &&
-				vma->vm_policy->policy != MPOL_DEFAULT)
+		} else if (vma->vm_policy)
 			pol = vma->vm_policy;
 	}
 	if (!pol)

@@ -1136,7 +1138,6 @@ static struct zonelist *zonelist_policy(
 				return policy->v.zonelist;
 		/*FALL THROUGH*/
 	case MPOL_INTERLEAVE: /* should not happen */
-	case MPOL_DEFAULT:
 		nd = numa_node_id();
 		break;
 	default:

@@ -1166,9 +1167,10 @@ static unsigned interleave_nodes(struct 
  */
 unsigned slab_node(struct mempolicy *policy)
 {
-	int pol = policy ? policy->policy : MPOL_DEFAULT;
+	if (!policy)
+		return numa_node_id();
 
-	switch (pol) {
+	switch (policy->policy) {
 	case MPOL_INTERLEAVE:
 		return interleave_nodes(policy);
 

@@ -1182,10 +1184,10 @@ unsigned slab_node(struct mempolicy *pol
 	case MPOL_PREFERRED:
 		if (policy->v.preferred_node >= 0)
 			return policy->v.preferred_node;
-		/* Fall through */
+		return numa_node_id();
 
 	default:
-		return numa_node_id();
+		BUG();
 	}
 }
 

@@ -1410,8 +1412,6 @@ int __mpol_equal(struct mempolicy *a, st
 	if (a->policy != b->policy)
 		return 0;
 	switch (a->policy) {
-	case MPOL_DEFAULT:
-		return 1;
 	case MPOL_INTERLEAVE:
 		return nodes_equal(a->v.nodes, b->v.nodes);
 	case MPOL_PREFERRED:

@@ -1436,7 +1436,6 @@ void __mpol_free(struct mempolicy *p)
 		return;
 	if (p->policy == MPOL_BIND)
 		kfree(p->v.zonelist);
-	p->policy = MPOL_DEFAULT;
 	kmem_cache_free(policy_cache, p);
 }
 

@@ -1603,7 +1602,7 @@ void mpol_shared_policy_init(struct shar
 	if (policy != MPOL_DEFAULT) {
 		struct mempolicy *newpol;
 
-		/* Falls back to MPOL_DEFAULT on any error */
+		/* Falls back to NULL policy [MPOL_DEFAULT] on any error */
 		newpol = mpol_new(policy, policy_nodes);
 		if (!IS_ERR(newpol)) {
 			/* Create pseudo-vma that contains just the policy */

@@ -1724,8 +1723,6 @@ static void mpol_rebind_policy(struct me
 		return;
 
 	switch (pol->policy) {
-	case MPOL_DEFAULT:
-		break;
 	case MPOL_INTERLEAVE:
 		nodes_remap(tmp, pol->v.nodes, *mpolmask, *newmask);
 		pol->v.nodes = tmp;

Index: Linux/Documentation/vm/numa_memory_policy.txt
===================================================================

Documentation/vm/numa_memory_policy.txt +27 -44

@@ -149,63 +149,47 @@ Components of Memory Policies
 
    Linux memory policy supports the following 4 behavioral modes:
 
-	Default Mode--MPOL_DEFAULT:  The behavior specified by this mode is
-	context or scope dependent.
+	Default Mode--MPOL_DEFAULT:  This mode is only used in the memory
+	policy APIs.  Internally, MPOL_DEFAULT is converted to the NULL
+	memory policy in all policy scopes.  Any existing non-default policy
+	will simply be removed when MPOL_DEFAULT is specified.  As a result,
+	MPOL_DEFAULT means "fall back to the next most specific policy scope."
+
+	    For example, a NULL or default task policy will fall back to the
+	    system default policy.  A NULL or default vma policy will fall
+	    back to the task policy.
 
-	    As mentioned in the Policy Scope section above, during normal
-	    system operation, the System Default Policy is hard coded to
-	    contain the Default mode.
-
-	    In this context, default mode means "local" allocation--that is
-	    attempt to allocate the page from the node associated with the cpu
-	    where the fault occurs.  If the "local" node has no memory, or the
-	    node's memory can be exhausted [no free pages available], local
-	    allocation will "fallback to"--attempt to allocate pages from--
-	    "nearby" nodes, in order of increasing "distance".
-
-		Implementation detail -- subject to change:  "Fallback" uses
-		a per node list of sibling nodes--called zonelists--built at
-		boot time, or when nodes or memory are added or removed from
-		the system [memory hotplug].  These per node zonelist are
-		constructed with nodes in order of increasing distance based
-		on information provided by the platform firmware.
-
-	    When a task/process policy or a shared policy contains the Default
-	    mode, this also means "local allocation", as described above.
-
-	    In the context of a VMA, Default mode means "fall back to task
-	    policy"--which may or may not specify Default mode.  Thus, Default
-	    mode can not be counted on to mean local allocation when used
-	    on a non-shared region of the address space.  However, see
-	    MPOL_PREFERRED below.
-
-	    The Default mode does not use the optional set of nodes.
+	    When specified in one of the memory policy APIs, the Default mode
+	    does not use the optional set of nodes.
 
 	MPOL_BIND:  This mode specifies that memory must come from the
 	set of nodes specified by the policy.
 
 	    The memory policy APIs do not specify an order in which the nodes
-	    will be searched.  However, unlike "local allocation", the Bind
-	    policy does not consider the distance between the nodes.  Rather,
-	    allocations will fallback to the nodes specified by the policy in
-	    order of numeric node id.  Like everything in Linux, this is subject
-	    to change.
+	    will be searched.  However, unlike "local allocation" discussed
+	    below, the Bind policy does not consider the distance between the
+	    nodes.  Rather, allocations will fallback to the nodes specified
+	    by the policy in order of numeric node id.  Like everything in
+	    Linux, this is subject to change.
 
 	MPOL_PREFERRED:  This mode specifies that the allocation should be
 	attempted from the single node specified in the policy.  If that
-	allocation fails, the kernel will search other nodes, exactly as
-	it would for a local allocation that started at the preferred node
-	in increasing distance from the preferred node.  "Local" allocation
-	policy can be viewed as a Preferred policy that starts at the node
-	containing the cpu where the allocation takes place.
+	allocation fails, the kernel will search other nodes, in order of
+	increasing distance from the preferred node based on information
+	provided by the platform firmware.
 
 	    Internally, the Preferred policy uses a single node--the
 	    preferred_node member of struct mempolicy.  A "distinguished
 	    value of this preferred_node, currently '-1', is interpreted
 	    as "the node containing the cpu where the allocation takes
-	    place"--local allocation.  This is the way to specify
-	    local allocation for a specific range of addresses--i.e. for
-	    VMA policies.
+	    place"--local allocation.  "Local" allocation policy can be
+	    viewed as a Preferred policy that starts at the node containing
+	    the cpu where the allocation takes place.
+
+	    As mentioned in the Policy Scope section above, during normal
+	    system operation, the System Default Policy is hard coded to
+	    specify "local allocation".  This policy uses the Preferred
+	    policy with the special negative value of preferred_node.
 
 	MPOL_INTERLEAVED:  This mode specifies that page allocations be
 	interleaved, on a page granularity, across the nodes specified in
 
--

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

Quoted 10 lines · Lee Schermerhorn

PATCH/RFC 2/5 Use MPOL_PREFERRED for system-wide default policy

Against: 2.6.23-rc3-mm1

V1 -> V2:
+ restore BUG()s in switch(policy) default cases -- per Christoph
+ eliminate unneeded re-init of struct mempolicy policy member before freeing

Quoted 8 lines

Currently, when one specifies MPOL_DEFAULT via a NUMA memory policy API [set_mempolicy(), mbind() and internal versions], the kernel simply installs a NULL struct mempolicy pointer in the appropriate context: task policy, vma policy, or shared policy. This causes any use of that policy to "fall back" to the next most specific policy scope. The only use of MPOL_DEFAULT to mean "local allocation" is in the system default policy.

In general, this seems like a good idea. It's certainly simplier to always assume a policy exists because it discourages "bah, I don't care about policies" style of thinking.

Quoted 9 lines

Index: Linux/mm/mempolicy.c
===================================================================

mm/mempolicy.c +0 -0

@@ -105,9 +105,13 @@ static struct kmem_cache *sn_cache;
    policied. */
 enum zone_type policy_zone = 0;
 

mm/mempolicy.c static struct kmem_cache *sn_cache; · line 116

Quoted 10 lines

mm/mempolicy.c +5 -1

+/*
+ * run-time system-wide default policy => local allocation
+ */
 struct mempolicy default_policy = {
 	.refcnt = ATOMIC_INIT(1), /* never free it */
-	.policy = MPOL_DEFAULT,
+	.policy = MPOL_PREFERRED,
+	.v =  { .preferred_node =  -1 },
 };
 

fairly clear.

Quoted 4 lines

mm/mempolicy.c +0 -0

 static void mpol_rebind_policy(struct mempolicy *pol,

@@ -180,7 +184,8 @@ static struct mempolicy *mpol_new(int mo
 		 mode, nodes ? nodes_addr(*nodes)[0] : -1);
 

mm/mempolicy.c static struct mempolicy *mpol_new(int mo · line 188

Quoted 4 lines

mm/mempolicy.c +2 -1

 	if (mode == MPOL_DEFAULT)
-		return NULL;
+		return NULL;	/* simply delete any existing policy */
+

Why do we not return default_policy and insert that into the VMA or whatever?

Quoted 20 lines · Lee Schermerhorn

Quoted 18 lines

PATCH/RFC 2/5 Use MPOL_PREFERRED for system-wide default policy

Against: 2.6.23-rc3-mm1

V1 -> V2:
+ restore BUG()s in switch(policy) default cases -- per Christoph
+ eliminate unneeded re-init of struct mempolicy policy member before freeing

Currently, when one specifies MPOL_DEFAULT via a NUMA memory policy API [set_mempolicy(), mbind() and internal versions], the kernel simply installs a NULL struct mempolicy pointer in the appropriate context: task policy, vma policy, or shared policy. This causes any use of that policy to "fall back" to the next most specific policy scope. The only use of MPOL_DEFAULT to mean "local allocation" is in the system default policy.

Quoted 3 lines

In general, this seems like a good idea. It's certainly simplier to always assume a policy exists because it discourages "bah, I don't care about policies" style of thinking.

More importantly, IMO, it eliminates 2 meanings for MPOL_DEFAULT in different contexts and promotes the use 0f [MPOL_PREFERRED, -1/null-nodemask] for local allocation. I think this makes the resulting documentation clearer.

<snip>

Quoted 26 lines

Quoted 9 lines

Index: Linux/mm/mempolicy.c
===================================================================

mm/mempolicy.c +0 -0

@@ -105,9 +105,13 @@ static struct kmem_cache *sn_cache;
    policied. */
 enum zone_type policy_zone = 0;
 

mm/mempolicy.c static struct kmem_cache *sn_cache; · line 116

Quoted 10 lines

mm/mempolicy.c +5 -1

+/*
+ * run-time system-wide default policy => local allocation
+ */
 struct mempolicy default_policy = {
 	.refcnt = ATOMIC_INIT(1), /* never free it */
-	.policy = MPOL_DEFAULT,
+	.policy = MPOL_PREFERRED,
+	.v =  { .preferred_node =  -1 },
 };
 

fairly clear.

Quoted 4 lines

mm/mempolicy.c +0 -0

 static void mpol_rebind_policy(struct mempolicy *pol,

@@ -180,7 +184,8 @@ static struct mempolicy *mpol_new(int mo
 		 mode, nodes ? nodes_addr(*nodes)[0] : -1);
 

Quoted 8 lines

mm/mempolicy.c static struct mempolicy *mpol_new(int mo · line 188

Quoted 4 lines

mm/mempolicy.c +2 -1

 	if (mode == MPOL_DEFAULT)
-		return NULL;
+		return NULL;	/* simply delete any existing policy */
+

Why do we not return default_policy and insert that into the VMA or whatever?

Because then, if we're installing a shared policy [shmem], we'll go ahead and create an rb-node and insert the [default] policy in the tree in the shared policy struct, instead of just deleting any policy ranges that the new policy covers. Andi already implemented the code to delete shared policy ranges covered by a subsequent null/default policy. I like this approach.

I have additional patches, to come later, that dynamically allocate the shared policy structure when a non-null [non-default] policy is installed. At some point, I plan on enhancing this to to use a single policy pointer, instead of the shared policy struct, when the policy covers the entire object range, and delete any existing shared policy struct when/if a default policy covers the entire range.

PATCH/RFC 03/05 - MPOL_PREFERRED cleanups for "local allocation" - V4

Against: 2.6.23-rc3-mm1

V3 -> V4:
+ updated Documentation/vm/numa_memory_policy.txt to better explain [I think] the "local allocation" feature of MPOL_PREFERRED.

V2 -> V3:
+ renamed get_nodemask() to get_policy_nodemask() to more closely match what it's doing.

V1 -> V2:
+ renamed get_zonemask() to get_nodemask(). Mel Gorman suggested this was a valid "cleanup".

Here are a couple of "cleanups" for MPOL_PREFERRED behavior when v.preferred_node < 0 -- i.e., "local allocation":

1) [do_]get_mempolicy() calls the now renamed get_policy_nodemask() to fetch the nodemask associated with a policy. Currently, get_policy_nodemask() returns the set of nodes with memory, when the policy 'mode' is 'PREFERRED, and the preferred_node is < 0. Return the set of allowed nodes instead. This will already have been masked to include only nodes with memory.

2) When a task is moved into a [new] cpuset, mpol_rebind_policy() is called to adjust any task and vma policy nodes to be valid in the new cpuset. However, when the policy is MPOL_PREFERRED, and the preferred_node is <0, no rebind is necessary. The "local allocation" indication is valid in any cpuset. Existing code will "do the right thing" because node_remap() will just return the argument node when it is outside of the valid range of node ids. However, I think it is clearer and cleaner to skip the remap explicitly in this case.

3) mpol_to_str() produces a printable, "human readable" string from a struct mempolicy. For MPOL_PREFERRED with preferred_node <0, show the entire set of valid nodes. Although, technically, MPOL_PREFERRED takes only a single node, preferred_node <0 is a local allocation policy, with the preferred node determined by the context where the task is executing. All of the allowed nodes are possible, as the task migrates amoung the nodes in the cpuset. Without this change, I believe that node_set() [via set_bit()] will set bit 31, resulting in a misleading display.

Signed-off-by Lee Schermerhorn <[email protected]>

 mm/mempolicy.c |   41 ++++++++++++++++++++++++++++++-----------
 1 file changed, 30 insertions(+), 11 deletions(-)

Index: Linux/mm/mempolicy.c
===================================================================

mm/mempolicy.c +4 -2

@@ -486,8 +486,10 @@ static long do_set_mempolicy(int mode, n
 	return 0;
 }
 
-/* Fill a zone bitmap for a policy */
-static void get_zonemask(struct mempolicy *p, nodemask_t *nodes)
+/*
+ * Return a node bitmap for a policy
+ */
+static void get_policy_nodemask(struct mempolicy *p, nodemask_t *nodes)
 {
 	int i;
 

mm/mempolicy.c +22 -8

 		else
 			node_set(p->v.preferred_node, *nodes);
 		break;

@@ -578,7 +582,7 @@ static long do_get_mempolicy(int *policy
 
 	err = 0;
 	if (nmask)
-		get_zonemask(pol, nmask);
+		get_policy_nodemask(pol, nmask);
 
  out:
 	if (vma)

@@ -1715,6 +1719,7 @@ static void mpol_rebind_policy(struct me
 {
 	nodemask_t *mpolmask;
 	nodemask_t tmp;
+	int nid;
 
 	if (!pol)
 		return;

@@ -1731,9 +1736,15 @@ static void mpol_rebind_policy(struct me
 						*mpolmask, *newmask);
 		break;
 	case MPOL_PREFERRED:
-		pol->v.preferred_node = node_remap(pol->v.preferred_node,
+		/*
+		 * no need to remap "local policy"
+		 */
+		nid = pol->v.preferred_node;
+		if (nid >= 0) {
+			pol->v.preferred_node = node_remap(nid,
 						*mpolmask, *newmask);
-		*mpolmask = *newmask;
+			*mpolmask = *newmask;
+		}
 		break;
 	case MPOL_BIND: {
 		nodemask_t nodes;

@@ -1808,7 +1819,7 @@ static const char * const policy_types[]
 static inline int mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
 {
 	char *p = buffer;
-	int l;
+	int nid, l;
 	nodemask_t nodes;
 	int mode = pol ? pol->policy : MPOL_DEFAULT;
 

@@ -1818,12 +1829,20 @@ static inline int mpol_to_str(char *buff
 		break;
 
 	case MPOL_PREFERRED:
-		nodes_clear(nodes);
-		node_set(pol->v.preferred_node, nodes);
+		nid = pol->v.preferred_node;
+		/*
+		 * local interleave, show all valid nodes
+		 */
+		if (nid < 0)
+			nodes = cpuset_current_mems_allowed;
+		else {
+			nodes_clear(nodes);
+			node_set(nid, nodes);
+		}
 		break;
 
 	case MPOL_BIND:
-		get_zonemask(pol, &nodes);
+		get_policy_nodemask(pol, &nodes);
 		break;
 
 	case MPOL_INTERLEAVE:
 
--

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

Quoted 19 lines · Lee Schermerhorn

PATCH/RFC 03/05 - MPOL_PREFERRED cleanups for "local allocation" - V4

Against: 2.6.23-rc3-mm1

V3 -> V4:
+ updated Documentation/vm/numa_memory_policy.txt to better explain [I think] the "local allocation" feature of MPOL_PREFERRED.

V2 -> V3:
+ renamed get_nodemask() to get_policy_nodemask() to more closely match what it's doing.

V1 -> V2:
+ renamed get_zonemask() to get_nodemask(). Mel Gorman suggested this was a valid "cleanup".

Here are a couple of "cleanups" for MPOL_PREFERRED behavior when v.preferred_node < 0 -- i.e., "local allocation":

Quoted 7 lines

1) [do_]get_mempolicy() calls the now renamed get_policy_nodemask() to fetch the nodemask associated with a policy. Currently, get_policy_nodemask() returns the set of nodes with memory, when the policy 'mode' is 'PREFERRED, and the preferred_node is < 0. Return the set of allowed nodes instead. This will already have been masked to include only nodes with memory.

Better name all right.

Quoted 17 lines

Index: Linux/mm/mempolicy.c
===================================================================

mm/mempolicy.c +4 -2

@@ -486,8 +486,10 @@ static long do_set_mempolicy(int mode, n
 	return 0;
 }
 
-/* Fill a zone bitmap for a policy */
-static void get_zonemask(struct mempolicy *p, nodemask_t *nodes)
+/*
+ * Return a node bitmap for a policy
+ */
+static void get_policy_nodemask(struct mempolicy *p, nodemask_t *nodes)
 {
 	int i;
 

mm/mempolicy.c static void get_zonemask(struct mempolic · line 511

Quoted 11 lines

mm/mempolicy.c +4 -2

@@ -502,9 +504,11 @@ static void get_zonemask(struct mempolic
 		*nodes = p->v.nodes;
 		break;
 	case MPOL_PREFERRED:
-		/* or use current node instead of memory_map? */
+		/*
+		 * for "local policy", return allowed memories
+		 */
 		if (p->v.preferred_node < 0)
-			*nodes = node_states[N_HIGH_MEMORY];
+			*nodes = cpuset_current_mems_allowed;

Is this change intentional? It looks like something that belongs as part of the the memoryless patch set.

Quoted 12 lines

mm/mempolicy.c static void get_zonemask(struct mempolic · line 511

Quoted 8 lines

mm/mempolicy.c +4 -2

 	case MPOL_PREFERRED:
-		/* or use current node instead of memory_map? */
+		/*
+		 * for "local policy", return allowed memories
+		 */
 		if (p->v.preferred_node < 0)
-			*nodes = node_states[N_HIGH_MEMORY];
+			*nodes = cpuset_current_mems_allowed;

Is this change intentional? It looks like something that belongs as part of the the memoryless patch set.

Absolutely intentional. The use of 'node_states[N_HIGH_MEMORY]' was added by the memoryless nodes patches. Formerly, this was 'node_online_map'. However, even this results in misleading info for tasks running in a cpuset.

It's a fine, point, but I think this is "more correct" that the existing code. I'm hoping that this change, with a corresponding man page update will head off some head scratching and support calls down the road.

Quoted 13 lines · Lee Schermerhorn

Quoted 12 lines

mm/mempolicy.c static void get_zonemask(struct mempolic · line 511

Quoted 8 lines

mm/mempolicy.c +4 -2

 	case MPOL_PREFERRED:
-		/* or use current node instead of memory_map? */
+		/*
+		 * for "local policy", return allowed memories
+		 */
 		if (p->v.preferred_node < 0)
-			*nodes = node_states[N_HIGH_MEMORY];
+			*nodes = cpuset_current_mems_allowed;

Is this change intentional? It looks like something that belongs as part of the the memoryless patch set.

Quoted 4 lines

Absolutely intentional. The use of 'node_states[N_HIGH_MEMORY]' was added by the memoryless nodes patches. Formerly, this was 'node_online_map'. However, even this results in misleading info for tasks running in a cpuset.

Sort of. This just means that the policy does not restrict the valid nodes. The cpuset does. I think this is okay but we may be confusing users as to which mechanism performs the restriction.

Quoted 3 lines

It's a fine, point, but I think this is "more correct" that the existing code. I'm hoping that this change, with a corresponding man page update will head off some head scratching and support calls down the road.

How does this sync with the nodemasks used by other policies? So far we are using a sort of cpuset agnostic nodeset and limit it when it is applied.

I think the integration between cpuset and memory policies could use some work and this is certainly something valid to do. Is there any way to describe that and have output that clarifies that distinction and helps the user figure out what is going on?

Quoted 3 lines

How does this sync with the nodemasks used by other policies? So far we are using a sort of cpuset agnostic nodeset and limit it when it is applied.

Not exactly: set_mempolicy() calls "contextualize_policy()" that returns an error if the nodemask is not a subset of mems_allowed; and then calls mpol_check_policy() to further vet the syscall args.

Now, I see that sys_mbind() does just AND the nodemask with mems_allowed. So, it won't give an error.

Should these be the same? If so, which way: error or silently mask off dis-allowed nodes? The latter doesn't let the user know what's going on, but with my new MPOL_F_MEMS_ALLOWED flag, a user can query the allowed nodes. And, I can update the man pages to state exactly what happens. So, how about:

Quoted 4 lines

I think the integration between cpuset and memory policies could use some work and this is certainly something valid to do. Is there any way to describe that and have output that clarifies that distinction and helps the user figure out what is going on?

Man pages can/will be updated and the ability to query allowed nodes should provide the necessary info. Would this satisfy your concern?

Quoted 28 lines · Lee Schermerhorn

Quoted 26 lines

PATCH/RFC 03/05 - MPOL_PREFERRED cleanups for "local allocation" - V4

Against: 2.6.23-rc3-mm1

V3 -> V4:
+ updated Documentation/vm/numa_memory_policy.txt to better explain [I think] the "local allocation" feature of MPOL_PREFERRED.

V2 -> V3:
+ renamed get_nodemask() to get_policy_nodemask() to more closely match what it's doing.

V1 -> V2:
+ renamed get_zonemask() to get_nodemask(). Mel Gorman suggested this was a valid "cleanup".

Here are a couple of "cleanups" for MPOL_PREFERRED behavior when v.preferred_node < 0 -- i.e., "local allocation":

1) [do_]get_mempolicy() calls the now renamed get_policy_nodemask() to fetch the nodemask associated with a policy. Currently, get_policy_nodemask() returns the set of nodes with memory, when the policy 'mode' is 'PREFERRED, and the preferred_node is < 0. Return the set of allowed nodes instead. This will already have been masked to include only nodes with memory.

Quoted 1 line

Better name all right.

:-) That's why you suggested it, right?

<snip>

Quoted 17 lines

Quoted 17 lines

Index: Linux/mm/mempolicy.c
===================================================================

mm/mempolicy.c +4 -2

@@ -486,8 +486,10 @@ static long do_set_mempolicy(int mode, n
 	return 0;
 }
 
-/* Fill a zone bitmap for a policy */
-static void get_zonemask(struct mempolicy *p, nodemask_t *nodes)
+/*
+ * Return a node bitmap for a policy
+ */
+static void get_policy_nodemask(struct mempolicy *p, nodemask_t *nodes)
 {
 	int i;
 

Quoted 15 lines

mm/mempolicy.c static void get_zonemask(struct mempolic · line 511

Quoted 11 lines

mm/mempolicy.c +4 -2

@@ -502,9 +504,11 @@ static void get_zonemask(struct mempolic
 		*nodes = p->v.nodes;
 		break;
 	case MPOL_PREFERRED:
-		/* or use current node instead of memory_map? */
+		/*
+		 * for "local policy", return allowed memories
+		 */
 		if (p->v.preferred_node < 0)
-			*nodes = node_states[N_HIGH_MEMORY];
+			*nodes = cpuset_current_mems_allowed;

Is this change intentional? It looks like something that belongs as part of the the memoryless patch set.

Absolutely intentional. The use of 'node_states[N_HIGH_MEMORY]' was added by the memoryless nodes patches. Formerly, this was 'node_online_map'. However, even this results in misleading info for tasks running in a cpuset.

When a task queries its memory policy via get_mempolicy(2), and the policy is MPOL_PREFERRED with the '-1' policy node--i.e., local allocation--the memory can come from any node from which the task is
allowed to allocate. Initially it will try to allocate only from nodes
containing cpus on which the task is allowed to execute. But, the allocation could overflow onto some other node allowed in the cpuset.

It's a fine, point, but I think this is "more correct" that the existing code. I'm hoping that this change, with a corresponding man page update will head off some head scratching and support calls down the road.

1) [do_]get_mempolicy() calls the now renamed get_policy_nodemask() to fetch the nodemask associated with a policy. Currently, get_policy_nodemask() returns the set of nodes with memory, when the policy 'mode' is 'PREFERRED, and the preferred_node is < 0. Return the set of allowed nodes instead. This will already have been masked to include only nodes with memory.

2) When a task is moved into a [new] cpuset, mpol_rebind_policy() is called to adjust any task and vma policy nodes to be valid in the new cpuset. However, when the policy is MPOL_PREFERRED, and the preferred_node is <0, no rebind is necessary. The "local allocation" indication is valid in any cpuset. Existing code will "do the right thing" because node_remap() will just return the argument node when it is outside of the valid range of node ids. However, I think it is clearer and cleaner to skip the remap explicitly in this case.

3) mpol_to_str() produces a printable, "human readable" string from a struct mempolicy. For MPOL_PREFERRED with preferred_node <0, show the entire set of valid nodes. Although, technically, MPOL_PREFERRED takes only a single node, preferred_node <0 is a local allocation policy, with the preferred node determined by the context where the task is executing. All of the allowed nodes are possible, as the task migrates amoung the nodes in the cpuset. Without this change, I believe that node_set() [via set_bit()] will set bit 31, resulting in a misleading display.

Quoted 6 lines · Lee Schermerhorn

1) [do_]get_mempolicy() calls the now renamed get_policy_nodemask() to fetch the nodemask associated with a policy. Currently, get_policy_nodemask() returns the set of nodes with memory, when the policy 'mode' is 'PREFERRED, and the preferred_node is < 0. Return the set of allowed nodes instead. This will already have been masked to include only nodes with memory.

Ok.

Quoted 8 lines

2) When a task is moved into a [new] cpuset, mpol_rebind_policy() is called to adjust any task and vma policy nodes to be valid in the new cpuset. However, when the policy is MPOL_PREFERRED, and the preferred_node is <0, no rebind is necessary. The "local allocation" indication is valid in any cpuset. Existing code will "do the right thing" because node_remap() will just return the argument node when it is outside of the valid range of node ids. However, I think it is clearer and cleaner to skip the remap explicitly in this case.

Sounds good. This is on the way to having cpuset relative node numbering???

Quoted 9 lines

3) mpol_to_str() produces a printable, "human readable" string from a struct mempolicy. For MPOL_PREFERRED with preferred_node <0, show the entire set of valid nodes. Although, technically, MPOL_PREFERRED takes only a single node, preferred_node <0 is a local allocation policy, with the preferred node determined by the context where the task is executing. All of the allowed nodes are possible, as the task migrates amoung the nodes in the cpuset. Without this change, I believe that node_set() [via set_bit()] will set bit 31, resulting in a misleading display.

Hmmm. But one wants mpol_to_str to represent the memory policy not the context information that may change through migration. What you do there is provide information from the context. You could add the nodemask but I think we need to have some indicator that this policy is referring to the local policy.

Quoted 5 lines

Hmmm. But one wants mpol_to_str to represent the memory policy not the context information that may change through migration. What you do there is provide information from the context. You could add the nodemask but I think we need to have some indicator that this policy is referring to the local policy.

True. I could make mpol_to_str return something like "*" for the nodemask and document this as "any allowed".

However, this will involve testing possibly several words of bitmask in the allocation path. Instead, I chose to encode the "context-dependent policy" indication in the upper bits of the policy member of the mempolicy structure. This member must already be tested to determine the policy mode, so no extra memory references should be required. However, for testing the policy--e.g., in the several switch() and if() statements--the context flag must be masked off using the policy_mode() inline function. On the upside, this allows additional flags to be so encoded, should that become useful.

Hi Lee -- sorry for the delay in responding. Yes, this provides
exactly the feature set I was interested in. One question regarding:

Quoted 11 lines · Lee Schermerhorn

However, this will involve testing possibly several words of bitmask in the allocation path. Instead, I chose to encode the "context-dependent policy" indication in the upper bits of the policy member of the mempolicy structure. This member must already be tested to determine the policy mode, so no extra memory references should be required. However, for testing the policy--e.g., in the several switch() and if() statements--the context flag must be masked off using the policy_mode() inline function. On the upside, this allows additional flags to be so encoded, should that become useful.

Instead of creating MPOL_CONTEXT, did you consider instead creating a
new MPOL for this, such as MPOL_INTERLEAVE_ALL? If the only intended user of the MPOL_CONTEXT "flag" is just MPOL_INTERLEAVE_ALL, it seems like you'll have simpler code this way.

Hi Lee -- sorry for the delay in responding. Yes, this provides exactly
the feature set I was interested in. One question regarding:

Quoted 11 lines · Lee Schermerhorn

However, this will involve testing possibly several words of bitmask in the allocation path. Instead, I chose to encode the "context-dependent policy" indication in the upper bits of the policy member of the mempolicy structure. This member must already be tested to determine the policy mode, so no extra memory references should be required. However, for testing the policy--e.g., in the several switch() and if() statements--the context flag must be masked off using the policy_mode() inline function. On the upside, this allows additional flags to be so encoded, should that become useful.

Instead of creating MPOL_CONTEXT, did you consider instead creating a
new MPOL for this, such as MPOL_INTERLEAVE_ALL? If the only intended user of the MPOL_CONTEXT "flag" is just MPOL_INTERLEAVE_ALL, it seems like you'll have simpler code this way.

I did think about it, and I did see your mail about this. I guess "simpler code" is in the eye of the beholder. I consider "cpuset independent interleave" to be an instance of MPOL_INTERLEAVE using a context dependent nodemask. If we have a separate policy for this [really should be MPOL_INTERLEAVE_ALLOWED, don't you think?], would we then want a separate policy for "local preferred"--e.g., MPOL_PREFERRED_LOCAL? If we did this internally, I wouldn't want to expose it via the APIs. We already have an established way to indicate "local preferred"--the NULL/empty nodemask. Can't break the API, so I chose to use the same way to indicate "all allowed" interleave.

Just one code note:

mm/mempolicy.c int __mpol_equal(struct mempolicy *a, st · line 1442

Quoted 3 lines · Lee Schermerhorn

mm/mempolicy.c +2 -1

-		return nodes_equal(a->v.nodes, b->v.nodes);
+		return a->policy & MPOL_CONTEXT ||
+			nodes_equal(a->v.nodes, b->v.nodes);

	For the sake of my sanity, can we add () around a->policy & 

MPOL_CONTEXT? 8-) This falls into order of precedence that I don't trust myself to memorize.

Quoted 8 lines · Ethan Solomita

Just one code note:

mm/mempolicy.c int __mpol_equal(struct mempolicy *a, st · line 1442

Quoted 3 lines · Lee Schermerhorn

mm/mempolicy.c +2 -1

-		return nodes_equal(a->v.nodes, b->v.nodes);
+		return a->policy & MPOL_CONTEXT ||
+			nodes_equal(a->v.nodes, b->v.nodes);

	For the sake of my sanity, can we add () around a->policy & 

MPOL_CONTEXT? 8-) This falls into order of precedence that I don't trust myself to memorize.

I agree and I would have done that, but then someone would have dinged me for "unneeded parentheses"--despite the fact that I can't find anything in the style guide about this [except in the bit about macro definitions that says to always add parentheses around expressions defining constants]. Can't win for losin' :-(.

Quoted 10 lines · Ethan Solomita

Just one code note:

mm/mempolicy.c int __mpol_equal(struct mempolicy *a, st · line 1442

Quoted 3 lines · Lee Schermerhorn

mm/mempolicy.c +2 -1

-		return nodes_equal(a->v.nodes, b->v.nodes);
+		return a->policy & MPOL_CONTEXT ||
+			nodes_equal(a->v.nodes, b->v.nodes);

	For the sake of my sanity, can we add () around a->policy & 

MPOL_CONTEXT? 8-) This falls into order of precedence that I don't trust myself to memorize.

I agree and I would have done that, but then someone would have dinged me for "unneeded parentheses"--despite the fact that I can't find anything in the style guide about this [except in the bit about macro definitions that says to always add parentheses around expressions defining constants]. Can't win for losin' :-(.

Do we want do_get_mempolicy() to return a policy number with
MPOL_CONTEXT set? That's what's happening with this patch, and I expect it'll confuse userland apps, e.g. numactl.

Do we want do_get_mempolicy() to return a policy number with
MPOL_CONTEXT set? That's what's happening with this patch, and I expect it'll confuse userland apps, e.g. numactl.

Quoted 3 lines · Ethan Solomita

Do we want do_get_mempolicy() to return a policy number with
MPOL_CONTEXT set? That's what's happening with this patch, and I expect it'll confuse userland apps, e.g. numactl.

Do we have a consistent way to deal with MPOL_CONTEXT now? I thought this was just to test some ideas.

PATCH/RFC 05/05 - add MPOL_F_MEMS_ALLOWED get_mempolicy() flag

Against: 2.6.23-rc3-mm1

Allow an application to query the memories allowed by its context.

From what understand, an application that is only policy aware can run

Updated numa_memory_policy.txt to mention that applications can use this to obtain allowed memories for constructing valid policies.

TODO: update out-of-tree libnuma wrapper[s], or maybe add a new wrapper--e.g., numa_get_mems_allowed() ?

Tested with memtoy V>=0.13.

Signed-off-by Lee Schermerhorn <[email protected]>

 Documentation/vm/numa_memory_policy.txt |   28 +++++++++++-----------------
 include/linux/mempolicy.h               |    1 +
 mm/mempolicy.c                          |   14 +++++++++++++-
 3 files changed, 25 insertions(+), 18 deletions(-)

Index: Linux/include/linux/mempolicy.h
===================================================================

include/linux/mempolicy.h +1 -0

@@ -26,6 +26,7 @@
 /* Flags for get_mem_policy */
 #define MPOL_F_NODE	(1<<0)	/* return next IL mode instead of node mask */
 #define MPOL_F_ADDR	(1<<1)	/* look up vma using address */
+#define MPOL_F_MEMS_ALLOWED (1<<2) /* return allowed memories */
 
 /* Flags for mbind */
 #define MPOL_MF_STRICT	(1<<0)	/* Verify existing pages in the mapping */

Index: Linux/mm/mempolicy.c
===================================================================

mm/mempolicy.c +3 -1

@@ -560,8 +560,20 @@ static long do_get_mempolicy(int *policy
 	struct mempolicy *pol = current->mempolicy;
 
 	cpuset_update_task_memory_state();
-	if (flags & ~(unsigned long)(MPOL_F_NODE|MPOL_F_ADDR))
+	if (flags &
+		~(unsigned long)(MPOL_F_NODE|MPOL_F_ADDR|MPOL_F_MEMS_ALLOWED))
 		return -EINVAL;
+

mm/mempolicy.c +0 -0

 	if (flags & MPOL_F_ADDR) {
 		down_read(&mm->mmap_sem);
 		vma = find_vma_intersection(mm, addr, addr+1);

Index: Linux/Documentation/vm/numa_memory_policy.txt
===================================================================

Documentation/vm/numa_memory_policy.txt +11 -18

@@ -294,24 +294,20 @@ MEMORY POLICIES AND CPUSETS
 
 Memory policies work within cpusets as described above.  For memory policies
 that require a node or set of nodes, the nodes are restricted to the set of
-nodes whose memories are allowed by the cpuset constraints.  If the
-intersection of the set of nodes specified for the policy and the set of nodes
-allowed by the cpuset is the empty set, the policy is considered invalid and
-cannot be installed.
+nodes whose memories are allowed by the cpuset constraints.  If the nodemask
+specified for the policy contains nodes that are not allowed by the cpuset, or
+the intersection of the set of nodes specified for the policy and the set of
+nodes with memory is the empty set, the policy is considered invalid
+and cannot be installed.
 
 The interaction of memory policies and cpusets can be problematic for a
 couple of reasons:
 
-1) the memory policy APIs take physical node id's as arguments.  However, the
-   memory policy APIs do not provide a way to determine what nodes are valid
-   in the context where the application is running.  An application MAY consult
-   the cpuset file system [directly or via an out of tree, and not generally
-   available, libcpuset API] to obtain this information, but then the
-   application must be aware that it is running in a cpuset and use what are
-   intended primarily as administrative APIs.
-
-   However, as long as the policy specifies at least one node that is valid
-   in the controlling cpuset, the policy can be used.
+1) the memory policy APIs take physical node id's as arguments.  As mentioned
+   above, it is illegal to specify nodes that are not allowed in the cpuset.
+   The application must query the allowed nodes using the get_mempolicy()
+   API with the MPOL_F_MEMS_ALLOWED flag to determine the allowed nodes and
+   restrict itself to those nodes.
 
 2) when tasks in two cpusets share access to a memory region, such as shared
    memory segments created by shmget() of mmap() with the MAP_ANONYMOUS and

@@ -321,7 +317,5 @@ couple of reasons:
    the memory policy APIs, as well as knowing in what cpusets other task might
    be attaching to the shared region, to use the cpuset information.
    Furthermore, if the cpusets' allowed memory sets are disjoint, "local"
+   allocation and "contextual interleave" are the only valid policies.
 
-Note, however, that local allocation, whether specified by MPOL_DEFAULT or
-MPOL_PREFERRED with an empty nodemask and "contextual interleave"--
-MPOL_INTERLEAVE with an empty nodemask--are valid policies in any context.
 
--

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

Quoted 6 lines · Lee Schermerhorn

PATCH/RFC 05/05 - add MPOL_F_MEMS_ALLOWED get_mempolicy() flag

Against: 2.6.23-rc3-mm1

Allow an application to query the memories allowed by its context.

I think you may be underplaying the significance of this patch here.

Quoted 1 line

From what understand, an application that is only policy aware can run

inside a cpuset and think it can use nodes it's not allowed. If that is right, then the language here implies that a policy-aware application can now get useful information without going through complicated hoops. That is pretty important.

Quoted 45 lines

Updated numa_memory_policy.txt to mention that applications can use this to obtain allowed memories for constructing valid policies.

TODO: update out-of-tree libnuma wrapper[s], or maybe add a new wrapper--e.g., numa_get_mems_allowed() ?

Tested with memtoy V>=0.13.

Signed-off-by Lee Schermerhorn <[email protected]>

 Documentation/vm/numa_memory_policy.txt |   28 +++++++++++-----------------
 include/linux/mempolicy.h               |    1 +
 mm/mempolicy.c                          |   14 +++++++++++++-
 3 files changed, 25 insertions(+), 18 deletions(-)

Index: Linux/include/linux/mempolicy.h
===================================================================

include/linux/mempolicy.h +1 -0

@@ -26,6 +26,7 @@
 /* Flags for get_mem_policy */
 #define MPOL_F_NODE	(1<<0)	/* return next IL mode instead of node mask */
 #define MPOL_F_ADDR	(1<<1)	/* look up vma using address */
+#define MPOL_F_MEMS_ALLOWED (1<<2) /* return allowed memories */
 
 /* Flags for mbind */
 #define MPOL_MF_STRICT	(1<<0)	/* Verify existing pages in the mapping */

Index: Linux/mm/mempolicy.c
===================================================================

mm/mempolicy.c +7 -1

@@ -560,8 +560,20 @@ static long do_get_mempolicy(int *policy
 	struct mempolicy *pol = current->mempolicy;
 
 	cpuset_update_task_memory_state();
-	if (flags & ~(unsigned long)(MPOL_F_NODE|MPOL_F_ADDR))
+	if (flags &
+		~(unsigned long)(MPOL_F_NODE|MPOL_F_ADDR|MPOL_F_MEMS_ALLOWED))
 		return -EINVAL;
+
+	if (flags & MPOL_F_MEMS_ALLOWED) {
+		if (flags & (MPOL_F_NODE|MPOL_F_ADDR))
+			return -EINVAL;
+		*policy = 0;	/* just so it's initialized */

mm/mempolicy.c static long do_get_mempolicy(int *policy · line 576

Quoted 6 lines

mm/mempolicy.c +6 -0

+		if (!nmask)
+			return -EFAULT;
+		*nmask  = cpuset_current_mems_allowed;
+		return 0;
+	}
+

Seems a fair implementation.

Some of these patches have been previously posted for comment individually. The patches also update the numa_memory_policy document where applicable. Needed man page updates are flagged. I will provide the needed updates for any of the patches that are accepted.

Cleanups:

1) Fix reference counting for shared, vma and other task's mempolicies. This was discussed back in late June'07, but never went anywhere. Closes possible races and fixes potential memory leak for shared policies. Adds code to allocation paths.

Patch does NOT update numa_memory_policy doc--the doc doesn't go into that much detail regarding design. Perhaps it should.

2) use MPOL_PREFERRED with preferred_node = -1 for system default local allocation. This removes all usage of MPOL_DEFAULT in in-kernel struct mempolicy 'policy' members. MPOL_DEFAULT is now an API-only value that requests fall back to the default policy for the target context [task or vma/shared policy]. This simplifies the description of policies and removes some runtime tests in the page allocation paths.

Needs man page update to clarify meaning of MPOL_DEFAULT with this patch. Should simplify things a bit.

2) cleanup MPOL_PREFERRED "local allocation" handling -- i.e., when preferred_node == -1.

Needs man page update to clarify returned nodemask when MPOL_PERFERRED policy specifies local allocation.

Enhancements:

4) cpuset-independent [a.k.a. "contextual"] interleave policy: NULL or empty nodemask to mempolicy API [set_mempolicy() and mbind()] now means "interleave over all permitted nodes in allocation context".

Needs man page update to describe contextual interleave--how to specify, behavior, ...

5) add MPOL_F_MEMS_ALLOWED flag for get_mempolicy(). Allows an application to query the valid nodes to avoid EINVAL errors when attempting to install memory policies from within a memory constrained cpuset.

Needs man page update to describe flag, behavior. Could also use libnuma update -- e.g., new numa_mems_allowed()

Testing:

I've run with these patches for the past few weeks. Some moderate stress testing and functional testing on an ia64 NUMA platform, shows no issues nor regression. memtoy >= 0.13 supports MPOL_F_MEMS_ALLOWED flag [mems command].

Some of the patches [ref count fix, contextual interneavel] do add code in some of the allocation paths. I hope to get some time in the next month on a terabyte system [~64 million 16KB pages] to measure the overhead of these patches allocating and migrating a few million pages to expose any increased overhead.

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

Andi, Christoph, Mel [added to cc]:

Any comments on these patches, posted 30aug? I've rebased to 23-rc4-mm1, but before reposting, I wanted to give you a chance to comment.

I'm going to add Mel's "one zonelist" series to my mempolicy tree with these patches and see how that goes. I'll slide Mel's patches in below these, as it looks like they're closer to acceptance into -mm.

Ethan: I believe that patch #4 provides the cpuset independent interleave capability that you were looking for. Does this meet your requirements?

Regards,
Lee

Quoted 64 lines · Lee Schermerhorn

Some of these patches have been previously posted for comment individually. The patches also update the numa_memory_policy document where applicable. Needed man page updates are flagged. I will provide the needed updates for any of the patches that are accepted.

Cleanups:

1) Fix reference counting for shared, vma and other task's mempolicies. This was discussed back in late June'07, but never went anywhere. Closes possible races and fixes potential memory leak for shared policies. Adds code to allocation paths.

Patch does NOT update numa_memory_policy doc--the doc doesn't go into that much detail regarding design. Perhaps it should.

2) use MPOL_PREFERRED with preferred_node = -1 for system default local allocation. This removes all usage of MPOL_DEFAULT in in-kernel struct mempolicy 'policy' members. MPOL_DEFAULT is now an API-only value that requests fall back to the default policy for the target context [task or vma/shared policy]. This simplifies the description of policies and removes some runtime tests in the page allocation paths.

Needs man page update to clarify meaning of MPOL_DEFAULT with this patch. Should simplify things a bit.

2) cleanup MPOL_PREFERRED "local allocation" handling -- i.e., when preferred_node == -1.

Needs man page update to clarify returned nodemask when MPOL_PERFERRED policy specifies local allocation.

Enhancements:

4) cpuset-independent [a.k.a. "contextual"] interleave policy: NULL or empty nodemask to mempolicy API [set_mempolicy() and mbind()] now means "interleave over all permitted nodes in allocation context".

Needs man page update to describe contextual interleave--how to specify, behavior, ...

5) add MPOL_F_MEMS_ALLOWED flag for get_mempolicy(). Allows an application to query the valid nodes to avoid EINVAL errors when attempting to install memory policies from within a memory constrained cpuset.

Needs man page update to describe flag, behavior. Could also use libnuma update -- e.g., new numa_mems_allowed()

Testing:

I've run with these patches for the past few weeks. Some moderate stress testing and functional testing on an ia64 NUMA platform, shows no issues nor regression. memtoy >= 0.13 supports MPOL_F_MEMS_ALLOWED flag [mems command].

Some of the patches [ref count fix, contextual interneavel] do add code in some of the allocation paths. I hope to get some time in the next month on a terabyte system [~64 million 16KB pages] to measure the overhead of these patches allocating and migrating a few million pages to expose any increased overhead.

Quoted 5 lines

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

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

Quoted 6 lines · Lee Schermerhorn

Andi, Christoph, Mel [added to cc]:

Any comments on these patches, posted 30aug? I've rebased to 23-rc4-mm1, but before reposting, I wanted to give you a chance to comment.

I hadn't intended to comment but because you asked, I took a look through. It wasn't an in-depth review but nothing jumped out as broken to me and I commented on what I spotted. The last patch to me was the most interesting and justifies the set unless someone can think of a real reason to not extend the get_mempolicy() API to retrieve this information. I made comments on what I saw but as I'm not a frequent user of policies so take the suggestions with a grain of salt.

Unless something jumps out to someone else, I think it'll be ready for wider testing after your next release.

Quoted 4 lines

I'm going to add Mel's "one zonelist" series to my mempolicy tree with these patches and see how that goes. I'll slide Mel's patches in below these, as it looks like they're closer to acceptance into -mm.

Andi, Christoph, Mel [added to cc]:

Any comments on these patches, posted 30aug? I've rebased to 23-rc4-mm1, but before reposting, I wanted to give you a chance to comment.

I'm going to add Mel's "one zonelist" series to my mempolicy tree with these patches and see how that goes. I'll slide Mel's patches in below these, as it looks like they're closer to acceptance into -mm.

Quoted 5 lines · Lee Schermerhorn

Andi, Christoph, Mel [added to cc]:

Any comments on these patches, posted 30aug? I've rebased to 23-rc4-mm1, but before reposting, I wanted to give you a chance to comment.

Sorry that it took some time but I only just got around to look at them. The one patch that I acked may be of higher priority and should probably go in immediately to be merged for 2.6.24.

Quoted 4 lines

I'm going to add Mel's "one zonelist" series to my mempolicy tree with these patches and see how that goes. I'll slide Mel's patches in below these, as it looks like they're closer to acceptance into -mm.

That patchset will have a significant impact on yours. You may be able to get rid of some of the switch statements. It would be great if we had some description as to where you are heading with the incremental changes to the memory policy semantics? I sure wish we would have something more consistent and easier to understand.

Quoted 8 lines · Christoph Lameter

Quoted 5 lines · Lee Schermerhorn

Andi, Christoph, Mel [added to cc]:

Any comments on these patches, posted 30aug? I've rebased to 23-rc4-mm1, but before reposting, I wanted to give you a chance to comment.

Quoted 3 lines

Sorry that it took some time but I only just got around to look at them. The one patch that I acked may be of higher priority and should probably go in immediately to be merged for 2.6.24.

OK. I'll pull that from the set and post it separately. I'll see if it conflicts with Mel's set. If so, we'll need to decide on the ordering. Do we think Mel's patches will make .24?

Quoted 10 lines

Quoted 4 lines

I'm going to add Mel's "one zonelist" series to my mempolicy tree with these patches and see how that goes. I'll slide Mel's patches in below these, as it looks like they're closer to acceptance into -mm.

That patchset will have a significant impact on yours. You may be able to get rid of some of the switch statements. It would be great if we had some description as to where you are heading with the incremental changes to the memory policy semantics? I sure wish we would have something more consistent and easier to understand.

The general reaction to such descriptions is "show me the code." So, if we agree that Mel's patches should go first, I'll rebase and update the numa_memory_policy doc accordingly to explain the resulting semantics. Perhaps Mel should considering updating that document where his patches change/invalidate the current descriptions.

Does this sound like a resonable way to proceed?

Quoted 9 lines · Lee Schermerhorn

Quoted 8 lines · Christoph Lameter

Quoted 5 lines · Lee Schermerhorn

Andi, Christoph, Mel [added to cc]:

Any comments on these patches, posted 30aug? I've rebased to 23-rc4-mm1, but before reposting, I wanted to give you a chance to comment.

Quoted 8 lines

Quoted 3 lines

Sorry that it took some time but I only just got around to look at them. The one patch that I acked may be of higher priority and should probably go in immediately to be merged for 2.6.24.

OK. I'll pull that from the set and post it separately. I'll see if it conflicts with Mel's set. If so, we'll need to decide on the ordering. Do we think Mel's patches will make .24?

I am hoping they will. They remove the nasty hack in relation to MPOL_BIND applying to the top two highest zones when ZONE_MOVABLE is configured and let MPOL_BIND use a node-local policy which I feel is important. There hasn't been a consensus on this yet and I would expect that to be hashed out at the start of the merge window as usual.

Quoted 19 lines · Mel Gorman

Quoted 9 lines · Lee Schermerhorn

Quoted 8 lines · Christoph Lameter

Quoted 5 lines · Lee Schermerhorn

Andi, Christoph, Mel [added to cc]:

Any comments on these patches, posted 30aug? I've rebased to 23-rc4-mm1, but before reposting, I wanted to give you a chance to comment.

Quoted 8 lines

Quoted 3 lines

Sorry that it took some time but I only just got around to look at them. The one patch that I acked may be of higher priority and should probably go in immediately to be merged for 2.6.24.

OK. I'll pull that from the set and post it separately. I'll see if it conflicts with Mel's set. If so, we'll need to decide on the ordering. Do we think Mel's patches will make .24?

Quoted 5 lines

I am hoping they will. They remove the nasty hack in relation to MPOL_BIND applying to the top two highest zones when ZONE_MOVABLE is configured and let MPOL_BIND use a node-local policy which I feel is important. There hasn't been a consensus on this yet and I would expect that to be hashed out at the start of the merge window as usual.

OK. I haven't had a chance to check your patch for the mem controller issue yet. Should I test with your latest series that uses the struct containing the zone id? I know that Christoph had concerns about increasing the number of cache lines. Which way do you think we'll go on this?

Do we think Mel's patches will make .24?

Quoted 5 lines

That patchset will have a significant impact on yours. You may be able to get rid of some of the switch statements. It would be great if we had some description as to where you are heading with the incremental changes to the memory policy semantics? I sure wish we would have something more consistent and easier to understand.

The general reaction to such descriptions is "show me the code." So, if we agree that Mel's patches should go first, I'll rebase and update the numa_memory_policy doc accordingly to explain the resulting semantics. Perhaps Mel should considering updating that document where his patches change/invalidate the current descriptions.

Does this sound like a resonable way to proceed?

Quoted 2 lines

Do we think Mel's patches will make .24?

No,

<snip>

What do you see holding it up? Is it the fact we are no longer doing the pointer packing and you don't want that structure to exist, or is it simply a case that 2.6.23 is too close the door and it won't get adequate coverage in -mm?

Quoted 4 lines · Mel Gorman

What do you see holding it up? Is it the fact we are no longer doing the pointer packing and you don't want that structure to exist, or is it simply a case that 2.6.23 is too close the door and it won't get adequate coverage in -mm?

No its not the pointer packing. The problem is that the patches have not been merged yet and 2.6.23 is close. We would need to merge it very soon and get some exposure in mm. Andrew?

Quoted 10 lines · On Thu, 13 Sep 2007 11:26:19 -0700 (PDT) Christoph Lameter <[email protected]>…

Quoted 4 lines · Mel Gorman

What do you see holding it up? Is it the fact we are no longer doing the pointer packing and you don't want that structure to exist, or is it simply a case that 2.6.23 is too close the door and it won't get adequate coverage in -mm?

No its not the pointer packing. The problem is that the patches have not been merged yet and 2.6.23 is close. We would need to merge it very soon and get some exposure in mm. Andrew?

You rang?

To which patches do you refer? "Memory Policy Cleanups and Enhancements"? That's still in my queue somewhere, but a) it has "RFC" in it which usually makes me run away and b) we already have no fewer than 221 memory management patches queued.

Quoted 10 lines · On Thu, 13 Sep 2007 11:26:19 -0700 (PDT) Christoph Lameter <[email protected]>…

Quoted 4 lines · Mel Gorman

What do you see holding it up? Is it the fact we are no longer doing the pointer packing and you don't want that structure to exist, or is it simply a case that 2.6.23 is too close the door and it won't get adequate coverage in -mm?

No its not the pointer packing. The problem is that the patches have not been merged yet and 2.6.23 is close. We would need to merge it very soon and get some exposure in mm. Andrew?

You rang?

To which patches do you refer? "Memory Policy Cleanups and Enhancements"? That's still in my queue somewhere, but a) it has "RFC" in it which usually makes me run away and b) we already have no fewer than 221 memory management patches queued.

On (13/09/07 14:17), Andrew Morton didst pronounce:

Quoted 14 lines

Quoted 10 lines · On Thu, 13 Sep 2007 11:26:19 -0700 (PDT) Christoph Lameter <[email protected]>…

Quoted 4 lines · Mel Gorman

What do you see holding it up? Is it the fact we are no longer doing the pointer packing and you don't want that structure to exist, or is it simply a case that 2.6.23 is too close the door and it won't get adequate coverage in -mm?

No its not the pointer packing. The problem is that the patches have not been merged yet and 2.6.23 is close. We would need to merge it very soon and get some exposure in mm. Andrew?

Quoted 7 lines

You rang?

To which patches do you refer? "Memory Policy Cleanups and Enhancements"? That's still in my queue somewhere, but a) it has "RFC" in it which usually makes me run away and b) we already have no fewer than 221 memory management patches queued.

Christoph's question is in relation to the patchset "Use one zonelist per node instead of multiple zonelists v7" and whether one zonelist will be merged in 2.6.24 in your opinion. I am hoping "yes" because it removes that hack with ZONE_MOVABLE and policies. I had sent you a version (v5) but there were further suggestions on ways to improve it so we're up to v7 now. Lee will hopefully be able to determine if v7 regresses policy behaviour or not.

Quoted 23 lines · Mel Gorman

On (13/09/07 14:17), Andrew Morton didst pronounce:

Quoted 14 lines

Quoted 10 lines · On Thu, 13 Sep 2007 11:26:19 -0700 (PDT) Christoph Lameter <[email protected]>…

Quoted 4 lines · Mel Gorman

What do you see holding it up? Is it the fact we are no longer doing the pointer packing and you don't want that structure to exist, or is it simply a case that 2.6.23 is too close the door and it won't get adequate coverage in -mm?

No its not the pointer packing. The problem is that the patches have not been merged yet and 2.6.23 is close. We would need to merge it very soon and get some exposure in mm. Andrew?

Quoted 7 lines

You rang?

To which patches do you refer? "Memory Policy Cleanups and Enhancements"? That's still in my queue somewhere, but a) it has "RFC" in it which usually makes me run away and b) we already have no fewer than 221 memory management patches queued.

Quoted 7 lines

Christoph's question is in relation to the patchset "Use one zonelist per node instead of multiple zonelists v7" and whether one zonelist will be merged in 2.6.24 in your opinion. I am hoping "yes" because it removes that hack with ZONE_MOVABLE and policies. I had sent you a version (v5) but there were further suggestions on ways to improve it so we're up to v7 now. Lee will hopefully be able to determine if v7 regresses policy behaviour or not.

Hi, Mel:

I'm running with your patches now. An earlier version--just received v7 end of day yesterday. Will rebuild today. I've been using the kernel with your patches for general patch development and kernel building on my ia64 numa platform. Before I rebooted to test another kernel [reclaim scalability/noreclaim patch set], your mail prompted me to try a couple of memtoy migration scripts. I managed to hang/panic the system with a null pointer deref and a very interesting stack trace, which I didn't capture [want to test w/ v7].

The trace included some
kprobes functions--which I'm not using--and a lot of tcp/network stack routines. I have no clue whether these are related to your patches. The hang that I experienced before the panic could have been a local site network glitch [happens, sometimes] that triggered a fault in the network stack.

Again, I'll retest with the v7 patches today. In the meantime, you might want to grab memtoy from:
http://free.linux.hp.com/~lts/Tools/memtoy-latest.tar.gz
and try out the test scripts in the Xpm-tests/Mbind directory. Note that these scripts assume a 4-node numa system, but from the comments you should be able to mod them for whatever numa system you have available. Building memtoy can also be a bit of a challenge, depending on your environment--no autoconfig or such. Check out the README for instructions/caveats/...

Since you are going to rework the one zonelist patch again anyway

I would
suggest to either use Kame-san full approach and include the node in the zonelist item structure (it does not add any memory since the struct is word aligned and this is an int)

or go back to the earlier approach of
packing the zone id into the low bits which would reduce the cache footprint.

Kame-san's approach is likely very useful if we have a lot of nodes and need to match a nodemask to a zonelist.

It increases the size on 32 bit NUMA which we've established is not confined to the NUMAQ. I think it's best to evaluate adding the node separetly at a later time.

Quoted 3 lines · Mel Gorman

It increases the size on 32 bit NUMA which we've established is not confined to the NUMAQ. I think it's best to evaluate adding the node separetly at a later time.

Na... 32 bit NUMA is not that important.

Quoted 6 lines · Christoph Lameter

Quoted 3 lines · Mel Gorman

It increases the size on 32 bit NUMA which we've established is not confined to the NUMAQ. I think it's best to evaluate adding the node separetly at a later time.

Quoted 2 lines

Na... 32 bit NUMA is not that important.

Oh and another thing: If you make both the node and the zoneid unsigned short then there is no loss. zoneid is always < 4 and node is always < 1024. We even could make the zoneid u8 and stuff some more stuff into the list.

ok, that is a very good point. When -mm is settled and one-zonelist in it's v7 incarnation has been merged, I'll roll a patch that does this and go through another test cycle. Lee's testing has passed one-zonelist in it's current incarnation and this close, I don't want to do more revisions. The patch to do this will be very small so should be easy to review in isolation. Sound like a plan?

Quoted 3 lines · Mel Gorman

It increases the size on 32 bit NUMA which we've established is not confined to the NUMAQ. I think it's best to evaluate adding the node separetly at a later time.

Na... 32 bit NUMA is not that important.

On (13/09/07 14:17), Andrew Morton didst pronounce:

Quoted 14 lines

Quoted 10 lines · On Thu, 13 Sep 2007 11:26:19 -0700 (PDT) Christoph Lameter <[email protected]>…

Quoted 4 lines · Mel Gorman

What do you see holding it up? Is it the fact we are no longer doing the pointer packing and you don't want that structure to exist, or is it simply a case that 2.6.23 is too close the door and it won't get adequate coverage in -mm?

No its not the pointer packing. The problem is that the patches have not been merged yet and 2.6.23 is close. We would need to merge it very soon and get some exposure in mm. Andrew?

Quoted 7 lines

You rang?

To which patches do you refer? "Memory Policy Cleanups and Enhancements"? That's still in my queue somewhere, but a) it has "RFC" in it which usually makes me run away and b) we already have no fewer than 221 memory management patches queued.

Christoph's question is in relation to the patchset "Use one zonelist per node instead of multiple zonelists v7" and whether one zonelist will be merged in 2.6.24 in your opinion. I am hoping "yes" because it removes that hack with ZONE_MOVABLE and policies. I had sent you a version (v5) but there were further suggestions on ways to improve it so we're up to v7 now. Lee will hopefully be able to determine if v7 regresses policy behaviour or not.

Quoted 23 lines

On (13/09/07 14:17), Andrew Morton didst pronounce:

Quoted 14 lines

Quoted 10 lines · On Thu, 13 Sep 2007 11:26:19 -0700 (PDT) Christoph Lameter <[email protected]>…

Quoted 4 lines · Mel Gorman

What do you see holding it up? Is it the fact we are no longer doing the pointer packing and you don't want that structure to exist, or is it simply a case that 2.6.23 is too close the door and it won't get adequate coverage in -mm?

No its not the pointer packing. The problem is that the patches have not been merged yet and 2.6.23 is close. We would need to merge it very soon and get some exposure in mm. Andrew?

Quoted 7 lines

You rang?

To which patches do you refer? "Memory Policy Cleanups and Enhancements"? That's still in my queue somewhere, but a) it has "RFC" in it which usually makes me run away and b) we already have no fewer than 221 memory management patches queued.

Quoted 7 lines

Christoph's question is in relation to the patchset "Use one zonelist per node instead of multiple zonelists v7" and whether one zonelist will be merged in 2.6.24 in your opinion. I am hoping "yes" because it removes that hack with ZONE_MOVABLE and policies. I had sent you a version (v5) but there were further suggestions on ways to improve it so we're up to v7 now. Lee will hopefully be able to determine if v7 regresses policy behaviour or not.

I've been testing the "one zonelist patches" with various memtoy scripts, and they seem to be working--i.e., pages ending up where I expect.

The tests aren't exhaustive or even particularly stressful, but
I did test all of the policies. I also measured the time to allocate
4G [256K pages] with several policies using memtoy. Here are the results--rough averages of 10 runs; very close grouping for each test--smaller is better:

Test 23-rc4-mm1 +one zonelist patches
sys default policy 2.768s > 2.755s
task pol bind local(1) 2.789s ~= 2.789s
task pol bind remote(2) 3.774s < 3.780s
vma pol bind local(3) 2.794s > 2.790s
vma pol bind remote(4) 3.769s < 3.777s
vma pol pref local(5) 2.774s > 2.770s
vma interleave 0-3 3.446s > 3.436s

Notes:
1) numactl -c3 -m3
2) numactl -c1 -m3
3) memtoy bound to node 3, mbind MPOL_BIND to node 3
4) memtoy bound to node 1, mbind MPOL_BIND to node 3
5) mbind MPOL_PREFERRED, null nodemask [preferred_node == -1 internally]

The results are very close, but it looks like one-zonelist is a bit faster for local allocations and a bit slower for remote allocations.

None of these tests overflowed the target node.

I've also run a moderate stress test [half an hour now] and it's holding up.

I'm still trying to absorb the patches, but so far they look good. Perhaps Andrew can tack them onto the bottom of the next -mm so that if someone else finds issues, they won't complicate merging earlier patches upstream?

Quoted 5 lines

I'm still trying to absorb the patches, but so far they look good. Perhaps Andrew can tack them onto the bottom of the next -mm so that if someone else finds issues, they won't complicate merging earlier patches upstream?

I hope so. Andrew, how do you feel about pulling V7 into -mm?

Quoted 7 lines

Quoted 6 lines

I'm still trying to absorb the patches, but so far they look good. Perhaps Andrew can tack them onto the bottom of the next -mm so that if someone else finds issues, they won't complicate merging earlier patches upstream?

Quoted 1 line

I hope so. Andrew, how do you feel about pulling V7 into -mm?

umm, sure, once the churn rate falls to less than one new revision per day?

I need to get rc6-mm1 out, and it'll be crap, and it'll need another -mm shortly after that to get things vaguely stable.

On (14/09/07 16:15), Lee Schermerhorn didst pronounce:

Quoted 4 lines

I've also run a moderate stress test [half an hour now] and it's holding up.

Great.

I wonder why they are slower for the remote allocations. I wonder if what we're seeing is due to a longer zonelist that is filtered instead of a customised shorter zonelist. By and large though, the differences are small as to not be noticed.

Quoted 2 lines · Lee Schermerhorn

Andi, Christoph, Mel [added to cc]:

Quoted 3 lines

Any comments on these patches, posted 30aug? I've rebased to 23-rc4-mm1, but before reposting, I wanted to give you a chance to comment.

Sorry that it took some time but I only just got around to look at them. The one patch that I acked may be of higher priority and should probably go in immediately to be merged for 2.6.24.

Andi thought this was important enough to go in to 23 as a bug fix. I think it's at least as important as the one you acked. However, I don't know that anyone has ever cited a problem with it. Then again, they might not notice if they occasionally leak a mempolicy struct, or use a freed or reused one for page allocation. I'd be happier to see it cook in -mm for a while. So, I'll go ahead and rebase atop Mel's patches.

Thoughts?