Re: [PATCH v3 3/4] mm/mempolicy: introduce MPOL_WEIGHTED_INTERLEAVE for weighted interleaving

[Gregory Price <gregory.price@memverge.com>]

On Fri, Jan 26, 2024 at 03:10:49PM +0800, Huang, Ying wrote:
> Gregory Price <gourry.memverge@gmail.com> writes:
> 
> > +		} else if (pol == current->mempolicy &&
> > +				(pol->mode == MPOL_WEIGHTED_INTERLEAVE)) {
> > +			if (pol->cur_il_weight)
> > +				*policy = current->il_prev;
> > +			else
> > +				*policy = next_node_in(current->il_prev,
> > +						       pol->nodes);
> 
> It appears that my previous comments about this is ignored.
> 
> https://lore.kernel.org/linux-mm/875xzkv3x2.fsf@yhuang6-desk2.ccr.corp.intel.com/
> 
> Please correct me if I am wrong.
>

The fix is in the following patch.  I'd originally planned to squash the
atomic patch into this one, but decided against it because it probably
warranted isolated scrutiny.

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

in this we return the node the weight applies to, otherwise we return
whatever is after il_prev.

I can pull this fix ahead.

> > +	/* if now at 0, move to next node and set up that node's weight */
> > +	if (unlikely(!policy->cur_il_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();
> > +	}
> 
> It appears that the code could be more concise if we allow
> policy->cur_il_weight == 0.  Duplicated code are in
> alloc_pages_bulk_array_weighted_interleave() too.  Anyway, can we define
> some function to reduce duplicated code.
> 

This is kind of complicated by the next patch, which places the node and
the weight into the same field to resolve the stale weight issue.

In that patch (cur_il_weight = 0) means "cur_il_weight invalid",
because the weight part can only be 0 when:

a) an error occuring during bulk allocation
b) a rebind event

I'll take some time to think about whether we can do away with
task->il_prev (as your next patch notes mentioned).


> > +		/* Otherwise we adjust nr_pages down, and continue from there */
> > +		rem_pages -= pol->cur_il_weight;
> > +		pol->cur_il_weight = 0;
> 
> This break the rule to keep pol->cur_il_weight != 0 except after initial
> setup.  Is it OK?
> 

The only way cur_il_weight can leave this function 0 at this point is if
an error occurs (specifically the failure to kmalloc immediately next).

If we don't clear cur_il_weight here, then we have a stale weight, and
the next allocation pass will over-allocate on the current node.

This semantic also changes a bit in the next patch, but is basically the
same.  If il_weight is 0, then either an error occurred or a rebind
event occured.

> > +				/* resume from this node w/ remaining weight */
> > +				resume_node = prev_node;
> > +				resume_weight = weight - (node_pages % weight);
> 
> resume_weight = weight - delta; ?
>

ack

~Gregory