Re: [PATCH v2] mm/mempolicy: fix mpol_rebind_nodemask() for MPOL_F_NUMA_BALANCING

["Huang, Ying" <ying.huang@linux.alibaba.com>]

Jinjiang Tu <tujinjiang@huawei.com> writes:

> 在 2025/12/22 17:51, Huang, Ying 写道:
>> Hi, Jinjiang,
>>
>> Sorry, I found the patch description is still confusing for me.
>>
>> Jinjiang Tu <tujinjiang@huawei.com> writes:
>>
>>> commit bda420b98505 ("numa balancing: migrate on fault among multiple
>>> bound nodes") adds new flag MPOL_F_NUMA_BALANCING to enable NUMA balancing
>>> for MPOL_BIND memory policy.
>> Is the following description better?  At least, I think we should
>> emphasize that MPOL_F_NUMA_BALANCING is set while both
>> MPOL_F_STATIC_NODES and MPOL_F_RELATIVE_NODES are cleared in the mode
>> parameter.
>
> Thanks, I will update it to make it clearer. How about the following
> description?
>
>
> commit bda420b98505 ("numa balancing: migrate on fault among multiple
> bound nodes") adds new flag MPOL_F_NUMA_BALANCING to enable NUMA balancing
> for MPOL_BIND memory policy.
>
> When the cpuset of tasks changes, the mempolicy of the task is rebound
> by mpol_rebind_nodemask(). When MPOL_F_STATIC_NODES and MPOL_F_RELATIVE_NODES
> are both not set, the behaviour is same whenever MPOL_F_NUMA_BALANCING

s/is/should be/

> is set or not. So, when an application calls set_mempolicy() with MPOL_F_NUMA_BALANCING
> set but both MPOL_F_STATIC_NODES and MPOL_F_RELATIVE_NODES cleared,
> mempolicy.w.cpuset_mems_allowed should be set to cpuset_current_mems_allowed nodemask.
> However, in current implementation, mpol_store_user_nodemask() wrongly returns true,
> causing mempolicy->w.user_nodemask to be incorrectly set to the user-specified nodemask.
> Later, when the cpuset of the application changes, mpol_rebind_nodemask() ends up rebinding
> based on the user-specified nodemask rather than the cpuset_mems_allowed
> nodemask as intended.
>
> To fix this, only set mempolicy->w.user_nodemask to the user-specified nodemask
> if MPOL_F_STATIC_NODES or MPOL_F_RELATIVE_NODES is present.

This looks good to me.  Thanks!  Feel free to add my

Reviewed-by: Huang Ying <ying.huang@linux.alibaba.com>

in the future versions.

>>
>> When an application calls set_mempolicy() with MPOL_F_NUMA_BALANCING set
>> but both MPOL_F_STATIC_NODES and MPOL_F_RELATIVE_NODES cleared,
>> mempolicy.w.cpuset_mems_allowed should be set to
>> cpuset_current_mems_allowed nodemask.  However, due to a bug in its
>> current implementation, mpol_store_user_nodemask() wrongly returns true,
>> causing mempolicy->w.user_nodemask to be incorrectly set to the
>> user-specified nodemask (or an empty nodemask).  Later, when the cpuset
>> of the application changes, mpol_rebind_nodemask() ends up rebinding
>> based on the user-specified nodemask rather than the cpuset_mems_allowed
>> nodemask as intended.
>>
>>> when the cpuset of tasks changes, the mempolicy of the task is rebound
>>> by mpol_rebind_nodemask(). The intended rebinding behavior of
>>> MPOL_F_NUMA_BALANCING was the same as when neither MPOL_F_STATIC_NODES nor
>>> MPOL_F_RELATIVE_NODES flags are set. However, this commit breaks it.
>>>
>>> struct mempolicy has a union member as bellow:
>>>
>>>     union {
>>>         nodemask_t cpuset_mems_allowed; /* relative to these nodes */
>>>         nodemask_t user_nodemask;       /* nodemask passed by user */
>>>     } w;
>>>
>>> w.cpuset_mems_allowed and w.user_nodemask are both nodemask type and their
>>> difference is only what type of nodemask is stored. mpol_set_nodemask()
>>> initializes the union like below:
>>>
>>>     static int mpol_set_nodemask(...)
>>>     {
>>>          if (mpol_store_user_nodemask(pol))
>>>                  pol->w.user_nodemask = *nodes;
>>>          else
>>>                  pol->w.cpuset_mems_allowed = cpuset_current_mems_allowed;
>>>     }
>>>
>>> mpol_store_user_nodemask() returns true for MPOL_F_NUMA_BALANCING
>>> incorrectly and the union stores user-passed nodemask. Consequently,
>>> mpol_rebind_nodemask() ends up rebinding based on the user-passed nodemask
>>> rather than the cpuset_mems_allowed nodemask as intended.
>>>
>>> To fix this, only store the user nodemask if MPOL_F_STATIC_NODES or
>>> MPOL_F_RELATIVE_NODES is present.
>>>
>>> Fixes: bda420b98505 ("numa balancing: migrate on fault among multiple bound nodes")
>>> Reviewed-by: Gregory Price <gourry@gourry.net>
>>> Signed-off-by: Jinjiang Tu <tujinjiang@huawei.com>
>> [snip]
>>

---
Best Regards,
Huang, Ying