Re: [RFC PATCH] cgroup/cpuset: fix a memory binding failure for cgroup v2

[Waiman Long <longman@redhat.com>]

On 4/21/22 18:22, Tejun Heo wrote:
> cc'ing Waiman and copying the whole body.
>
> Waiman, can you please take a look?
>
> Thanks.
>
> On Tue, Apr 19, 2022 at 10:09:58AM +0800, Feng Tang wrote:
>> We got report that setting cpuset.mems failed when the nodemask
>> contains a newly onlined memory node (not enumerated during boot)
>> for cgroup v2, while the binding succeeded for cgroup v1.
>>
>> The root cause is, for cgroup v2, when a new memory node is onlined,
>> top_cpuset's 'mem_allowed' is not updated with the new nodemask of
>> memory nodes, and the following setting memory nodemask will fail,
>> if the nodemask contains a new node.
>>
>> Fix it by updating top_cpuset.mems_allowed right after the
>> new memory node is onlined, just like v1.
>>
>> Signed-off-by: Feng Tang <feng.tang@intel.com>
>> ---
>> Very likely I missed some details here, but it looks strange that
>> the top_cpuset.mem_allowed is not updatd even after we onlined
>> several memory nodes after boot.
>>
>>   kernel/cgroup/cpuset.c | 3 +--
>>   1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/kernel/cgroup/cpuset.c b/kernel/cgroup/cpuset.c
>> index 9390bfd9f1cd..b97caaf16374 100644
>> --- a/kernel/cgroup/cpuset.c
>> +++ b/kernel/cgroup/cpuset.c
>> @@ -3314,8 +3314,7 @@ static void cpuset_hotplug_workfn(struct work_struct *work)
>>   	/* synchronize mems_allowed to N_MEMORY */
>>   	if (mems_updated) {
>>   		spin_lock_irq(&callback_lock);
>> -		if (!on_dfl)
>> -			top_cpuset.mems_allowed = new_mems;
>> +		top_cpuset.mems_allowed = new_mems;
>>   		top_cpuset.effective_mems = new_mems;
>>   		spin_unlock_irq(&callback_lock);
>>   		update_tasks_nodemask(&top_cpuset);
The on_dfl check was added by commit 7e88291beefb ("cpuset: make 
cs->{cpus, mems}_allowed as user-configured masks"). This is the 
expected behavior for cgroup v2 as we don't want to remove a node 
because it is hot-removed. However, I do see a problem in case we are 
adding a node that is not originally in top_cpuset.mems_allowed. We 
should be allowed to add the extra memory node. So something like

         if (!on_dfl)
                 top_cpuset.mems_allowed = new_mems;
         else if (!nodes_subset(new_mems, top_cpuset.mems_allowed))
                 nodes_or(top_cpuset.mems_allowed, 
top_cpuset.mems_allowed, new_mems);

For v2, top_cpuset.mems_allowed is set to node_possible_map in 
cpuset_bind(). Perhaps node_possible_map may not include all the nodes 
that are hot-pluggable.

I don't know if that is similar problem with cpu_possible_mask or not.

Cheers,
Longman