Re: [PATCH 2/2 v6] mm/mempolicy: Don't create weight sysfs for memoryless nodes

[Honggyu Kim <honggyu.kim@sk.com>]

Hi Joshua,

On 3/4/2025 6:56 AM, Joshua Hahn wrote:
> On Thu, 27 Feb 2025 12:20:03 +0900 Honggyu Kim <honggyu.kim@sk.com> wrote:
> 
> Hi Honggyu, thank you for taking time to review my patch, as always!

My pleasure!

> I thought I had sent this, but it seems like it was left in my draft
> without being sent.
> 
> I will follow Gregory's advice and we will drop the patch from this series,
> and send the first patch only (with Yunjeong's changes). Thanks again!

It'd be great if you could add her with the following.
Co-developed-by: Yunjeong Mun <yunjeong.mun@sk.com>

> 
>>
>> On 2/27/2025 11:32 AM, Honggyu Kim wrote:
>>> Hi Joshua,
>>>
>>> On 2/27/2025 6:35 AM, Joshua Hahn wrote:
>>>> We should never try to allocate memory from a memoryless node. Creating a
>>>> sysfs knob to control its weighted interleave weight does not make sense,
>>>> and can be unsafe.
>>>>
>>>> Only create weighted interleave weight knobs for nodes with memory.
>>>>
>>>> Signed-off-by: Joshua Hahn <joshua.hahnjy@gmail.com>
>>>> ---
>>>>    mm/mempolicy.c | 2 +-
>>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
>>>> index 4cc04ff8f12c..50cbb7c047fa 100644
>>>> --- a/mm/mempolicy.c
>>>> +++ b/mm/mempolicy.c
>>>> @@ -3721,7 +3721,7 @@ static int add_weighted_interleave_group(struct
>>>> kobject *root_kobj)
>>>>            return err;
>>>>        }
>>>> -    for_each_node_state(nid, N_POSSIBLE) {
>>>
>>> Actually, we're aware of this issue and currently trying to fix this.
>>> In our system, we've attached 4ch of CXL memory for each socket as
>>> follows.
>>>
>>>           node0             node1
>>>         +-------+   UPI   +-------+
>>>         | CPU 0 |-+-----+-| CPU 1 |
>>>         +-------+         +-------+
>>>         | DRAM0 |         | DRAM1 |
>>>         +---+---+         +---+---+
>>>             |                 |
>>>         +---+---+         +---+---+
>>>         | CXL 0 |         | CXL 4 |
>>>         +---+---+         +---+---+
>>>         | CXL 1 |         | CXL 5 |
>>>         +---+---+         +---+---+
>>>         | CXL 2 |         | CXL 6 |
>>>         +---+---+         +---+---+
>>>         | CXL 3 |         | CXL 7 |
>>>         +---+---+         +---+---+
>>>           node2             node3
>>>
>>> The 4ch of CXL memory are detected as a single NUMA node in each socket,
>>> but it shows as follows with the current N_POSSIBLE loop.
>>>
>>> $ ls /sys/kernel/mm/mempolicy/weighted_interleave/
>>> node0 node1 node2 node3 node4 node5
>>> node6 node7 node8 node9 node10 node11

FYI, we used to set node2 and node3 only for weights for CXL memory here
and ignored node{4-11}.  That sounds silly but it worked.

> 
> I see. For my education, would you mind explaining how the numbering works
> here? I am not very familiar with this setup, and not sure how you would
> figure out what node is which, just by looking at the numbering.

Regarding the numbering, I'm not 100% sure, but I guess there could be a
logical NUMA node that combines 4ch of CXL memory and 4 nodes for CXL 
memory so in total 5 nodes per socket.

I don't have much knowledge on this but maybe this is related to PXM
(Proximity Domain).

> 
>>>> +    for_each_node_state(nid, N_MEMORY) {
>>
>> Thinking it again, we can leave it as a separate patch but add our patch
>> on top of it.
> 
> That sounds good to me.
> 
>> The only concern I have is having only N_MEMORY patch hides weight
>> setting knobs for CXL memory and it makes there is no way to set weight
>> values to CXL memory in my system.
> 
> You can use weighted interleave auto-tuning : -)

Not possible because using N_MEMORY doesn't provide "node" knobs for CXL
memory at all as follows.

$ ls /sys/kernel/mm/mempolicy/weighted_interleave/
node0 node1

We need node2 and node3 for CXL memory here.

> In all seriousness, this makes sense. It seems pretty problematic that
> the knobs aren't created for the CXL channels,

Yeah, it's even worse than the current status.

> and I'm not sure that hiding> it is the correct approach here (it was not my intent, either).

It isn't your problem but we shouldn't hide those nodes until it is
correctly fixed with hot plugging event handler.

> 
>> IMHO, this and our patch is better to be submitted together.
> 
> That sounds good. We can hold off on this patch then, and just consider
> the first patch of this series. Thank you for letting me know!

The N_POSSIBLE and N_MEMORY stuffs should had been fixed earlier than
this work. I will take a few days if we can submit it together.

> 
> Thank you for always reviewing my patches. Have a great day!
> Joshua

Thanks for your work and have a great day you too!

Kind regards,
Honggyu

> 
> Sent using hkml (https://github.com/sjp38/hackermail)
>