Re: [PATCH] Mempolicy: make dequeue_huge_page_vma() obey MPOL_BIND nodemask rework

[Nishanth Aravamudan <nacc@us.ibm.com>]

On 06.03.2008 [16:24:53 -0500], Lee Schermerhorn wrote:
> 
> Fix for earlier patch:
> "mempolicy-make-dequeue_huge_page_vma-obey-bind-policy"
> 
> Against: 2.6.25-rc3-mm1 atop the above patch.
> 
> As suggested by Nish Aravamudan, remove the mpol_bind_nodemask()
> helper and return a pointer to the policy node mask from
> huge_zonelist for MPOL_BIND.  This hides more of the mempolicy
> quirks from hugetlb.
> 
> In making this change, I noticed that the huge_zonelist() stub
> for !NUMA wasn't nulling out the mpol.  Added that as well.

Hrm, I was thinking more of the following (on top of this patch):

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 4c5d41d..3790f5a 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -1298,9 +1298,7 @@ struct zonelist *huge_zonelist(struct vm_area_struct *vma, unsigned long addr,
 
 	*mpol = NULL;		/* probably no unref needed */
 	*nodemask = NULL;	/* assume !MPOL_BIND */
-	if (pol->policy == MPOL_BIND) {
-			*nodemask = &pol->v.nodes;
-	} else if (pol->policy == MPOL_INTERLEAVE) {
+	if (pol->policy == MPOL_INTERLEAVE) {
 		unsigned nid;
 
 		nid = interleave_nid(pol, vma, addr, HPAGE_SHIFT);
@@ -1310,10 +1308,12 @@ struct zonelist *huge_zonelist(struct vm_area_struct *vma, unsigned long addr,
 
 	zl = zonelist_policy(GFP_HIGHUSER, pol);
 	if (unlikely(pol != &default_policy && pol != current->mempolicy)) {
-		if (pol->policy != MPOL_BIND)
+		if (pol->policy != MPOL_BIND) {
 			__mpol_free(pol);	/* finished with pol */
-		else
+		} else {
 			*mpol = pol;	/* unref needed after allocation */
+			*nodemask = &pol->v.nodes;
+		}
 	}
 	return zl;
 }

but perhaps that won't do the right thing if pol == current->mempolicy
and pol->policy == MPOL_BIND. So something like:


diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 4c5d41d..7eb77e0 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -1298,9 +1298,7 @@ struct zonelist *huge_zonelist(struct vm_area_struct *vma, unsigned long addr,
 
 	*mpol = NULL;		/* probably no unref needed */
 	*nodemask = NULL;	/* assume !MPOL_BIND */
-	if (pol->policy == MPOL_BIND) {
-			*nodemask = &pol->v.nodes;
-	} else if (pol->policy == MPOL_INTERLEAVE) {
+	if (pol->policy == MPOL_INTERLEAVE) {
 		unsigned nid;
 
 		nid = interleave_nid(pol, vma, addr, HPAGE_SHIFT);
@@ -1309,11 +1307,12 @@ struct zonelist *huge_zonelist(struct vm_area_struct *vma, unsigned long addr,
 	}
 
 	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
+	if (unlikely(pol != &default_policy && pol != current->mempolicy
+						&& pol->policy != MPOL_BIND))
+		__mpol_free(pol);	/* finished with pol */
+	if (pol->policy == MPOL_BIND) {
 			*mpol = pol;	/* unref needed after allocation */
+			*nodemask = &pol->v.nodes;
 	}
 	return zl;
 }

Still not quite as clean, but I think it's best to keep the *mpol and
*nodemask assignments together, as if *mpol is being assigned, that's
the only time we should need to set *nodemask, right?

Thanks,
Nish

-- 
Nishanth Aravamudan <nacc@us.ibm.com>
IBM Linux Technology Center

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