linux-kernel - Re: [PATCH v3] Weighted interleave auto-tuning

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250121212430.3902148-1-joshua.hahnjy@gmail.com>
Date: Tue, 21 Jan 2025 13:24:28 -0800
From: Joshua Hahn <joshua.hahnjy@...il.com>
To: Hyeonggon Yoo <hyeonggon.yoo@...com>
Cc: "gourry@...rry.net" <gourry@...rry.net>,
	"ying.huang@...ux.alibaba.com" <ying.huang@...ux.alibaba.com>,
	kernel_team@...ynix.com,
	42.hyeyoo@...il.com,
	"rafael@...nel.org" <rafael@...nel.org>,
	"lenb@...nel.org" <lenb@...nel.org>,
	"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>,
	김홍규 System SW <honggyu.kim@...com>,
	김락기 System SW <rakie.kim@...com>,
	"dan.j.williams@...el.com" <dan.j.williams@...el.com>,
	"Jonathan.Cameron@...wei.com" <Jonathan.Cameron@...wei.com>,
	"dave.jiang@...el.com" <dave.jiang@...el.com>,
	"horen.chuang@...ux.dev" <horen.chuang@...ux.dev>,
	"hannes@...xchg.org" <hannes@...xchg.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-acpi@...r.kernel.org" <linux-acpi@...r.kernel.org>,
	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	"kernel-team@...a.com" <kernel-team@...a.com>
Subject: Re: [PATCH v3] Weighted interleave auto-tuning

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@...ck.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