[PATCH] mempolicy: check home_node is in the nodes of policy

[PATCH] mempolicy: check home_node is in the nodes of policy

[Chunsheng Luo]

set_mempolicy_home_node should be used after setting the memory
policy. If the home_node isn't in the nodes of policy, we should
return failure to avoid misunderstanding.

Signed-off-by: Chunsheng Luo <luochunsheng@ustc.edu>
---
 mm/mempolicy.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 10a590ee1c89..9282be2ae18e 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -1536,6 +1536,12 @@ SYSCALL_DEFINE4(set_mempolicy_home_node, unsigned long, start, unsigned long, le
 			err = -EOPNOTSUPP;
 			break;
 		}
+
+		if (!node_isset(home_node, old->nodes)) {
+			err = -EINVAL;
+			break;
+		}
+
 		new = mpol_dup(old);
 		if (IS_ERR(new)) {
 			err = PTR_ERR(new);
-- 
2.43.0

Re: [PATCH] mempolicy: check home_node is in the nodes of policy

[Andrew Morton]

On Fri, 26 Jan 2024 08:22:40 -0500 Chunsheng Luo <luochunsheng@ustc.edu> wrote:

> set_mempolicy_home_node should be used after setting the memory
> policy. If the home_node isn't in the nodes of policy, we should
> return failure to avoid misunderstanding.

Thanks.  Under what circumstances does userspace trigger this issue?

Re: [PATCH] mempolicy: check home_node is in the nodes of policy

[ustc]

For example,  In a system with NUMA nodes 0,1,2,3, i mbind process to 
node 0-2 and set home_node to node 3, it will not be allocated from node 
3, then from node closer to node 3. But i think home_node should be set 
directly from node 0-2, which makes more sense. So i think it needs to 
return failure to prompt user.


On 2024/1/28 16:29, Andrew Morton wrote:
> On Fri, 26 Jan 2024 08:22:40 -0500 Chunsheng Luo <luochunsheng@ustc.edu> wrote:
>
>> set_mempolicy_home_node should be used after setting the memory
>> policy. If the home_node isn't in the nodes of policy, we should
>> return failure to avoid misunderstanding.
> Thanks.  Under what circumstances does userspace trigger this issue?
>

Re: [PATCH] mempolicy: check home_node is in the nodes of policy

[Gregory Price]

On Fri, Jan 26, 2024 at 08:22:40AM -0500, Chunsheng Luo wrote:
> set_mempolicy_home_node should be used after setting the memory
> policy. If the home_node isn't in the nodes of policy, we should
> return failure to avoid misunderstanding.
> 
> Signed-off-by: Chunsheng Luo <luochunsheng@ustc.edu>
> ---
>  mm/mempolicy.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

Since it's not possible to add/remove a node to a mask without also
erasing the home node, this seems reasonable.

e.g. this is what happens presently
mbind(0-2)    :   mask(0,1,2),   home_node(NUMA_NO_NODE)
home_node(3)  :   mask(0,1,2),   home_node(3)
mbind(0-3)    :   mask(0,1,2,3), home_node(NUMA_NO_NODE)

However, it is possible for a cgroup migration or a change to
cpusets.mems_allowed to change a nodemask without somping the home_node.

e.g.:
mbind(2-3)    :   mask(2-3),  home_node(NUMA_NO_NODE)
home_node(3)  :   mask(2-3),  home_node(3)
cpusets(0-1)  :   mask(0-1),  home_node(3)

Should the rebind code also shift the home-node or un-set it accordingly
to keep the mask/home_node behavior consistent with the syscalls?

(see mpol_rebind_nodemask)


> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index 10a590ee1c89..9282be2ae18e 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -1536,6 +1536,12 @@ SYSCALL_DEFINE4(set_mempolicy_home_node, unsigned long, start, unsigned long, le
>  			err = -EOPNOTSUPP;
>  			break;
>  		}
> +
> +		if (!node_isset(home_node, old->nodes)) {
> +			err = -EINVAL;
> +			break;
> +		}
> +
>  		new = mpol_dup(old);
>  		if (IS_ERR(new)) {
>  			err = PTR_ERR(new);
> -- 
> 2.43.0
> 
> 

Re: [PATCH] mempolicy: check home_node is in the nodes of policy

[Chunsheng Luo]

[-- Attachment #1: Type: text/plain, Size: 2132 bytes --]


On 2024/1/29 23:12, Gregory Price wrote:
> On Fri, Jan 26, 2024 at 08:22:40AM -0500, Chunsheng Luo wrote:
>> set_mempolicy_home_node should be used after setting the memory
>> policy. If the home_node isn't in the nodes of policy, we should
>> return failure to avoid misunderstanding.
>>
>> Signed-off-by: Chunsheng Luo<luochunsheng@ustc.edu>
>> ---
>>   mm/mempolicy.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
> Since it's not possible to add/remove a node to a mask without also
> erasing the home node, this seems reasonable.
>
> e.g. this is what happens presently
> mbind(0-2)    :   mask(0,1,2),   home_node(NUMA_NO_NODE)
> home_node(3)  :   mask(0,1,2),   home_node(3)
> mbind(0-3)    :   mask(0,1,2,3), home_node(NUMA_NO_NODE)
>
> However, it is possible for a cgroup migration or a change to
> cpusets.mems_allowed to change a nodemask without somping the home_node.
>
> e.g.:
> mbind(2-3)    :   mask(2-3),  home_node(NUMA_NO_NODE)
> home_node(3)  :   mask(2-3),  home_node(3)
> cpusets(0-1)  :   mask(0-1),  home_node(3)
>
> Should the rebind code also shift the home-node or un-set it accordingly
> to keep the mask/home_node behavior consistent with the syscalls?
>
> (see mpol_rebind_nodemask)

Thank you for your reply.

First, home_node can only be set for VMA policy.

second, the result of cgroup migration is consistent with the result of the first e.g.

when process mem_allowed updates, vma policy will alse be updated.

Function call:

update_tasks_nodemask

      ->mpol_rebind_mm

          -> for_each_vma: mpol_rebind_policy

              -> mpol_rebind_nodemask

>
>> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
>> index 10a590ee1c89..9282be2ae18e 100644
>> --- a/mm/mempolicy.c
>> +++ b/mm/mempolicy.c
>> @@ -1536,6 +1536,12 @@ SYSCALL_DEFINE4(set_mempolicy_home_node, unsigned long, start, unsigned long, le
>>   			err = -EOPNOTSUPP;
>>   			break;
>>   		}
>> +
>> +		if (!node_isset(home_node, old->nodes)) {
>> +			err = -EINVAL;
>> +			break;
>> +		}
>> +
>>   		new = mpol_dup(old);
>>   		if (IS_ERR(new)) {
>>   			err = PTR_ERR(new);
>> -- 
>> 2.43.0
>>
>>

[-- Attachment #2: Type: text/html, Size: 3004 bytes --]

Re: [PATCH] mempolicy: check home_node is in the nodes of policy

[Chunsheng Luo]

[-- Attachment #1: Type: text/plain, Size: 2434 bytes --]


On 2024/1/30 10:29, Chunsheng Luo wrote:
>
>
> On 2024/1/29 23:12, Gregory Price wrote:
>> On Fri, Jan 26, 2024 at 08:22:40AM -0500, Chunsheng Luo wrote:
>>> set_mempolicy_home_node should be used after setting the memory
>>> policy. If the home_node isn't in the nodes of policy, we should
>>> return failure to avoid misunderstanding.
>>>
>>> Signed-off-by: Chunsheng Luo<luochunsheng@ustc.edu>
>>> ---
>>>   mm/mempolicy.c | 6 ++++++
>>>   1 file changed, 6 insertions(+)
>>>
>> Since it's not possible to add/remove a node to a mask without also
>> erasing the home node, this seems reasonable.
>>
>> e.g. this is what happens presently
>> mbind(0-2)    :   mask(0,1,2),   home_node(NUMA_NO_NODE)
>> home_node(3)  :   mask(0,1,2),   home_node(3)
>> mbind(0-3)    :   mask(0,1,2,3), home_node(NUMA_NO_NODE)
>>
>> However, it is possible for a cgroup migration or a change to
>> cpusets.mems_allowed to change a nodemask without somping the home_node.
>>
>> e.g.:
>> mbind(2-3)    :   mask(2-3),  home_node(NUMA_NO_NODE)
>> home_node(3)  :   mask(2-3),  home_node(3)
>> cpusets(0-1)  :   mask(0-1),  home_node(3)
>>
>> Should the rebind code also shift the home-node or un-set it accordingly
>> to keep the mask/home_node behavior consistent with the syscalls?
>>
>> (see mpol_rebind_nodemask)
> Thank you for your reply.
> First, home_node can only be set for VMA policy.
> second, the result of cgroup migration is consistent with the result of the first e.g.
> when process mem_allowed updates, vma policy will alse be updated.
> Function call:
> update_tasks_nodemask
>       ->mpol_rebind_mm
>           -> for_each_vma: mpol_rebind_policy
>               -> mpol_rebind_nodemask
Sorry, I make a mistake. The mpol_rebind_nodemask() only modifies the 
nodemask of the vma policy,
but doesn't modify the home_node, so the behavior is different from 
do_mbind(), so you are right.
>>> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
>>> index 10a590ee1c89..9282be2ae18e 100644
>>> --- a/mm/mempolicy.c
>>> +++ b/mm/mempolicy.c
>>> @@ -1536,6 +1536,12 @@ SYSCALL_DEFINE4(set_mempolicy_home_node, unsigned long, start, unsigned long, le
>>>   			err = -EOPNOTSUPP;
>>>   			break;
>>>   		}
>>> +
>>> +		if (!node_isset(home_node, old->nodes)) {
>>> +			err = -EINVAL;
>>> +			break;
>>> +		}
>>> +
>>>   		new = mpol_dup(old);
>>>   		if (IS_ERR(new)) {
>>>   			err = PTR_ERR(new);
>>> -- 
>>> 2.43.0
>>>
>>>

[-- Attachment #2: Type: text/html, Size: 3661 bytes --]