Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1621307 > unrolled thread

[RFC 2/6] mm, mempolicy: stop adjusting current->il_next in mpol_rebind_nodemask()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [RFC 2/6] mm, mempolicy: stop adjusting current->il_next in mpol_rebind_nodemask() Vlastimil Babka <vbabka@suse.cz> - 2017-04-11 16:10 +0200
    Re: [RFC 2/6] mm, mempolicy: stop adjusting current->il_next in  mpol_rebind_nodemask() Christoph Lameter <cl@linux.com> - 2017-04-11 19:40 +0200
      Re: [RFC 2/6] mm, mempolicy: stop adjusting current->il_next in  mpol_rebind_nodemask() Vlastimil Babka <vbabka@suse.cz> - 2017-04-11 21:10 +0200
        Re: [RFC 2/6] mm, mempolicy: stop adjusting current->il_next in  mpol_rebind_nodemask() Vlastimil Babka <vbabka@suse.cz> - 2017-04-12 10:50 +0200
          Re: [RFC 2/6] mm, mempolicy: stop adjusting current->il_next in  mpol_rebind_nodemask() Vlastimil Babka <vbabka@suse.cz> - 2017-04-12 23:20 +0200
          Re: [RFC 2/6] mm, mempolicy: stop adjusting current->il_next in  mpol_rebind_nodemask() Christoph Lameter <cl@linux.com> - 2017-04-12 23:20 +0200

#1621307 — [RFC 2/6] mm, mempolicy: stop adjusting current->il_next in mpol_rebind_nodemask()

The task->il_next variable remembers the last allocation node for task's
MPOL_INTERLEAVE policy. mpol_rebind_nodemask() updates interleave and
bind mempolicies due to changing cpuset mems. Currently it also tries to
make sure that current->il_next is valid within the updated nodemask. This is
bogus, because 1) we are updating potentially any task's mempolicy, not just
current, and 2) we might be updating per-vma mempolicy, not task one.

The interleave_nodes() function that uses il_next can cope fine with the value
not being within the currently allowed nodes, so this hasn't manifested as an
actual issue. Thus it also won't be an issue if we just remove this adjustment
completely.

Signed-off-by: Vlastimil Babka <vbabka@suse.cz>
---
 mm/mempolicy.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 37d0b334bfe9..efeec8d2bce5 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -349,12 +349,6 @@ static void mpol_rebind_nodemask(struct mempolicy *pol, const nodemask_t *nodes,
 		pol->v.nodes = tmp;
 	else
 		BUG();
-
-	if (!node_isset(current->il_next, tmp)) {
-		current->il_next = next_node_in(current->il_next, tmp);
-		if (current->il_next >= MAX_NUMNODES)
-			current->il_next = numa_node_id();
-	}
 }
 
 static void mpol_rebind_preferred(struct mempolicy *pol,
-- 
2.12.2

[toc] | [next] | [standalone]

#1621500 — Re: [RFC 2/6] mm, mempolicy: stop adjusting current->il_next in mpol_rebind_nodemask()

On Tue, 11 Apr 2017, Vlastimil Babka wrote:

> The task->il_next variable remembers the last allocation node for task's
> MPOL_INTERLEAVE policy. mpol_rebind_nodemask() updates interleave and
> bind mempolicies due to changing cpuset mems. Currently it also tries to
> make sure that current->il_next is valid within the updated nodemask. This is
> bogus, because 1) we are updating potentially any task's mempolicy, not just
> current, and 2) we might be updating per-vma mempolicy, not task one.
>
> The interleave_nodes() function that uses il_next can cope fine with the value
> not being within the currently allowed nodes, so this hasn't manifested as an
> actual issue. Thus it also won't be an issue if we just remove this adjustment
> completely.

Well, interleave_nodes() will then potentially return a node outside of
the allowed memory policy when its called for the first time after
mpol_rebind_.. . But thenn it will find the next node within the
nodemask and work correctly for the next invocations.

But yea the race can probably be ignored. The idea was that the
application has a stable memory footprint during rebinding.

[toc] | [prev] | [next] | [standalone]

#1621565 — Re: [RFC 2/6] mm, mempolicy: stop adjusting current->il_next in mpol_rebind_nodemask()

On 11.4.2017 19:32, Christoph Lameter wrote:
> On Tue, 11 Apr 2017, Vlastimil Babka wrote:
> 
>> The task->il_next variable remembers the last allocation node for task's
>> MPOL_INTERLEAVE policy. mpol_rebind_nodemask() updates interleave and
>> bind mempolicies due to changing cpuset mems. Currently it also tries to
>> make sure that current->il_next is valid within the updated nodemask. This is
>> bogus, because 1) we are updating potentially any task's mempolicy, not just
>> current, and 2) we might be updating per-vma mempolicy, not task one.
>>
>> The interleave_nodes() function that uses il_next can cope fine with the value
>> not being within the currently allowed nodes, so this hasn't manifested as an
>> actual issue. Thus it also won't be an issue if we just remove this adjustment
>> completely.
> 
> Well, interleave_nodes() will then potentially return a node outside of
> the allowed memory policy when its called for the first time after
> mpol_rebind_.. . But thenn it will find the next node within the
> nodemask and work correctly for the next invocations.

Hmm, you're right. But that could be easily fixed if il_next became il_prev, so
we would return the result of next_node_in(il_prev) and also store it as the new
il_prev, right? I somehow assumed it already worked that way.

> But yea the race can probably be ignored. The idea was that the
> application has a stable memory footprint during rebinding.

[toc] | [prev] | [next] | [standalone]

#1621932 — Re: [RFC 2/6] mm, mempolicy: stop adjusting current->il_next in mpol_rebind_nodemask()

On 04/11/2017 09:03 PM, Vlastimil Babka wrote:
> On 11.4.2017 19:32, Christoph Lameter wrote:
>> On Tue, 11 Apr 2017, Vlastimil Babka wrote:
>>
>>> The task->il_next variable remembers the last allocation node for task's
>>> MPOL_INTERLEAVE policy. mpol_rebind_nodemask() updates interleave and
>>> bind mempolicies due to changing cpuset mems. Currently it also tries to
>>> make sure that current->il_next is valid within the updated nodemask. This is
>>> bogus, because 1) we are updating potentially any task's mempolicy, not just
>>> current, and 2) we might be updating per-vma mempolicy, not task one.
>>>
>>> The interleave_nodes() function that uses il_next can cope fine with the value
>>> not being within the currently allowed nodes, so this hasn't manifested as an
>>> actual issue. Thus it also won't be an issue if we just remove this adjustment
>>> completely.
>>
>> Well, interleave_nodes() will then potentially return a node outside of
>> the allowed memory policy when its called for the first time after
>> mpol_rebind_.. . But thenn it will find the next node within the
>> nodemask and work correctly for the next invocations.
> 
> Hmm, you're right. But that could be easily fixed if il_next became il_prev, so
> we would return the result of next_node_in(il_prev) and also store it as the new
> il_prev, right? I somehow assumed it already worked that way.

Like this?
----8<----
commit 0ec64a0b8e614ea655328d0fb539447c407ba7c1
Author: Vlastimil Babka <vbabka@suse.cz>
Date:   Mon Apr 3 13:11:32 2017 +0200

    mm, mempolicy: stop adjusting current->il_next in mpol_rebind_nodemask()
    
    The task->il_next variable stores the next allocation node id for task's
    MPOL_INTERLEAVE policy. mpol_rebind_nodemask() updates interleave and
    bind mempolicies due to changing cpuset mems. Currently it also tries to
    make sure that current->il_next is valid within the updated nodemask. This is
    bogus, because 1) we are updating potentially any task's mempolicy, not just
    current, and 2) we might be updating a per-vma mempolicy, not task one.
    
    The interleave_nodes() function that uses il_next can cope fine with the value
    not being within the currently allowed nodes, so this hasn't manifested as an
    actual issue.
    
    We can remove the need for updating il_next completely by changing it to
    il_prev and store the node id of the previous interleave allocation instead of
    the next id. Then interleave_nodes() can calculate the next id using the
    current nodemask and also store it as il_prev, except when querying the next
    node via do_get_mempolicy().
    
    Signed-off-by: Vlastimil Babka <vbabka@suse.cz>

diff --git a/include/linux/sched.h b/include/linux/sched.h
index 050d7113924a..9aca0db1e588 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -886,7 +886,7 @@ struct task_struct {
 #ifdef CONFIG_NUMA
 	/* Protected by alloc_lock: */
 	struct mempolicy		*mempolicy;
-	short				il_next;
+	short				il_prev;
 	short				pref_node_fork;
 #endif
 #ifdef CONFIG_NUMA_BALANCING
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 37d0b334bfe9..25f9bde58521 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -349,12 +349,6 @@ static void mpol_rebind_nodemask(struct mempolicy *pol, const nodemask_t *nodes,
 		pol->v.nodes = tmp;
 	else
 		BUG();
-
-	if (!node_isset(current->il_next, tmp)) {
-		current->il_next = next_node_in(current->il_next, tmp);
-		if (current->il_next >= MAX_NUMNODES)
-			current->il_next = numa_node_id();
-	}
 }
 
 static void mpol_rebind_preferred(struct mempolicy *pol,
@@ -812,9 +806,8 @@ static long do_set_mempolicy(unsigned short mode, unsigned short flags,
 	}
 	old = current->mempolicy;
 	current->mempolicy = new;
-	if (new && new->mode == MPOL_INTERLEAVE &&
-	    nodes_weight(new->v.nodes))
-		current->il_next = first_node(new->v.nodes);
+	if (new && new->mode == MPOL_INTERLEAVE)
+		current->il_prev = MAX_NUMNODES-1;
 	task_unlock(current);
 	mpol_put(old);
 	ret = 0;
@@ -863,6 +856,18 @@ static int lookup_node(unsigned long addr)
 	return err;
 }
 
+/* Do dynamic interleaving for a process */
+static unsigned interleave_nodes(struct mempolicy *policy, bool update_prev)
+{
+	unsigned next;
+	struct task_struct *me = current;
+
+	next = next_node_in(me->il_prev, policy->v.nodes);
+	if (next < MAX_NUMNODES && update_prev)
+		me->il_prev = next;
+	return next;
+}
+
 /* Retrieve NUMA policy */
 static long do_get_mempolicy(int *policy, nodemask_t *nmask,
 			     unsigned long addr, unsigned long flags)
@@ -916,7 +921,7 @@ static long do_get_mempolicy(int *policy, nodemask_t *nmask,
 			*policy = err;
 		} else if (pol == current->mempolicy &&
 				pol->mode == MPOL_INTERLEAVE) {
-			*policy = current->il_next;
+			*policy = interleave_nodes(current->mempolicy, false);
 		} else {
 			err = -EINVAL;
 			goto out;
@@ -1694,19 +1699,6 @@ static struct zonelist *policy_zonelist(gfp_t gfp, struct mempolicy *policy,
 	return node_zonelist(nd, gfp);
 }
 
-/* Do dynamic interleaving for a process */
-static unsigned interleave_nodes(struct mempolicy *policy)
-{
-	unsigned nid, next;
-	struct task_struct *me = current;
-
-	nid = me->il_next;
-	next = next_node_in(nid, policy->v.nodes);
-	if (next < MAX_NUMNODES)
-		me->il_next = next;
-	return nid;
-}
-
 /*
  * Depending on the memory policy provide a node from which to allocate the
  * next slab entry.
@@ -1731,7 +1723,7 @@ unsigned int mempolicy_slab_node(void)
 		return policy->v.preferred_node;
 
 	case MPOL_INTERLEAVE:
-		return interleave_nodes(policy);
+		return interleave_nodes(policy, true);
 
 	case MPOL_BIND: {
 		struct zoneref *z;
@@ -1794,7 +1786,7 @@ static inline unsigned interleave_nid(struct mempolicy *pol,
 		off += (addr - vma->vm_start) >> shift;
 		return offset_il_node(pol, vma, off);
 	} else
-		return interleave_nodes(pol);
+		return interleave_nodes(pol, true);
 }
 
 #ifdef CONFIG_HUGETLBFS
@@ -2060,7 +2052,8 @@ struct page *alloc_pages_current(gfp_t gfp, unsigned order)
 	 * nor system default_policy
 	 */
 	if (pol->mode == MPOL_INTERLEAVE)
-		page = alloc_page_interleave(gfp, order, interleave_nodes(pol));
+		page = alloc_page_interleave(gfp, order,
+				interleave_nodes(pol, true));
 	else
 		page = __alloc_pages_nodemask(gfp, order,
 				policy_zonelist(gfp, pol, numa_node_id()),

[toc] | [prev] | [next] | [standalone]

#1622560 — Re: [RFC 2/6] mm, mempolicy: stop adjusting current->il_next in mpol_rebind_nodemask()

On 12.4.2017 23:16, Christoph Lameter wrote:
> On Wed, 12 Apr 2017, Vlastimil Babka wrote:
> 
>>>> Well, interleave_nodes() will then potentially return a node outside of
>>>> the allowed memory policy when its called for the first time after
>>>> mpol_rebind_.. . But thenn it will find the next node within the
>>>> nodemask and work correctly for the next invocations.
>>>
>>> Hmm, you're right. But that could be easily fixed if il_next became il_prev, so
>>> we would return the result of next_node_in(il_prev) and also store it as the new
>>> il_prev, right? I somehow assumed it already worked that way.
> 
> Yup that makes sense and I thought about that when I saw the problem too.
> 
>> @@ -863,6 +856,18 @@ static int lookup_node(unsigned long addr)
>>  	return err;
>>  }
>>
>> +/* Do dynamic interleaving for a process */
>> +static unsigned interleave_nodes(struct mempolicy *policy, bool update_prev)
> 
> Why do you need an additional flag? Would it not be better to always
> update and switch the update_prev=false case to simply use
> next_node_in()?

Looked to me as better wrapping, but probably overengineered, ok. Will change
for v2.

[toc] | [prev] | [next] | [standalone]

#1622561 — Re: [RFC 2/6] mm, mempolicy: stop adjusting current->il_next in mpol_rebind_nodemask()

On Wed, 12 Apr 2017, Vlastimil Babka wrote:

> >> Well, interleave_nodes() will then potentially return a node outside of
> >> the allowed memory policy when its called for the first time after
> >> mpol_rebind_.. . But thenn it will find the next node within the
> >> nodemask and work correctly for the next invocations.
> >
> > Hmm, you're right. But that could be easily fixed if il_next became il_prev, so
> > we would return the result of next_node_in(il_prev) and also store it as the new
> > il_prev, right? I somehow assumed it already worked that way.

Yup that makes sense and I thought about that when I saw the problem too.

> @@ -863,6 +856,18 @@ static int lookup_node(unsigned long addr)
>  	return err;
>  }
>
> +/* Do dynamic interleaving for a process */
> +static unsigned interleave_nodes(struct mempolicy *policy, bool update_prev)

Why do you need an additional flag? Would it not be better to always
update and switch the update_prev=false case to simply use
next_node_in()?

> +{
> +	unsigned next;
> +	struct task_struct *me = current;
> +
> +	next = next_node_in(me->il_prev, policy->v.nodes);
> +	if (next < MAX_NUMNODES && update_prev)
> +		me->il_prev = next;
> +	return next;
> +}
> +
>  /* Retrieve NUMA policy */
>  static long do_get_mempolicy(int *policy, nodemask_t *nmask,
>  			     unsigned long addr, unsigned long flags)
> @@ -916,7 +921,7 @@ static long do_get_mempolicy(int *policy, nodemask_t *nmask,
>  			*policy = err;
>  		} else if (pol == current->mempolicy &&
>  				pol->mode == MPOL_INTERLEAVE) {
> -			*policy = current->il_next;
> +			*policy = interleave_nodes(current->mempolicy, false);

Here

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web