Re: [PATCH v3] Weighted interleave auto-tuning

[Joshua Hahn <joshua.hahnjy@gmail.com>]

Hi Hyeonggon, thank you for the review!

[...snip...]

> Hi Joshua, thanks for the update!
> It actually is what I was intended in the manual / auto mode description.
> 
> I don't have a strong opinion on the weight of the hot-plugged NUMA node
> in manual mode, as it's not ideal whatever weight we choose and the user
> need to update the weight after hot-plug events anyway.

I'm glad that I was able to correctly interpret the framework you laid
out in the previous conversations. And yes -- I agree, I think no matter
what value I choose, it will always be sub-optimal for some definition
of optimality. I simply chose 1 because it is now the new smallest
weight possible, since 0 no longer works.

> Some comments inlined below:
> 
> > diff --git a/Documentation/ABI/testing/sysfs-kernel-mm-mempolicy-weighted-interleave b/Documentation/ABI/testing/sysfs-kernel-mm-mempolicy-weighted-interleave
> > index 0b7972de04e9..d30dc29c53ff 100644
> > --- a/Documentation/ABI/testing/sysfs-kernel-mm-mempolicy-weighted-interleave
> > +++ b/Documentation/ABI/testing/sysfs-kernel-mm-mempolicy-weighted-interleave
> > @@ -20,6 +20,30 @@ Description:	Weight configuration interface for nodeN
> >   		Minimum weight: 1
> >   		Maximum weight: 255
> >   
> > -		Writing an empty string or `0` will reset the weight to the
> > -		system default. The system default may be set by the kernel
> > -		or drivers at boot or during hotplug events.
> > +		Writing invalid values (i.e. any values not in [1,255],
> > +		empty string, ...) will return -EINVAL.
> > +
> > +What:		/sys/kernel/mm/mempolicy/weighted_interleave/mode
> > +Date:		January 2025
> > +Contact:	Linux memory management mailing list <linux-mm@kvack.org>
> > +Description:	Auto-weighting configuration interface
> > +
> > +		Configuration modes for weighted interleave. Can take one of
> > +		two options: "manual" and "auto". Default is "auto".
> > +
> > +		In auto mode, all node weights are re-calculated and overwritten
> > +		(visible via the nodeN interfaces) whenever new bandwidth data
> > +		is made available either during boot or hotplug events.
> > +
> > +		In manual mode, node weights can only be updated by the user.
> > +		If a node is hotplugged while the user is in manual mode,
> > +		the node will have a default weight of 1.
> > +
> > +		Modes can be changed by writing either "auto" or "manual" to the
> > +		interface. All other strings will be ignored, and -EINVAL will
> > +		be returned. If "auto" is written to the interface but the
> > +		recalculation / updates fail at any point (-ENOMEM or -ENODEV)
> > +		then the mode will remain in manual mode.
> > +
> > +		Writing a new weight to a node directly via the nodeN interface
> > +		will also automatically update the system to manual mode.
> 
> I think the last paragraph should also be included in the nodeX parameter.

I agree, I will definitely add this in the next version!
 
> > @@ -2450,16 +2548,8 @@ static unsigned long alloc_pages_bulk_array_weighted_interleave(gfp_t gfp,
> >   	if (!weights)
> >   		return total_allocated;
> >   
> > -	rcu_read_lock();
> > -	table = rcu_dereference(iw_table);
> > -	if (table)
> > -		memcpy(weights, table, nr_node_ids);
> > -	rcu_read_unlock();
> > -
> > -	/* calculate total, detect system default usage */
> >   	for_each_node_mask(node, nodes) {
> > -		if (!weights[node])
> > -			weights[node] = 1;
> > +		weights[node] = get_il_weight(node);
> >   		weight_total += weights[node];
> >   	}
> 
> Uh-hum...
> Looks like it now allows copying weights from different versions of iw_tables?

This is a good point, this is actually an artifact from a previous
iteration where get_il_weight was needed to handle the weight being
0, but since we no longer allow 0 as a value, it makes more sense to
just take a snapshot under a single rcu lock. Thank you for the catch!

I will also go over the other places this is used and just make sure the
locking behavior is as intended.

> Otherwise this patch looks good to me.
> 
> Best,
> Hyeonggon

Thanks again Hyeonggon, I'll send out a v4 with the changes you mentioned!
Have a great day!!
Joshua