Re: Re: [External Mail] [RFC PATCH] mm/mempolicy: Weighted interleave auto-tuning

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

On Mon, 16 Dec 2024 16:53:47 +0900 Hyeonggon Yoo <hyeonggon.yoo@sk.com> wrote:
> 
> On 2024-12-14 4:57 AM, Joshua Hahn wrote:
> > On Fri, 13 Dec 2024 15:19:20 +0900 Hyeonggon Yoo <hyeonggon.yoo@sk.com> wrote:
> > 
> >> On 2024-12-11 06:54 AM, Joshua Hahn wrote:

[-----8<-----]

> > Hi Hyeonggon,
> > 
> > Thank you for reviewing my patch!
> 
> No problem :)
> 
> > As Gregory showed in his reply, I think it would be possible to get host bandwidth information
> > using the ACPI HMAT.
> 
> You're right. I was wrong. I checked on our server, and its bandwidth 
> information was valid for both local memory and CXL memory. Sorry for
> the noise.

No worries, thank you for verifying from your end as well!

> > [-----8<-----]
> > 
> >>> +What:		/sys/kernel/mm/mempolicy/weighted_interleave/max_node_weight
> >>> +Date:		December 2024
> >>> +Contact:	Linux memory management mailing list <linux-mm@kvack.org>
> >>> +Description:	Weight limiting / scaling interface
> >>> +
> >>> +		The maximum interleave weight for a memory node. When it is
> >>> +		updated, any previous changes to interleave weights (i.e. via
> >>> +		the nodeN sysfs interfaces) are ignored, and new weights are
> >>> +		calculated using ACPI-reported bandwidths and scaled.
> >>> +
> >>
> >> At first this paragraph sounded like "previously stored weights are
> >> discarded after setting max_node_weight", but I think you mean
> >> "User can override the default values, but defaults values are
> >> calculated regardless of the values set by the user". Right?
> > 
> > In the implementation, the first way you interpreted is the correct
> > description. That is, if a user manually changes a ndoe weight,
> > then updates the max_node_weight, the previous manual change will
> > be overwritten by the newly scaled values.
>  >
> > Does this behavior make sense?
> 
> Ok. then current implementation overwrites the node weights
> previously set by the user.
> 
> I think it makes sense to re-scale all nodes and overwrite manually set 
> node weights, because it's what the knob is meant to do, and the user 
> still can override the weight by setting node weight after updating
> max_node_weight.

Thank you for your feedback. There is a slight concern, however, where there
is a semantic mismatch between the name "max_node_weight" and its value.
Like the description suggests, it is possible for individual nodes to have
weights greater than the "max node weight".

However, the alternative would be to re-scale all weights whenever an
individual node's weight is manually overwritten to be greater than
the max, which I think makes even less sense since users probably don't
expect changing one weight to influence other nodes as well.
 
> By the way, could you please explain which part of this patch implements
> this rule? IIUC it does not invalidate iw_table after updating these
> default values, which makes get_il_weight() to use manully set node 
> weights even after updating max_node_weight. (Or probably I just
> misunderstood the code?)

Ah, I am sorry for this mistake. It seems like I didn't update the
actual iw table, updating the default instead. Thank you for the catch,
this will be updated in the v2.

Actually, there are a few more changes that I would like to make in the
v2, the biggest of which is to lay down the intended behavior more
explicitly in the documentation so that there is no ambiguity
from the user perspective, and make sure that the code actually
does reflect the intention as well. 

> > Have a great day!
> 
> Have a great day too :)
> 
> --
> Hyeonggon

Thank you again for your feedback, happy holidays!

Joshua