Re: [PATCH v5 01/11] mm/mempolicy: implement the sysfs-based weighted_interleave interface

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

On Wed, Dec 27, 2023 at 02:42:15PM +0800, Huang, Ying wrote:
> Gregory Price <gourry.memverge@gmail.com> writes:
> 
> > +		These weights only affect new allocations, and changes at runtime
> > +		will not cause migrations on already allocated pages.
> > +
> > +		Writing an empty string resets the weight value to 1.
> 
> I still think that it's a good idea to provide some better default
> weight value with HMAT or CDAT if available.  So, better not to make "1"
> as part of ABI?
> 

That's the eventual goal, but this is just the initial mechanism.

My current thought is that the CXL driver will apply weights as the
system iterates through devices and creates numa nodes.  In the
meantime, you have to give the "possible" nodes a default value to
prevent nodes onlined after boot from showing up with 0-value.

Not allowing 0-value weights is simply easier in many respects.

> > +
> > +		Minimum weight: 1
> 
> Can weight be "0"?  Do we need a way to specify that a node don't want
> to participate weighted interleave?
> 

In this code, weight cannot be 0.  My thoguht is that removing the node
from the nodemask is the way to denote 0.

The problem with 0 is hotplug, migration, and cpusets.mems_allowed.  

Example issue:  Use set local weights to [1,0,1,0] for nodes [0-3],
and has a cpusets.mems_allowed mask of (0, 2).

Lets say the user migrates the task via cgroups from nodes (0,2) to
(1,3).

The task will instantly crash as basically OOM because weights of
[1,0,1,0] will prevent memory from being allocations.

Not allowing nodes weights of 0 is defensive.  Instead, simply removing
the node from the nodemask and/or mems_allowed is both equivalent to and
the preferred way to apply a weight of 0.

> > +		Maximum weight: 255
> > diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> > index 10a590ee1c89..0e77633b07a5 100644
> > --- a/mm/mempolicy.c
> > +++ b/mm/mempolicy.c
> > @@ -131,6 +131,8 @@ static struct mempolicy default_policy = {
> >  
> >  static struct mempolicy preferred_node_policy[MAX_NUMNODES];
> >  
> > +static char iw_table[MAX_NUMNODES];
> > +
> 
> It's kind of obscure whether "char" is "signed" or "unsigned".  Given
> the max weight is 255 above, it's better to use "u8"?
>

bah, stupid mistake.  I will switch this to u8.

> And, we may need a way to specify whether the weight has been overridden
> by the user.
> A special value (such as 255) can be used for that.  If
> so, the maximum weight should be 254 instead of 255.  As a user space
> interface, is it better to use 100 as the maximum value?
> 

There's global weights and local weights.  These are the global weights.

Local weights are stored in task->mempolicy.wil.il_weights.

(policy->mode_flags & MPOL_F_GWEIGHT) denotes the override.
This is set if (mempolicy_args->il_weights) was provided.

This simplifies the interface.

(note: local weights are not introduced until the last patch 11/11)

> > +
> > +static void sysfs_mempolicy_release(struct kobject *mempolicy_kobj)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < MAX_NUMNODES; i++)
> > +		sysfs_wi_node_release(node_attrs[i], mempolicy_kobj);
> 
> IIUC, if this is called in error path (such as, in
> add_weighted_interleave_group()), some node_attrs[] element may be
> "NULL"?
> 

The null check is present in sysfs_wi_node_release

if (!node_attr)
	return;

Is it preferable to pull this out? Seemed more defensive to put it
inside the function.

~Gregory