More Mempolicy Reference Counting Fixes

PATCH 0/2 Memory Policy: More Reference Counting Fixes

While testing huge pages using shm segments created with the SHM_HUGETLB flag, I came across additional problems with memory policy reference counting. While tracking these down, I found even more... Some of the problems were introduced by myself in my previous mempolicy ref counting patch, and some were paths that I missed in my inspection and testing.

These 2 patches are both based against 2.6.23-rc8-mm2, but should also probably go to the .23-stable tree when it opens. They have been tested on ia64 and x86_64 platforms.

Lee

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

PATCH 1/2 Fix memory policy usage in pci driver

Against: 2.6.23-rc8-mm2

In an attempt to ensure memory allocation from the proper node, the pci driver temporarily replaces the current task's memory policy with the system default policy. Trying to be a good citizen, the driver then call's mpol_get() on the new policy. When it's finished probing, it undoes the '_get by calling mpol_free() [on the system default policy] and then restores the current task's saved mempolicy.

A couple of issues here:

1) it's never necessary to set a task's mempolicy to the system default policy in order to get system default allocation behavior. Simply set the current task's mempolicy to NULL and allocations will fall back to system default policy.

2) we should never [need to] call mpol_free() on the system default policy. [I plan on trapping this with a VM_BUG_ON() in a subsequent patch.]

This patch removes the calls to mpol_get() and mpol_free() and uses NULL for the temporary task mempolicy to effect default allocation behavior.

 drivers/pci/pci-driver.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Index: Linux/drivers/pci/pci-driver.c
===================================================================
@@ -177,13 +177,11 @@ static int pci_call_probe(struct pci_dri
 	    set_cpus_allowed(current, node_to_cpumask(node));
 	/* And set default memory allocation policy */
 	oldpol = current->mempolicy;
-	current->mempolicy = &default_policy;
-	mpol_get(current->mempolicy);
+	current->mempolicy = NULL;	/* fall back to system default policy */
 #endif
 	error = drv->probe(dev, id);
 #ifdef CONFIG_NUMA
 	set_cpus_allowed(current, oldmask);
-	mpol_free(current->mempolicy);
 	current->mempolicy = oldpol;
 #endif
 	return error;

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

Resend with 'RFC' removed. Please review/consider for merge.

Note: this is required BEFORE patch 2 of this series to avoid hitting the [VM_]BUG_ON()s added by the second patch.

Lee

====
PATCH 1/2 Fix memory policy usage in pci driver

Against: 2.6.23-rc8-mm2

In an attempt to ensure memory allocation from the local node, the pci driver temporarily replaces the current task's memory policy with the system default policy. Trying to be a good citizen, the driver then call's mpol_get() on the new policy. When it's finished probing, it undoes the '_get by calling mpol_free() [on the system default policy] and then restores the current task's saved mempolicy.

A couple of issues here:

1) it's never necessary to set a task's mempolicy to the system default policy in order to get system default allocation behavior. Simply set the current task's mempolicy to NULL and allocations will fall back to system default policy.

2) we should never [need to] call mpol_free() on the system default policy. [I plan on trapping this with a VM_BUG_ON() in a subsequent patch.]

This patch removes the calls to mpol_get() and mpol_free() and uses NULL for the temporary task mempolicy to effect default allocation behavior.

 drivers/pci/pci-driver.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Index: Linux/drivers/pci/pci-driver.c
===================================================================
--- Linux.orig/drivers/pci/pci-driver.c 2007-10-09 14:31:57.000000000
-0400
+++ Linux/drivers/pci/pci-driver.c 2007-10-09 14:43:57.000000000 -0400
@@ -177,13 +177,11 @@ static int pci_call_probe(struct pci_dri
set_cpus_allowed(current, node_to_cpumask(node));
/* And set default memory allocation policy */ oldpol = current->mempolicy;
- current->mempolicy = &default_policy;
- mpol_get(current->mempolicy);
+ current->mempolicy = NULL; /* fall back to system default policy */
#endif
error = drv->probe(dev, id);
#ifdef CONFIG_NUMA
set_cpus_allowed(current, oldmask);
- mpol_free(current->mempolicy);
current->mempolicy = oldpol;
#endif
return error;

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

I see that you're starting to build up the next -mm tree. Would you please apply the following patch? I posted it earlier [twice] and Christoph Ack'd it [twice]. Haven't heard any objections.

Lee

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

PATCH Mem Policy: Fix memory policy usage in pci driver

Against: 2.6.23-mm1

In an attempt to ensure memory allocation from the local node, the pci driver temporarily replaces the current task's memory policy with the system default policy. Trying to be a good citizen, the driver then call's mpol_get() on the new policy. When it's finished probing, it undoes the '_get by calling mpol_free() [on the system default policy] and then restores the current task's saved mempolicy.

A couple of issues here:

1) it's never necessary to set a task's mempolicy to the system default policy in order to get system default allocation behavior. Simply set the current task's mempolicy to NULL and allocations will fall back to system default policy.

2) we should never [need to] call mpol_free() on the system default policy. [I plan on trapping this with a VM_BUG_ON() in a subsequent patch.]

This patch removes the calls to mpol_get() and mpol_free() and uses NULL for the temporary task mempolicy to effect default allocation behavior.

Twice:

 drivers/pci/pci-driver.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Index: Linux/drivers/pci/pci-driver.c
===================================================================
@@ -177,13 +177,11 @@ static int pci_call_probe(struct pci_dri
 	    set_cpus_allowed(current, node_to_cpumask(node));
 	/* And set default memory allocation policy */
 	oldpol = current->mempolicy;
-	current->mempolicy = &default_policy;
-	mpol_get(current->mempolicy);
+	current->mempolicy = NULL;	/* fall back to system default policy */
 #endif
 	error = drv->probe(dev, id);
 #ifdef CONFIG_NUMA
 	set_cpus_allowed(current, oldmask);
-	mpol_free(current->mempolicy);
 	current->mempolicy = oldpol;
 #endif
 	return error;

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

On (11/10/07 15:11), Lee Schermerhorn didst pronounce:

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

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

PATCH 2/2 Mempolicy: Fixup Shm and Interleave Policy Reference Counting

Against: 2.6.23-rc8-mm2

* In the memory policy reference counting cleanup patch, I missed one path that needs to unreference the memory policy. After computing the target node for interleave policy, we need to drop the reference if the policy is not the system default nor the current task's policy.

* In huge_zonelist(), I was unconditionally unref'ing the policy in the interleave path, even when it was a policy that didn't need it. Fix this!

Note: I investigated moving the check for "policy_needs_unref" to the mpol_free() wrapper, but this led to nasty circular header dependencies. If we wanted to make mpol_free() an external function, rather than a static inline, I could do this and remove several checks. I'd still need to keep an explicit check in alloc_page_vma() if we want to use a tail-call for the fast path.

* get_vma_policy() assumes that shared policies are referenced by the get_policy() vm_op, if any. This is true for shmem_get_policy() but not for shm_get_policy() when the "backing file" does not support a get_policy() vm_op. The latter is the case for SHM_HUGETLB segments. Because get_vma_policy() expects the get_policy() op to have added a ref, it doesn't do so itself. This results in premature freeing of the policy. Add the mpol_get() to the shm_get_policy() op when the backing file doesn't support shared policies.

* Further, shm_get_policy() was falling back to current task's task policy if the backing file did not support get_policy() vm_op and the vma policy was null. This is not valid when get_vma_policy() is called from show_numa_map() as task != current. Also, this did not match the behavior of the shmem_get_policy() vm_op which did NOT fall back to task policy. So, modify shm_get_policy() NOT to fall back to current->mempolicy.

* Now, turns out that get_vma_policy() was not handling fallback to task policy correctly when the get_policy() vm_op returns NULL. Rather, it was falling back directly to system default policy. So, fix get_vma_policy() to use only non-NULL policy returned from the vma get_policy op and indicate that this policy does not need another ref count.

* Document mempolicy return value reference semantics assumed by the changes discussed above for the set_ and get_policy vm_ops in <linux/mm.h>--where the prototypes are defined.

* Add VM_BUG_ON()s to __mpol_free() and mpol_get() to trap attempts to ref/unref the system default policy. Should no longer occur.

 include/linux/mempolicy.h |    5 ++++-
 include/linux/mm.h        |   14 ++++++++++++++
 ipc/shm.c                 |    6 +++---
 mm/mempolicy.c            |   32 ++++++++++++++++++++++++--------
 4 files changed, 45 insertions(+), 12 deletions(-)

Index: Linux/mm/mempolicy.c
===================================================================
@@ -1112,19 +1112,25 @@ static struct mempolicy * get_vma_policy
 		struct vm_area_struct *vma, unsigned long addr)
 {
 	struct mempolicy *pol = task->mempolicy;
-	int shared_pol = 0;
+	int pol_needs_ref = (task != current);
 
 	if (vma) {
 		if (vma->vm_ops && vma->vm_ops->get_policy) {
-			pol = vma->vm_ops->get_policy(vma, addr);
-			shared_pol = 1;	/* if pol non-NULL, add ref below */
+			struct mempolicy *vpol = vma->vm_ops->get_policy(vma,
+									addr);
+			if (vpol) {
+				pol = vpol;
+				pol_needs_ref = 0; /* get_policy() added ref */
+			}
 		} else if (vma->vm_policy &&
-				vma->vm_policy->policy != MPOL_DEFAULT)
+				vma->vm_policy->policy != MPOL_DEFAULT) {
 			pol = vma->vm_policy;
+			pol_needs_ref++;
+		}
 	}
 	if (!pol)
 		pol = &default_policy;
-	else if (!shared_pol && pol != current->mempolicy)
+	else if (pol_needs_ref)
 		mpol_get(pol);	/* vma or other task's policy */
 	return pol;
 }

@@ -1262,18 +1268,21 @@ struct zonelist *huge_zonelist(struct vm
 {
 	struct mempolicy *pol = get_vma_policy(current, vma, addr);
 	struct zonelist *zl;
+	int policy_needs_unref = (pol != &default_policy && \
+					pol != current->mempolicy);
 
 	*mpol = NULL;		/* probably no unref needed */
 	if (pol->policy == MPOL_INTERLEAVE) {
 		unsigned nid;
 
 		nid = interleave_nid(pol, vma, addr, HPAGE_SHIFT);
-		__mpol_free(pol);		/* finished with pol */
+		if (unlikely(policy_needs_unref))
+			__mpol_free(pol);	/* finished with pol */
 		return NODE_DATA(nid)->node_zonelists + gfp_zone(gfp_flags);
 	}
 
 	zl = zonelist_policy(GFP_HIGHUSER, pol);
-	if (unlikely(pol != &default_policy && pol != current->mempolicy)) {
+	if (unlikely(policy_needs_unref)) {
 		if (pol->policy != MPOL_BIND)
 			__mpol_free(pol);	/* finished with pol */
 		else

@@ -1325,6 +1334,9 @@ alloc_page_vma(gfp_t gfp, struct vm_area
 {
 	struct mempolicy *pol = get_vma_policy(current, vma, addr);
 	struct zonelist *zl;
+	int policy_needs_unref = (pol != &default_policy && \
+				pol != current->mempolicy);
+
 
 	cpuset_update_task_memory_state();
 

@@ -1332,10 +1344,12 @@ alloc_page_vma(gfp_t gfp, struct vm_area
 		unsigned nid;
 
 		nid = interleave_nid(pol, vma, addr, PAGE_SHIFT);
+		if (unlikely(policy_needs_unref))
+			__mpol_free(pol);
 		return alloc_page_interleave(gfp, 0, nid);
 	}
 	zl = zonelist_policy(gfp, pol);
-	if (pol != &default_policy && pol != current->mempolicy) {
+	if (unlikely(policy_needs_unref)) {
 		/*
 		 * slow path: ref counted policy -- shared or vma
 		 */

@@ -1444,6 +1458,8 @@ int __mpol_equal(struct mempolicy *a, st
 /* Slow path of a mpol destructor. */
 void __mpol_free(struct mempolicy *p)
 {
+	VM_BUG_ON(p == &default_policy);
+
 	if (!atomic_dec_and_test(&p->refcnt))
 		return;
 	if (p->policy == MPOL_BIND)

Index: Linux/ipc/shm.c
===================================================================
@@ -263,10 +263,10 @@ static struct mempolicy *shm_get_policy(
 
 	if (sfd->vm_ops->get_policy)
 		pol = sfd->vm_ops->get_policy(vma, addr);
-	else if (vma->vm_policy)
+	else if (vma->vm_policy) {
 		pol = vma->vm_policy;
-	else
-		pol = current->mempolicy;
+		mpol_get(pol);		/* get_vma_policy() assumes this */
+	}
 	return pol;
 }
 #endif

Index: Linux/include/linux/mempolicy.h
===================================================================
@@ -2,6 +2,7 @@
 #define _LINUX_MEMPOLICY_H 1
 
 #include <linux/errno.h>
+#include <linux/mm.h>
 
 /*
  * NUMA memory policies for Linux.

@@ -72,6 +73,8 @@ struct mempolicy {
 	nodemask_t cpuset_mems_allowed;	/* mempolicy relative to these nodes */
 };
 
+extern struct mempolicy default_policy;
+
 /*
  * Support for managing mempolicy data objects (clone, copy, destroy)
  * The default fast path of a NULL MPOL_DEFAULT policy is always inlined.

@@ -97,6 +100,7 @@ static inline struct mempolicy *mpol_cop
 
 static inline void mpol_get(struct mempolicy *pol)
 {
+	VM_BUG_ON(pol == &default_policy);
 	if (pol)
 		atomic_inc(&pol->refcnt);
 }

@@ -149,7 +153,6 @@ extern void mpol_rebind_task(struct task
 extern void mpol_rebind_mm(struct mm_struct *mm, nodemask_t *new);
 extern void mpol_fix_fork_child_flag(struct task_struct *p);
 
-extern struct mempolicy default_policy;
 extern struct zonelist *huge_zonelist(struct vm_area_struct *vma,
 		unsigned long addr, gfp_t gfp_flags, struct mempolicy **mpol);
 extern unsigned slab_node(struct mempolicy *policy);

Index: Linux/include/linux/mm.h
===================================================================
@@ -173,7 +173,21 @@ struct vm_operations_struct {
 	 * writable, if an error is returned it will cause a SIGBUS */
 	int (*page_mkwrite)(struct vm_area_struct *vma, struct page *page);
 #ifdef CONFIG_NUMA
+	/*
+	 * set_policy() op must add a reference to any non-NULL @new mempolicy
+	 * to hold the policy upon return.  Caller should pass NULL @new to
+	 * remove a policy and fall back to surrounding context--i.e. do not
+	 * install a MPOL_DEFAULT policy, nor the task or system default
+	 * mempolicy.
+	 */
 	int (*set_policy)(struct vm_area_struct *vma, struct mempolicy *new);
+
+	/*
+	 * get_policy() op must add reference [mpol_get()] to any mempolicy
+	 * at (vma,addr).  If no [shared/vma] mempolicy exists at that addr,
+	 * get_policy() op must return NULL--i.e., do not "fallback" to task
+	 * or system default policy.
+	 */
 	struct mempolicy *(*get_policy)(struct vm_area_struct *vma,
 					unsigned long addr);
 	int (*migrate)(struct vm_area_struct *vma, const nodemask_t *from,

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

Could you add support for SHM_HUGETLB segments instead to make this consistent so that shared policies always use a get_policy function?

Hmmm..... The show_numa_map issue is special. Maybe fix that one instead?

I have patches that do this as part of my shared policy series that is currently "on hold" while we sort these other things out. SHM_HUGETLB segments do use the shm_get_policy() vm_op. However, it detects that the hugetlb shm segment does not support get_policy(), so it just uses the vma policy at that address. You should like this behavior! :-). My patches implement shared policy for SHM_HUGETLB, which you don't like. So, I think we should leave this as is... for now.

Yeah, I know. However, I like the idea of having a separately configurable VM debug check. I will remove the include and the
VM_BUG_ON for now. But, what would [any one else?] think about moving
VM_BUG_ON() to asm-generic/bug.h in a separate patch?

Lee

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

Here's V2 of the second patch of the series, modified as suggested by Christoph vis a vis include of <linux/mm.h> in mempolicy.h. I did not change the hugetlbfs to add *_policy() ops as I think the current mechanisms work as required now. I.e., even if I added the policy vm_ops to hugetlbfs, I think the changes below to mempolicy and shm_get_policy() would still be required in the general case.

I have removed the 'RFC'. Please review for possible merge.

Lee

====
PATCH 2/2 Mempolicy: Fixup Shm and Interleave Policy Reference Counting

Against: 2.6.23-rc8-mm2

V1 -> V2:
+ remove include of <linux/mm.h> from mempolicy.h and use BUG_ON(), conditional on CONFIG_DEBUG_VM, in mpol_get()

In the memory policy reference counting cleanup patch, I missed one path that needs to unreference the memory policy. After computing the target node for interleave policy, we need to drop the reference if the policy is not the system default nor the current task's policy.

In huge_zonelist(), I was unconditionally unref'ing the policy in the interleave path, even when it was a policy that didn't need it. Fix this!

Note: I investigated moving the check for "policy_needs_unref" to the mpol_free() wrapper, but this led to nasty circular header dependencies. If we wanted to make mpol_free() an external function, rather than a static inline, I could do this and remove several checks. I'd still need to keep an explicit check in alloc_page_vma() if we want to use a tail-call for the fast path.

get_vma_policy() assumes that shared policies are referenced by the get_policy() vm_op, if any. This is true for shmem_get_policy() but not for shm_get_policy() when the "backing file" does not support a get_policy() vm_op. The latter is the case for SHM_HUGETLB segments. Because get_vma_policy() expects the get_policy() op to have added a ref, it doesn't do so itself. This results in premature freeing of the policy. Add the mpol_get() to the shm_get_policy() op when the backing file doesn't support shared policies.

Further, shm_get_policy() was falling back to current task's task policy if the backing file did not support get_policy() vm_op and the vma policy was null. This is not valid when get_vma_policy() is called from show_numa_map() as task != current. Also, this did not match the behavior of the shmem_get_policy() vm_op which did NOT fall back to task policy. So, modify shm_get_policy() NOT to fall back to current->mempolicy.

Now, turns out that get_vma_policy() was not handling fallback to task policy correctly when the get_policy() vm_op returns NULL. Rather, it was falling back directly to system default policy. So, fix get_vma_policy() to use only non-NULL policy returned from the vma get_policy op and indicate that this policy does not need another ref count.

Add [VM_]BUG_ON()s to __mpol_free() and mpol_get() to trap attempts to ref/unref the system default policy. Should no longer occur.

Document mempolicy return value reference semantics assumed by the changes discussed above for the set_ and get_policy vm_ops in <linux/mm.h>--where the prototypes are defined.

 include/linux/mempolicy.h |    6 +++++-
 include/linux/mm.h        |   14 ++++++++++++++
 ipc/shm.c                 |    6 +++---
 mm/mempolicy.c            |   32 ++++++++++++++++++++++++--------
 4 files changed, 46 insertions(+), 12 deletions(-)

Index: Linux/mm/mempolicy.c
===================================================================
@@ -1112,19 +1112,25 @@ static struct mempolicy * get_vma_policy
 		struct vm_area_struct *vma, unsigned long addr)
 {
 	struct mempolicy *pol = task->mempolicy;
-	int shared_pol = 0;
+	int pol_needs_ref = (task != current);
 
 	if (vma) {
 		if (vma->vm_ops && vma->vm_ops->get_policy) {
-			pol = vma->vm_ops->get_policy(vma, addr);
-			shared_pol = 1;	/* if pol non-NULL, add ref below */
+			struct mempolicy *vpol = vma->vm_ops->get_policy(vma,
+									addr);
+			if (vpol) {
+				pol = vpol;
+				pol_needs_ref = 0; /* get_policy() added ref */
+			}
 		} else if (vma->vm_policy &&
-				vma->vm_policy->policy != MPOL_DEFAULT)
+				vma->vm_policy->policy != MPOL_DEFAULT) {
 			pol = vma->vm_policy;
+			pol_needs_ref++;
+		}
 	}
 	if (!pol)
 		pol = &default_policy;
-	else if (!shared_pol && pol != current->mempolicy)
+	else if (pol_needs_ref)
 		mpol_get(pol);	/* vma or other task's policy */
 	return pol;
 }

@@ -1262,18 +1268,21 @@ struct zonelist *huge_zonelist(struct vm
 {
 	struct mempolicy *pol = get_vma_policy(current, vma, addr);
 	struct zonelist *zl;
+	int policy_needs_unref = (pol != &default_policy && \
+					pol != current->mempolicy);
 
 	*mpol = NULL;		/* probably no unref needed */
 	if (pol->policy == MPOL_INTERLEAVE) {
 		unsigned nid;
 
 		nid = interleave_nid(pol, vma, addr, HPAGE_SHIFT);
-		__mpol_free(pol);		/* finished with pol */
+		if (unlikely(policy_needs_unref))
+			__mpol_free(pol);	/* finished with pol */
 		return NODE_DATA(nid)->node_zonelists + gfp_zone(gfp_flags);
 	}
 
 	zl = zonelist_policy(GFP_HIGHUSER, pol);
-	if (unlikely(pol != &default_policy && pol != current->mempolicy)) {
+	if (unlikely(policy_needs_unref)) {
 		if (pol->policy != MPOL_BIND)
 			__mpol_free(pol);	/* finished with pol */
 		else

@@ -1325,6 +1334,9 @@ alloc_page_vma(gfp_t gfp, struct vm_area
 {
 	struct mempolicy *pol = get_vma_policy(current, vma, addr);
 	struct zonelist *zl;
+	int policy_needs_unref = (pol != &default_policy && \
+				pol != current->mempolicy);
+
 
 	cpuset_update_task_memory_state();
 

@@ -1332,10 +1344,12 @@ alloc_page_vma(gfp_t gfp, struct vm_area
 		unsigned nid;
 
 		nid = interleave_nid(pol, vma, addr, PAGE_SHIFT);
+		if (unlikely(policy_needs_unref))
+			__mpol_free(pol);
 		return alloc_page_interleave(gfp, 0, nid);
 	}
 	zl = zonelist_policy(gfp, pol);
-	if (pol != &default_policy && pol != current->mempolicy) {
+	if (unlikely(policy_needs_unref)) {
 		/*
 		 * slow path: ref counted policy -- shared or vma
 		 */

@@ -1444,6 +1458,8 @@ int __mpol_equal(struct mempolicy *a, st
 /* Slow path of a mpol destructor. */
 void __mpol_free(struct mempolicy *p)
 {
+	VM_BUG_ON(p == &default_policy);
+
 	if (!atomic_dec_and_test(&p->refcnt))
 		return;
 	if (p->policy == MPOL_BIND)

Index: Linux/ipc/shm.c
===================================================================
@@ -263,10 +263,10 @@ static struct mempolicy *shm_get_policy(
 
 	if (sfd->vm_ops->get_policy)
 		pol = sfd->vm_ops->get_policy(vma, addr);
-	else if (vma->vm_policy)
+	else if (vma->vm_policy) {
 		pol = vma->vm_policy;
-	else
-		pol = current->mempolicy;
+		mpol_get(pol);		/* get_vma_policy() assumes this */
+	}
 	return pol;
 }
 #endif

Index: Linux/include/linux/mempolicy.h
===================================================================
@@ -72,6 +72,8 @@ struct mempolicy {
 	nodemask_t cpuset_mems_allowed;	/* mempolicy relative to these nodes */
 };
 
+extern struct mempolicy default_policy;
+
 /*
  * Support for managing mempolicy data objects (clone, copy, destroy)
  * The default fast path of a NULL MPOL_DEFAULT policy is always inlined.

@@ -97,6 +99,9 @@ static inline struct mempolicy *mpol_cop
 
 static inline void mpol_get(struct mempolicy *pol)
 {
+#ifdef CONFIG_DEBUG_VM
+	BUG_ON(pol == &default_policy);
+#endif
 	if (pol)
 		atomic_inc(&pol->refcnt);
 }

@@ -149,7 +154,6 @@ extern void mpol_rebind_task(struct task
 extern void mpol_rebind_mm(struct mm_struct *mm, nodemask_t *new);
 extern void mpol_fix_fork_child_flag(struct task_struct *p);
 
-extern struct mempolicy default_policy;
 extern struct zonelist *huge_zonelist(struct vm_area_struct *vma,
 		unsigned long addr, gfp_t gfp_flags, struct mempolicy **mpol);
 extern unsigned slab_node(struct mempolicy *policy);

Index: Linux/include/linux/mm.h
===================================================================
@@ -173,7 +173,21 @@ struct vm_operations_struct {
 	 * writable, if an error is returned it will cause a SIGBUS */
 	int (*page_mkwrite)(struct vm_area_struct *vma, struct page *page);
 #ifdef CONFIG_NUMA
+	/*
+	 * set_policy() op must add a reference to any non-NULL @new mempolicy
+	 * to hold the policy upon return.  Caller should pass NULL @new to
+	 * remove a policy and fall back to surrounding context--i.e. do not
+	 * install a MPOL_DEFAULT policy, nor the task or system default
+	 * mempolicy.
+	 */
 	int (*set_policy)(struct vm_area_struct *vma, struct mempolicy *new);
+
+	/*
+	 * get_policy() op must add reference [mpol_get()] to any mempolicy
+	 * at (vma,addr).  If no [shared/vma] mempolicy exists at that addr,
+	 * get_policy() op must return NULL--i.e., do not "fallback" to task
+	 * or system default policy.
+	 */
 	struct mempolicy *(*get_policy)(struct vm_area_struct *vma,
 					unsigned long addr);
 	int (*migrate)(struct vm_area_struct *vma, const nodemask_t *from,

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

I am still concerned with all this special casing which gets very difficult to follow. Isnt there some way to simplify the refcount handling here? It seems that the refcount fix introduced more bugs. One solution would be to revert that patch instead.

Drop the BUG_ON completely? If this is a bug fix release then lets keep this as minimal as possible.

Could you make this a series of separate patches. Each for one issue?

Maybe get_vma_policy() should make no such assumption? Why is get_vma_policy taking a refcount at all? The vma policies are guaranteed based on the process that is running. But what keeps the shared policies from being freed? Isnt there an inherent race here that cannot be remedied by taking a refcount?

get_vma_policy() is passed a pointer to the task struct. It does *not* fall back to the current tasks policy.

Nope. Its falling back to the task policy.

static struct mempolicy * get_vma_policy(struct task_struct *task,
struct vm_area_struct *vma, unsigned long addr)
{

-->     struct mempolicy *pol = task->mempolicy;
        int shared_pol = 0;

        if (vma) {
                if (vma->vm_ops && vma->vm_ops->get_policy) {
                        pol = vma->vm_ops->get_policy(vma, addr);
                        shared_pol = 1; /* if pol non-NULL, add ref below */
                } 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;
}

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

I've tried to remove the special cases by making all [existing] get_policy ops consistent and then documenting the rules on the prototypes for any possible future ops. The basic rule is--don't fall back to task/sysdefault policy--let get_vma_policy() handle that. This is required to get the ref counting right.

If you apply the patch and look at get_vma_policy, you'll see that it will only add a reference count for other task's policy or vma policy. Now, the policy returned by the get_policy() op is essentially a vma policy, but the policy op must add the ref itself 1) to prevent races with other tasks, in the case of shared policy or 2) to be consistent with #1 for SHM_HUGETLB segments which [currently] don't use shared policy.

I try to avoid taking ref on current task's policy and sys default policy because 1) it's not necessary and 2) these should be the vast majority of the cases. In previous discussions with yourself and Andi [back in June/July] you agreed this was the way to go. All my recent changes have been an attempt to do this in the most consistent manner possible.

I could, I guess, and just add it into my tree for testing. But, I fear that, in the future, someone might add some code like I removed from the pci driver and break some other assumptions. The bug on is only compiled when DEBUG_VM is configured. In that case, the overhead in mpol_get() is the least of the additional overhead!

But if you REALLY don't like it [and I agree it's ugly, with the explicit #ifdef], I guess I can remove it. I'd sure like to hear other opinions, tho'

Yeah. I can do that. I'm outta here for a week+ after today and won't get to it until later in the month, as I have a lot of other things to tie off today.

In the meantime, however, if anyone tries to apply a policy [mbind] to a SHM_HUGETLB segment, they will BUG-out on the 2nd page fault with the current upstream [2.6.23] code. Kind of serious I think...

But, the get_policy() vm_op can overwrite 'pol' with a NULL return value. This can happen when you have a real shmem segment with default
policy == NULL/no policy. See below:

^^^ possibly NULL for shmem w/ default policy

^^^ could get here w/ NULL shmem policy and
!NULL task policy. Incorrect fallback.

Patch 2/2 clears all of this up. I think. I did test it, but could have missed something... again.

Lee

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

And even after the fix they will have trouble with cpusets constraints that do not match the mpol bind set?

Ah. The logical fix then is to define an additional temporary variable and only overwrite the pol variable if a policy has been returned? That would be consistent with how the vma policies are handled?

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