In the prior patch, we carry only the current weight for a weighted
interleave round with us across calls through the allocator path.

node = next_node_in(current->il_prev, pol->nodemask)
pol->cur_il_weight <--- this weight applies to the above node

This separation of data can cause a race condition.

If a cgroup-initiated task migration or mems_allowed change occurs
from outside the context of the task, this can cause the weight to
become stale, meaning we may end using that weight to allocate
memory on the wrong node.

Example:
  1) task A sets (cur_il_weight = 8) and (current->il_prev) to
     node0. node1 is the next set bit in pol->nodemask
  2) rebind event occurs, removing node1 from the nodemask.
     node2 is now the next set bit in pol->nodemask
     cur_il_weight is now stale.
  3) allocation occurs, next_node_in(il_prev, nodes) returns
     node2. cur_il_weight is now applied to the wrong node.

The upper level allocator logic must still enforce mems_allowed,
so this isn't dangerous, but it is innaccurate.

Just clearing the weight is insufficient, as it creates two more
race conditions.  The root of the issue is the separation of weight
and node data between nodemask and cur_il_weight.

To solve this, update cur_il_weight to be an atomic_t, and place the
node that the weight applies to in the upper bits of the field:

atomic_t cur_il_weight
	node bits 32:8
	weight bits 7:0

Now retrieving or clearing the active interleave node and weight
is a single atomic operation, and we are not dependent on the
potentially changing state of (pol->nodemask) to determine what
node the weight applies to.

Two special observations:
- if the weight is non-zero, cur_il_weight must *always* have a
  valid node number, e.g. it cannot be NUMA_NO_NODE (-1).
  This is because we steal the top byte for the weight.

- MAX_NUMNODES is presently limited to 1024 or less on every
  architecture. This would permanently limit MAX_NUMNODES to
  an absolute maximum of (1 << 24) to avoid overflows.

Per some reading and discussion, it appears that max nodes is
limited to 1024 so that zone type still fits in page flags, so
this method seemed preferable compared to the alternatives of
trying to make all or part of mempolicy RCU protected (which
may not be possible, since it is often referenced during code
chunks which call operations that may sleep).

Signed-off-by: Gregory Price <gregory.price@memverge.com>
---
 include/linux/mempolicy.h |  2 +-
 mm/mempolicy.c            | 93 +++++++++++++++++++++++++--------------
 2 files changed, 61 insertions(+), 34 deletions(-)

diff --git a/include/linux/mempolicy.h b/include/linux/mempolicy.h
index c644d7bbd396..8108fc6e96ca 100644
--- a/include/linux/mempolicy.h
+++ b/include/linux/mempolicy.h
@@ -56,7 +56,7 @@ struct mempolicy {
 	} w;
 
 	/* Weighted interleave settings */
-	u8 cur_il_weight;
+	atomic_t cur_il_weight;
 };
 
 /*
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 5a517511658e..41b5fef0a6f5 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -321,7 +321,7 @@ static struct mempolicy *mpol_new(unsigned short mode, unsigned short flags,
 	policy->mode = mode;
 	policy->flags = flags;
 	policy->home_node = NUMA_NO_NODE;
-	policy->cur_il_weight = 0;
+	atomic_set(&policy->cur_il_weight, 0);
 
 	return policy;
 }
@@ -356,6 +356,7 @@ static void mpol_rebind_nodemask(struct mempolicy *pol, const nodemask_t *nodes)
 		tmp = *nodes;
 
 	pol->nodes = tmp;
+	atomic_set(&pol->cur_il_weight, 0);
 }
 
 static void mpol_rebind_preferred(struct mempolicy *pol,
@@ -973,8 +974,10 @@ static long do_get_mempolicy(int *policy, nodemask_t *nmask,
 			*policy = next_node_in(current->il_prev, pol->nodes);
 		} else if (pol == current->mempolicy &&
 				(pol->mode == MPOL_WEIGHTED_INTERLEAVE)) {
-			if (pol->cur_il_weight)
-				*policy = current->il_prev;
+			int cweight = atomic_read(&pol->cur_il_weight);
+
+			if (cweight & 0xFF)
+				*policy = cweight >> 8;
 			else
 				*policy = next_node_in(current->il_prev,
 						       pol->nodes);
@@ -1864,36 +1867,48 @@ static unsigned int weighted_interleave_nodes(struct mempolicy *policy)
 	unsigned int node, next;
 	struct task_struct *me = current;
 	u8 __rcu *table;
+	int cur_weight;
 	u8 weight;
 
-	node = next_node_in(me->il_prev, policy->nodes);
-	if (node == MAX_NUMNODES)
-		return node;
+	cur_weight = atomic_read(&policy->cur_il_weight);
+	node = cur_weight >> 8;
+	weight = cur_weight & 0xff;
 
-	/* on first alloc after setting mempolicy, acquire first weight */
-	if (unlikely(!policy->cur_il_weight)) {
+	/* If nodemask was rebound, just fetch the next node */
+	if (!weight || !node_isset(node, policy->nodes)) {
+		node = next_node_in(me->il_prev, policy->nodes);
+		/* can only happen if nodemask has become invalid */
+		if (node == MAX_NUMNODES)
+			return node;
 		rcu_read_lock();
 		table = rcu_dereference(iw_table);
 		/* detect system-default values */
 		weight = table ? table[node] : 1;
-		policy->cur_il_weight = weight ? weight : 1;
+		weight = weight ? weight : 1;
 		rcu_read_unlock();
 	}
 
 	/* account for this allocation call */
-	policy->cur_il_weight--;
+	weight--;
 
 	/* if now at 0, move to next node and set up that node's weight */
-	if (unlikely(!policy->cur_il_weight)) {
+	if (unlikely(!weight)) {
 		me->il_prev = node;
 		next = next_node_in(node, policy->nodes);
-		rcu_read_lock();
-		table = rcu_dereference(iw_table);
-		/* detect system-default values */
-		weight = table ? table[next] : 1;
-		policy->cur_il_weight = weight ? weight : 1;
-		rcu_read_unlock();
-	}
+		if (next != MAX_NUMNODES) {
+			rcu_read_lock();
+			table = rcu_dereference(iw_table);
+			/* detect system-default values */
+			weight = table ? table[next] : 1;
+			weight = weight ? weight : 1;
+			rcu_read_unlock();
+			cur_weight = (next << 8) | weight;
+		} else /* policy->nodes became invalid */
+			cur_weight = 0;
+	} else
+		cur_weight = (node << 8) | weight;
+
+	atomic_set(&policy->cur_il_weight, cur_weight);
 	return node;
 }
 
@@ -2385,6 +2400,7 @@ static unsigned long alloc_pages_bulk_array_weighted_interleave(gfp_t gfp,
 	nodemask_t nodes;
 	int nnodes, node, resume_node, next_node;
 	int prev_node = me->il_prev;
+	int cur_node_and_weight = atomic_read(&pol->cur_il_weight);
 	int i;
 
 	if (!nr_pages)
@@ -2394,10 +2410,11 @@ static unsigned long alloc_pages_bulk_array_weighted_interleave(gfp_t gfp,
 	if (!nnodes)
 		return 0;
 
+	node = cur_node_and_weight >> 8;
+	weight = cur_node_and_weight & 0xff;
 	/* Continue allocating from most recent node and adjust the nr_pages */
-	if (pol->cur_il_weight) {
-		node = next_node_in(prev_node, nodes);
-		node_pages = pol->cur_il_weight;
+	if (weight && node_isset(node, nodes)) {
+		node_pages = weight;
 		if (node_pages > rem_pages)
 			node_pages = rem_pages;
 		nr_allocated = __alloc_pages_bulk(gfp, node, NULL, node_pages,
@@ -2408,27 +2425,36 @@ static unsigned long alloc_pages_bulk_array_weighted_interleave(gfp_t gfp,
 		 * if that's all the pages, no need to interleave, otherwise
 		 * we need to set up the next interleave node/weight correctly.
 		 */
-		if (rem_pages < pol->cur_il_weight) {
+		if (rem_pages < weight) {
 			/* stay on current node, adjust cur_il_weight */
-			pol->cur_il_weight -= rem_pages;
+			weight -= rem_pages;
+			atomic_set(&pol->cur_il_weight, ((node << 8) | weight));
 			return total_allocated;
-		} else if (rem_pages == pol->cur_il_weight) {
+		} else if (rem_pages == weight) {
 			/* move to next node / weight */
 			me->il_prev = node;
 			next_node = next_node_in(node, nodes);
-			rcu_read_lock();
-			table = rcu_dereference(iw_table);
-			weight = table ? table[next_node] : 1;
-			/* detect system-default usage */
-			pol->cur_il_weight = weight ? weight : 1;
-			rcu_read_unlock();
+			if (next_node == MAX_NUMNODES) {
+				next_node = 0;
+				weight = 0;
+			} else {
+				rcu_read_lock();
+				table = rcu_dereference(iw_table);
+				weight = table ? table[next_node] : 1;
+				/* detect system-default usage */
+				weight = weight ? weight : 1;
+				rcu_read_unlock();
+			}
+			atomic_set(&pol->cur_il_weight,
+				   ((next_node << 8) | weight));
 			return total_allocated;
 		}
 		/* Otherwise we adjust nr_pages down, and continue from there */
-		rem_pages -= pol->cur_il_weight;
-		pol->cur_il_weight = 0;
+		rem_pages -= weight;
 		prev_node = node;
 	}
+	/* clear cur_il_weight in case of an allocation failure */
+	atomic_set(&pol->cur_il_weight, 0);
 
 	/* create a local copy of node weights to operate on outside rcu */
 	weights = kmalloc(nr_node_ids, GFP_KERNEL);
@@ -2513,7 +2539,8 @@ static unsigned long alloc_pages_bulk_array_weighted_interleave(gfp_t gfp,
 	}
 	/* resume allocating from the calculated node and weight */
 	me->il_prev = resume_node;
-	pol->cur_il_weight = resume_weight;
+	resume_node = next_node_in(resume_node, nodes);
+	atomic_set(&pol->cur_il_weight, ((resume_node << 8) | resume_weight));
 	kfree(weights);
 	return total_allocated;
 }
-- 
2.39.1

Gregory Price <gourry.memverge@gmail.com> writes:

> In the prior patch, we carry only the current weight for a weighted
> interleave round with us across calls through the allocator path.
>
> node = next_node_in(current->il_prev, pol->nodemask)
> pol->cur_il_weight <--- this weight applies to the above node
>
> This separation of data can cause a race condition.
>
> If a cgroup-initiated task migration or mems_allowed change occurs
> from outside the context of the task, this can cause the weight to
> become stale, meaning we may end using that weight to allocate
> memory on the wrong node.
>
> Example:
>   1) task A sets (cur_il_weight = 8) and (current->il_prev) to
>      node0. node1 is the next set bit in pol->nodemask
>   2) rebind event occurs, removing node1 from the nodemask.
>      node2 is now the next set bit in pol->nodemask
>      cur_il_weight is now stale.
>   3) allocation occurs, next_node_in(il_prev, nodes) returns
>      node2. cur_il_weight is now applied to the wrong node.
>
> The upper level allocator logic must still enforce mems_allowed,
> so this isn't dangerous, but it is innaccurate.
>
> Just clearing the weight is insufficient, as it creates two more
> race conditions.  The root of the issue is the separation of weight
> and node data between nodemask and cur_il_weight.
>
> To solve this, update cur_il_weight to be an atomic_t, and place the
> node that the weight applies to in the upper bits of the field:
>
> atomic_t cur_il_weight
> 	node bits 32:8
> 	weight bits 7:0
>
> Now retrieving or clearing the active interleave node and weight
> is a single atomic operation, and we are not dependent on the
> potentially changing state of (pol->nodemask) to determine what
> node the weight applies to.
>
> Two special observations:
> - if the weight is non-zero, cur_il_weight must *always* have a
>   valid node number, e.g. it cannot be NUMA_NO_NODE (-1).

IIUC, we don't need that, "MAX_NUMNODES-1" is used instead.

>   This is because we steal the top byte for the weight.
>
> - MAX_NUMNODES is presently limited to 1024 or less on every
>   architecture. This would permanently limit MAX_NUMNODES to
>   an absolute maximum of (1 << 24) to avoid overflows.
>
> Per some reading and discussion, it appears that max nodes is
> limited to 1024 so that zone type still fits in page flags, so
> this method seemed preferable compared to the alternatives of
> trying to make all or part of mempolicy RCU protected (which
> may not be possible, since it is often referenced during code
> chunks which call operations that may sleep).
>
> Signed-off-by: Gregory Price <gregory.price@memverge.com>
> ---
>  include/linux/mempolicy.h |  2 +-
>  mm/mempolicy.c            | 93 +++++++++++++++++++++++++--------------
>  2 files changed, 61 insertions(+), 34 deletions(-)
>
> diff --git a/include/linux/mempolicy.h b/include/linux/mempolicy.h
> index c644d7bbd396..8108fc6e96ca 100644
> --- a/include/linux/mempolicy.h
> +++ b/include/linux/mempolicy.h
> @@ -56,7 +56,7 @@ struct mempolicy {
>  	} w;
>  
>  	/* Weighted interleave settings */
> -	u8 cur_il_weight;
> +	atomic_t cur_il_weight;

If we use this field for node and weight, why not change the field name?
For example, cur_wil_node_weight.

>  };
>  
>  /*
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index 5a517511658e..41b5fef0a6f5 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -321,7 +321,7 @@ static struct mempolicy *mpol_new(unsigned short mode, unsigned short flags,
>  	policy->mode = mode;
>  	policy->flags = flags;
>  	policy->home_node = NUMA_NO_NODE;
> -	policy->cur_il_weight = 0;
> +	atomic_set(&policy->cur_il_weight, 0);
>  
>  	return policy;
>  }
> @@ -356,6 +356,7 @@ static void mpol_rebind_nodemask(struct mempolicy *pol, const nodemask_t *nodes)
>  		tmp = *nodes;
>  
>  	pol->nodes = tmp;
> +	atomic_set(&pol->cur_il_weight, 0);
>  }
>  
>  static void mpol_rebind_preferred(struct mempolicy *pol,
> @@ -973,8 +974,10 @@ static long do_get_mempolicy(int *policy, nodemask_t *nmask,
>  			*policy = next_node_in(current->il_prev, pol->nodes);
>  		} else if (pol == current->mempolicy &&
>  				(pol->mode == MPOL_WEIGHTED_INTERLEAVE)) {
> -			if (pol->cur_il_weight)
> -				*policy = current->il_prev;
> +			int cweight = atomic_read(&pol->cur_il_weight);
> +
> +			if (cweight & 0xFF)
> +				*policy = cweight >> 8;

Please define some helper functions or macros instead of operate on bits
directly.

>  			else
>  				*policy = next_node_in(current->il_prev,
>  						       pol->nodes);

If we record current node in pol->cur_il_weight, why do we still need
curren->il_prev.  Can we only use pol->cur_il_weight?  And if so, we can
even make current->il_prev a union.

--
Best Regards,
Huang, Ying

[snip]

On Fri, Jan 26, 2024 at 03:40:27PM +0800, Huang, Ying wrote:
> Gregory Price <gourry.memverge@gmail.com> writes:
> 
> > Two special observations:
> > - if the weight is non-zero, cur_il_weight must *always* have a
> >   valid node number, e.g. it cannot be NUMA_NO_NODE (-1).
> 
> IIUC, we don't need that, "MAX_NUMNODES-1" is used instead.
> 

Correct, I just thought it pertinent to call this out explicitly since
I'm stealing the top byte, but the node value has traditionally been a
full integer.

This may be relevant should anyone try to carry, a random node value
into this field. For example, if someone tried to copy policy->home_node
into cur_il_weight for whatever reason.

It's worth breaking out a function to defend against this - plus to hide
the bit operations directly as you recommend below.

> >  	/* Weighted interleave settings */
> > -	u8 cur_il_weight;
> > +	atomic_t cur_il_weight;
> 
> If we use this field for node and weight, why not change the field name?
> For example, cur_wil_node_weight.
> 

ack.

> > +			if (cweight & 0xFF)
> > +				*policy = cweight >> 8;
> 
> Please define some helper functions or macros instead of operate on bits
> directly.
> 

ack.

> >  			else
> >  				*policy = next_node_in(current->il_prev,
> >  						       pol->nodes);
> 
> If we record current node in pol->cur_il_weight, why do we still need
> curren->il_prev.  Can we only use pol->cur_il_weight?  And if so, we can
> even make current->il_prev a union.
> 

I just realized that there's a problem here for shared memory policies.

from weighted_interleave_nodes, I do this:

cur_weight = atomic_read(&policy->cur_il_weight);
...
weight--;
...
atomic_set(&policy->cur_il_weight, cur_weight);

On a shared memory policy, this is a race condition.


I don't think we can combine il_prev and cur_wil_node_weight because
the task policy may be different than the current policy.

i.e. it's totally valid to do the following:

1) set_mempolicy(MPOL_INTERLEAVE)
2) mbind(..., MPOL_WEIGHTED_INTERLEAVE)

Using current->il_prev between these two policies, is just plain incorrect,
so I will need to rethink this, and the existing code will need to be
updated such that weighted_interleave does not use current->il_prev.

~Gregory

Gregory Price <gregory.price@memverge.com> writes:

> On Fri, Jan 26, 2024 at 03:40:27PM +0800, Huang, Ying wrote:
>> Gregory Price <gourry.memverge@gmail.com> writes:
>> 
>> > Two special observations:
>> > - if the weight is non-zero, cur_il_weight must *always* have a
>> >   valid node number, e.g. it cannot be NUMA_NO_NODE (-1).
>> 
>> IIUC, we don't need that, "MAX_NUMNODES-1" is used instead.
>> 
>
> Correct, I just thought it pertinent to call this out explicitly since
> I'm stealing the top byte, but the node value has traditionally been a
> full integer.
>
> This may be relevant should anyone try to carry, a random node value
> into this field. For example, if someone tried to copy policy->home_node
> into cur_il_weight for whatever reason.
>
> It's worth breaking out a function to defend against this - plus to hide
> the bit operations directly as you recommend below.
>
>> >  	/* Weighted interleave settings */
>> > -	u8 cur_il_weight;
>> > +	atomic_t cur_il_weight;
>> 
>> If we use this field for node and weight, why not change the field name?
>> For example, cur_wil_node_weight.
>> 
>
> ack.
>
>> > +			if (cweight & 0xFF)
>> > +				*policy = cweight >> 8;
>> 
>> Please define some helper functions or macros instead of operate on bits
>> directly.
>> 
>
> ack.
>
>> >  			else
>> >  				*policy = next_node_in(current->il_prev,
>> >  						       pol->nodes);
>> 
>> If we record current node in pol->cur_il_weight, why do we still need
>> curren->il_prev.  Can we only use pol->cur_il_weight?  And if so, we can
>> even make current->il_prev a union.
>> 
>
> I just realized that there's a problem here for shared memory policies.
>
> from weighted_interleave_nodes, I do this:
>
> cur_weight = atomic_read(&policy->cur_il_weight);
> ...
> weight--;
> ...
> atomic_set(&policy->cur_il_weight, cur_weight);
>
> On a shared memory policy, this is a race condition.
>
>
> I don't think we can combine il_prev and cur_wil_node_weight because
> the task policy may be different than the current policy.
>
> i.e. it's totally valid to do the following:
>
> 1) set_mempolicy(MPOL_INTERLEAVE)
> 2) mbind(..., MPOL_WEIGHTED_INTERLEAVE)
>
> Using current->il_prev between these two policies, is just plain incorrect,
> so I will need to rethink this, and the existing code will need to be
> updated such that weighted_interleave does not use current->il_prev.

IIUC, weighted_interleave_nodes() is only used for mempolicy of tasks
(set_mempolicy()), as in the following code.

+		*nid = (ilx == NO_INTERLEAVE_INDEX) ?
+			weighted_interleave_nodes(pol) :
+			weighted_interleave_nid(pol, ilx);

But, in contrast, it's bad to put task-local "current weight" in
mempolicy.  So, I think that it's better to move cur_il_weight to
task_struct.  And maybe combine it with current->il_prev.

--
Best Regards,
Huang, Ying

On Mon, Jan 29, 2024 at 04:17:46PM +0800, Huang, Ying wrote:
> Gregory Price <gregory.price@memverge.com> writes:
> 
> > Using current->il_prev between these two policies, is just plain incorrect,
> > so I will need to rethink this, and the existing code will need to be
> > updated such that weighted_interleave does not use current->il_prev.
> 
> IIUC, weighted_interleave_nodes() is only used for mempolicy of tasks
> (set_mempolicy()), as in the following code.
> 
> +		*nid = (ilx == NO_INTERLEAVE_INDEX) ?
> +			weighted_interleave_nodes(pol) :
> +			weighted_interleave_nid(pol, ilx);
>

Was digging through this the past couple of days.  It does look like
this is true - because if (pol) comes from a vma, ilx will not be
NO_INTERLEAVE_INDEX.  If this changes in the future, however,
weighted_interleave_nodes may begin to miscount under heavy contention.

It may be worth documenting this explicitly, because this is incredibly
non-obvious.  I will add a comment to this chunk here.

> But, in contrast, it's bad to put task-local "current weight" in
> mempolicy.  So, I think that it's better to move cur_il_weight to
> task_struct.  And maybe combine it with current->il_prev.
> 

Given all of this, I think is reasonably. That is effectively what is
happening anyway for anyone that just uses `numactl -w --interleave=...`

Style question: is it preferable add an anonymous union into task_struct:

union {
    short il_prev;
    atomic_t wil_node_weight;
};

Or should I break out that union explicitly in mempolicy.h?

The latter involves additional code updates in mempolicy.c for the union
name (current->___.il_prev) but it lets us add documentation to mempolicy.h

~Gregory

On Mon, Jan 29, 2024 at 10:48:47AM -0500, Gregory Price wrote:
> On Mon, Jan 29, 2024 at 04:17:46PM +0800, Huang, Ying wrote:
> > Gregory Price <gregory.price@memverge.com> writes:
> > 
> > But, in contrast, it's bad to put task-local "current weight" in
> > mempolicy.  So, I think that it's better to move cur_il_weight to
> > task_struct.  And maybe combine it with current->il_prev.
> > 
> Style question: is it preferable add an anonymous union into task_struct:
> 
> union {
>     short il_prev;
>     atomic_t wil_node_weight;
> };
> 
> Or should I break out that union explicitly in mempolicy.h?
> 

Having attempted this, it looks like including mempolicy.h into sched.h
is a non-starter.  There are build issues likely associated from the
nested include of uapi/linux/mempolicy.h

So I went ahead and did the following.  Style-wise If it's better to just
integrate this as an anonymous union in task_struct, let me know, but it
seemed better to add some documentation here.

I also added static get/set functions to mempolicy.c to touch these
values accordingly.

As suggested, I changed things to allow 0-weight in il_prev.node_weight
adjusted the logic accordingly. Will be testing this for a day or so
before sending out new patches.

~Gregory



diff --git a/include/linux/sched.h b/include/linux/sched.h
index ffe8f618ab86..f0d2af3bbc69 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -745,6 +745,29 @@ struct kmap_ctrl {
 #endif
 };

+
+/*
+ * Describes task_struct interleave settings
+ *
+ * Interleave uses mpol_interleave.node
+ *   last node allocated from
+ *   intended for use in next_node_in() on the next allocation
+ *
+ * Weighted interleave uses mpol_interleave.node_weight
+ *   node is the value of the current node to allocate from
+ *   weight is the number of allocations left on that node
+ *   when weight is 0, next_node_in(node) will be invoked
+ */
+union mpol_interleave {
+       struct {
+               short node;
+               short resv;
+       };
+       /* structure: short node; u8 resv; u8 weight; */
+       atomic_t node_weight;
+};
+
+
 struct task_struct {
 #ifdef CONFIG_THREAD_INFO_IN_TASK
        /*
@@ -1258,7 +1281,7 @@ struct task_struct {
 #ifdef CONFIG_NUMA
        /* Protected by alloc_lock: */
        struct mempolicy                *mempolicy;
-       short                           il_prev;
+       union mpol_interleave           il_prev;
        short                           pref_node_fork;
 #endif
 #ifdef CONFIG_NUMA_BALANCING



diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 92740b8f0eb5..48e365b507b2 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -149,6 +149,66 @@ static struct mempolicy preferred_node_policy[MAX_NUMNODES];
 static u8 __rcu *iw_table;
 static DEFINE_MUTEX(iw_table_lock);

+static u8 get_il_weight(int node)
+{
+       u8 __rcu *table;
+       u8 weight;
+
+       rcu_read_lock();
+       table = rcu_dereference(iw_table);
+       /* if no iw_table, use system default */
+       weight = table ? table[node] : 1;
+       /* if value in iw_table is 0, use system default */
+       weight = weight ? weight : 1;
+       rcu_read_unlock();
+       return weight;
+}
+
+/* Clear any interleave values from task->il_prev */
+static void clear_il_prev(void)
+{
+       int node_weight;
+
+       node_weight = MAKE_WIL_PREV(MAX_NUMNODES - 1, 0);
+       atomic_set(&current->il_prev.node_weight, node_weight);
+}
+
+/* get the next value for weighted interleave */
+static void get_wil_prev(int *node, u8 *weight)
+{
+       int node_weight;
+
+       node_weight = atomic_read(&current->il_prev.node_weight);
+       *node = WIL_NODE(node_weight);
+       *weight = WIL_WEIGHT(node_weight);
+}
+
+/* set the next value for weighted interleave */
+static void set_wil_prev(int node, u8 weight)
+{
+       int node_weight;
+
+       if (node == MAX_NUMNODES)
+               node -= 1;
+       node_weight = MAKE_WIL_PREV(node, weight);
+       atomic_set(&current->il_prev.node_weight, node_weight);
+}
+
+/* get the previous interleave node */
+static short get_il_prev(void)
+{
+       return current->il_prev.node;
+}
+
+/* set the previous interleave node */
+static void set_il_prev(int node)
+{
+       if (unlikely(node >= MAX_NUMNODES))
+               node = MAX_NUMNODES - 1;
+
+       current->il_prev.node = node;
+}
+

Gregory Price <gregory.price@memverge.com> writes:

> On Mon, Jan 29, 2024 at 10:48:47AM -0500, Gregory Price wrote:
>> On Mon, Jan 29, 2024 at 04:17:46PM +0800, Huang, Ying wrote:
>> > Gregory Price <gregory.price@memverge.com> writes:
>> > 
>> > But, in contrast, it's bad to put task-local "current weight" in
>> > mempolicy.  So, I think that it's better to move cur_il_weight to
>> > task_struct.  And maybe combine it with current->il_prev.
>> > 
>> Style question: is it preferable add an anonymous union into task_struct:
>> 
>> union {
>>     short il_prev;
>>     atomic_t wil_node_weight;
>> };
>> 
>> Or should I break out that union explicitly in mempolicy.h?
>> 
>
> Having attempted this, it looks like including mempolicy.h into sched.h
> is a non-starter.  There are build issues likely associated from the
> nested include of uapi/linux/mempolicy.h
>
> So I went ahead and did the following.  Style-wise If it's better to just
> integrate this as an anonymous union in task_struct, let me know, but it
> seemed better to add some documentation here.
>
> I also added static get/set functions to mempolicy.c to touch these
> values accordingly.
>
> As suggested, I changed things to allow 0-weight in il_prev.node_weight
> adjusted the logic accordingly. Will be testing this for a day or so
> before sending out new patches.
>

Thanks about this again.  It seems that we don't need to touch
task->il_prev and task->il_weight during rebinding for weighted
interleave too.

For weighted interleaving, il_prev is the node used for previous
allocation, il_weight is the weight after previous allocation.  So
weighted_interleave_nodes() could be as follows,

unsigned int weighted_interleave_nodes(struct mempolicy *policy)
{
        unsigned int nid;
        struct task_struct *me = current;

        nid = me->il_prev;
        if (!me->il_weight || !node_isset(nid, policy->nodes)) {
                nid = next_node_in(...);
                me->il_prev = nid;
                me->il_weight = weights[nid];
        }
        me->il_weight--;

        return nid;
}

If this works, we can just add il_weight into task_struct.

--
Best Regards,
Huang, Ying

On Tue, Jan 30, 2024 at 11:15:35AM +0800, Huang, Ying wrote:
> Gregory Price <gregory.price@memverge.com> writes:
> 
> > On Mon, Jan 29, 2024 at 10:48:47AM -0500, Gregory Price wrote:
> >> On Mon, Jan 29, 2024 at 04:17:46PM +0800, Huang, Ying wrote:
> >> > Gregory Price <gregory.price@memverge.com> writes:
> >> > 
> >> > But, in contrast, it's bad to put task-local "current weight" in
> >> > mempolicy.  So, I think that it's better to move cur_il_weight to
> >> > task_struct.  And maybe combine it with current->il_prev.
> >> > 
> >> Style question: is it preferable add an anonymous union into task_struct:
> >> 
> >> union {
> >>     short il_prev;
> >>     atomic_t wil_node_weight;
> >> };
> >> 
> >> Or should I break out that union explicitly in mempolicy.h?
> >> 
> >
> > Having attempted this, it looks like including mempolicy.h into sched.h
> > is a non-starter.  There are build issues likely associated from the
> > nested include of uapi/linux/mempolicy.h
> >
> > So I went ahead and did the following.  Style-wise If it's better to just
> > integrate this as an anonymous union in task_struct, let me know, but it
> > seemed better to add some documentation here.
> >
> > I also added static get/set functions to mempolicy.c to touch these
> > values accordingly.
> >
> > As suggested, I changed things to allow 0-weight in il_prev.node_weight
> > adjusted the logic accordingly. Will be testing this for a day or so
> > before sending out new patches.
> >
> 
> Thanks about this again.  It seems that we don't need to touch
> task->il_prev and task->il_weight during rebinding for weighted
> interleave too.
> 

It's not clear to me this is the case.  cpusets takes the task_lock to
change mems_allowed and rebind task->mempolicy, but I do not see the
task lock access blocking allocations.

Comments from cpusets suggest allocations can happen in parallel.

/*
 * cpuset_change_task_nodemask - change task's mems_allowed and mempolicy
 * @tsk: the task to change
 * @newmems: new nodes that the task will be set
 *
 * We use the mems_allowed_seq seqlock to safely update both tsk->mems_allowed
 * and rebind an eventual tasks' mempolicy. If the task is allocating in
 * parallel, it might temporarily see an empty intersection, which results in
 * a seqlock check and retry before OOM or allocation failure.
 */


For normal interleave, this isn't an issue because it always proceeds to
the next node. The same is not true of weighted interleave, which may
have a hanging weight in task->il_weight.

That is why I looked to combine the two, so at least node/weight were
carried together.

> unsigned int weighted_interleave_nodes(struct mempolicy *policy)
> {
>         unsigned int nid;
>         struct task_struct *me = current;
> 
>         nid = me->il_prev;
>         if (!me->il_weight || !node_isset(nid, policy->nodes)) {
>                 nid = next_node_in(...);
>                 me->il_prev = nid;
>                 me->il_weight = weights[nid];
>         }
>         me->il_weight--;
> 
>         return nid;
> }

I ended up with this:

static unsigned int weighted_interleave_nodes(struct mempolicy *policy)
{
       unsigned int node;
       u8 weight;

       get_wil_prev(&node, &weight);
       /* If nodemask was rebound, just fetch the next node */
       if (!weight) {
               node = next_node_in(node, policy->nodes);
               /* can only happen if nodemask has become invalid */
               if (node == MAX_NUMNODES)
                       return node;
               weight = get_il_weight(node);
       }
       weight--;
       set_wil_prev(node, weight);
       return node;
}

~Gregory

Gregory Price <gregory.price@memverge.com> writes:

> On Tue, Jan 30, 2024 at 11:15:35AM +0800, Huang, Ying wrote:
>> Gregory Price <gregory.price@memverge.com> writes:
>> 
>> > On Mon, Jan 29, 2024 at 10:48:47AM -0500, Gregory Price wrote:
>> >> On Mon, Jan 29, 2024 at 04:17:46PM +0800, Huang, Ying wrote:
>> >> > Gregory Price <gregory.price@memverge.com> writes:
>> >> > 
>> >> > But, in contrast, it's bad to put task-local "current weight" in
>> >> > mempolicy.  So, I think that it's better to move cur_il_weight to
>> >> > task_struct.  And maybe combine it with current->il_prev.
>> >> > 
>> >> Style question: is it preferable add an anonymous union into task_struct:
>> >> 
>> >> union {
>> >>     short il_prev;
>> >>     atomic_t wil_node_weight;
>> >> };
>> >> 
>> >> Or should I break out that union explicitly in mempolicy.h?
>> >> 
>> >
>> > Having attempted this, it looks like including mempolicy.h into sched.h
>> > is a non-starter.  There are build issues likely associated from the
>> > nested include of uapi/linux/mempolicy.h
>> >
>> > So I went ahead and did the following.  Style-wise If it's better to just
>> > integrate this as an anonymous union in task_struct, let me know, but it
>> > seemed better to add some documentation here.
>> >
>> > I also added static get/set functions to mempolicy.c to touch these
>> > values accordingly.
>> >
>> > As suggested, I changed things to allow 0-weight in il_prev.node_weight
>> > adjusted the logic accordingly. Will be testing this for a day or so
>> > before sending out new patches.
>> >
>> 
>> Thanks about this again.  It seems that we don't need to touch
>> task->il_prev and task->il_weight during rebinding for weighted
>> interleave too.
>> 
>
> It's not clear to me this is the case.  cpusets takes the task_lock to
> change mems_allowed and rebind task->mempolicy, but I do not see the
> task lock access blocking allocations.
>
> Comments from cpusets suggest allocations can happen in parallel.
>
> /*
>  * cpuset_change_task_nodemask - change task's mems_allowed and mempolicy
>  * @tsk: the task to change
>  * @newmems: new nodes that the task will be set
>  *
>  * We use the mems_allowed_seq seqlock to safely update both tsk->mems_allowed
>  * and rebind an eventual tasks' mempolicy. If the task is allocating in
>  * parallel, it might temporarily see an empty intersection, which results in
>  * a seqlock check and retry before OOM or allocation failure.
>  */
>
>
> For normal interleave, this isn't an issue because it always proceeds to
> the next node. The same is not true of weighted interleave, which may
> have a hanging weight in task->il_weight.

So, I added a check as follows,

node_isset(current->il_prev, policy->nodes)

If prev node is removed from nodemask, allocation will proceed to the
next node.  Otherwise, it's safe to use current->il_weight.  

--
Best Regards,
Huang, Ying

> That is why I looked to combine the two, so at least node/weight were
> carried together.
>
>> unsigned int weighted_interleave_nodes(struct mempolicy *policy)
>> {
>>         unsigned int nid;
>>         struct task_struct *me = current;
>> 
>>         nid = me->il_prev;
>>         if (!me->il_weight || !node_isset(nid, policy->nodes)) {
>>                 nid = next_node_in(...);
>>                 me->il_prev = nid;
>>                 me->il_weight = weights[nid];
>>         }
>>         me->il_weight--;
>> 
>>         return nid;
>> }
>
> I ended up with this:
>
> static unsigned int weighted_interleave_nodes(struct mempolicy *policy)
> {
>        unsigned int node;
>        u8 weight;
>
>        get_wil_prev(&node, &weight);
>        /* If nodemask was rebound, just fetch the next node */
>        if (!weight) {
>                node = next_node_in(node, policy->nodes);
>                /* can only happen if nodemask has become invalid */
>                if (node == MAX_NUMNODES)
>                        return node;
>                weight = get_il_weight(node);
>        }
>        weight--;
>        set_wil_prev(node, weight);
>        return node;
> }
>
> ~Gregory

On Tue, Jan 30, 2024 at 01:18:30PM +0800, Huang, Ying wrote:
> Gregory Price <gregory.price@memverge.com> writes:
> 
> > For normal interleave, this isn't an issue because it always proceeds to
> > the next node. The same is not true of weighted interleave, which may
> > have a hanging weight in task->il_weight.
> 
> So, I added a check as follows,
> 
> node_isset(current->il_prev, policy->nodes)
> 
> If prev node is removed from nodemask, allocation will proceed to the
> next node.  Otherwise, it's safe to use current->il_weight.  
> 

Funny enough I have this on one of my branches and dropped it, but after
digging through everything - this should be sufficient.

I'll just add il_weight next to il_prev and have a new set of patches
out today. Code is already there, just needs one last cleanup pass.

~Gregory