On 2024/12/20 23:13, Waiman Long wrote: > On 12/20/24 1:11 AM, Chen Ridong wrote: >> >> On 2024/12/20 12:16, Waiman Long wrote: >>> On 12/19/24 11:07 PM, chenridong wrote: >>>> On 2024/12/20 10:55, Waiman Long wrote: >>>>> On 12/19/24 8:31 PM, Chen Ridong wrote: >>>>>> From: Chen Ridong <chenridong@xxxxxxxxxx> >>>>>> >>>>>> A warning was found: >>>>>> >>>>>> WARNING: CPU: 10 PID: 3486953 at fs/kernfs/file.c:828 >>>>>> CPU: 10 PID: 3486953 Comm: rmdir Kdump: loaded Tainted: G >>>>>> RIP: 0010:kernfs_should_drain_open_files+0x1a1/0x1b0 >>>>>> RSP: 0018:ffff8881107ef9e0 EFLAGS: 00010202 >>>>>> RAX: 0000000080000002 RBX: ffff888154738c00 RCX: dffffc0000000000 >>>>>> RDX: 0000000000000007 RSI: 0000000000000004 RDI: ffff888154738c04 >>>>>> RBP: ffff888154738c04 R08: ffffffffaf27fa15 R09: ffffed102a8e7180 >>>>>> R10: ffff888154738c07 R11: 0000000000000000 R12: ffff888154738c08 >>>>>> R13: ffff888750f8c000 R14: ffff888750f8c0e8 R15: ffff888154738ca0 >>>>>> FS: 00007f84cd0be740(0000) GS:ffff8887ddc00000(0000) >>>>>> knlGS:0000000000000000 >>>>>> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 >>>>>> CR2: 0000555f9fbe00c8 CR3: 0000000153eec001 CR4: 0000000000370ee0 >>>>>> DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000 >>>>>> DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400 >>>>>> Call Trace: >>>>>> kernfs_drain+0x15e/0x2f0 >>>>>> __kernfs_remove+0x165/0x300 >>>>>> kernfs_remove_by_name_ns+0x7b/0xc0 >>>>>> cgroup_rm_file+0x154/0x1c0 >>>>>> cgroup_addrm_files+0x1c2/0x1f0 >>>>>> css_clear_dir+0x77/0x110 >>>>>> kill_css+0x4c/0x1b0 >>>>>> cgroup_destroy_locked+0x194/0x380 >>>>>> cgroup_rmdir+0x2a/0x140 >>>>> Were you using cgroup v1 or v2 when this warning happened? >>>> I was using cgroup v1. >>> Thanks for the confirmation. >>>>>> It can be explained by: >>>>>> rmdir echo 1 > cpuset.cpus >>>>>> kernfs_fop_write_iter // active=0 >>>>>> cgroup_rm_file >>>>>> kernfs_remove_by_name_ns kernfs_get_active // active=1 >>>>>> __kernfs_remove // active=0x80000002 >>>>>> kernfs_drain cpuset_write_resmask >>>>>> wait_event >>>>>> //waiting (active == 0x80000001) >>>>>> kernfs_break_active_protection >>>>>> // active = 0x80000001 >>>>>> // continue >>>>>> kernfs_unbreak_active_protection >>>>>> // active = 0x80000002 >>>>>> ... >>>>>> kernfs_should_drain_open_files >>>>>> // warning occurs >>>>>> kernfs_put_active >>>>>> >>>>>> This warning is caused by 'kernfs_break_active_protection' when it is >>>>>> writing to cpuset.cpus, and the cgroup is removed concurrently. >>>>>> >>>>>> The commit 3a5a6d0c2b03 ("cpuset: don't nest cgroup_mutex inside >>>>>> get_online_cpus()") made cpuset_hotplug_workfn asynchronous, which >>>>>> grabs >>>>>> the cgroup_mutex. To avoid deadlock. the commit 76bb5ab8f6e3 >>>>>> ("cpuset: >>>>>> break kernfs active protection in cpuset_write_resmask()") added >>>>>> 'kernfs_break_active_protection' in the cpuset_write_resmask. This >>>>>> could >>>>>> lead to this warning. >>>>>> >>>>>> After the commit 2125c0034c5d ("cgroup/cpuset: Make cpuset hotplug >>>>>> processing synchronous"), the cpuset_write_resmask no longer needs to >>>>>> wait the hotplug to finish, which means that cpuset_write_resmask >>>>>> won't >>>>>> grab the cgroup_mutex. So the deadlock doesn't exist anymore. >>>>>> Therefore, >>>>>> remove kernfs_break_active_protection operation in the >>>>>> 'cpuset_write_resmask' >>>>> The hotplug operation itself is now being done synchronously, but task >>>>> transfer (cgroup_transfer_tasks()) because of lacking online CPUs is >>>>> still being done asynchronously. So kernfs_break_active_protection() >>>>> will still be needed for cgroup v1. >>>>> >>>>> Cheers, >>>>> Longman >>>>> >>>>> >>>> Thank you, Longman. >>>> IIUC, The commit 2125c0034c5d ("cgroup/cpuset: Make cpuset hotplug >>>> processing synchronous") deleted the 'flush_work(&cpuset_hotplug_work)' >>>> in the cpuset_write_resmask. And I do not see any process within the >>>> cpuset_write_resmask that will grab cgroup_mutex, except for >>>> 'flush_work(&cpuset_hotplug_work)'. >>>> >>>> Although cgroup_transfer_tasks() is asynchronous, the >>>> cpuset_write_resmask will not wait any work that will grab >>>> cgroup_mutex. >>>> Consequently, the deadlock does not exist anymore. >>>> >>>> Did I miss something? >>> Right. The flush_work() call is still needed for a different work >>> function. cpuset_write_resmask() will not need to grab cgroup_mutex, but >>> the asynchronously executed cgroup_transfer_tasks() will. I will work on >>> a patch to fix that issue. >>> >>> Cheers, >>> Longman >> If flush_work() is added back, this warning still exists. Do you have a >> idea to fix this warning? > > I was wrong. The flush_work() call isn't needed in this case and we > shouldn't need to break kernfs protection. However, your patch > description isn't quite right. > >> After the commit 2125c0034c5d ("cgroup/cpuset: Make cpuset hotplug >> processing synchronous"), the cpuset_write_resmask no longer needs to >> wait the hotplug to finish, which means that cpuset_write_resmask won't >> grab the cgroup_mutex. So the deadlock doesn't exist anymore. > > cpuset_write_resmask() never needs to grab the cgroup_mutex. The act of > calling flush_work() can create a multiple processes circular locking > dependency that involve cgroup_mutex which can cause a deadlock. After > making cpuset hotplug synchronous, concurrent hotplug and cpuset > operations are no longer possible. However, concurrent task transfer out > of a previously empty CPU cpuset and adding CPU back to that cpuset is > possible. This will result in what the comment said "keep removing tasks > added > after execution capability is restored". That should be rare though and > we should probably add a check in cgroup_transfer_tasks() to detect such > a case and break out of it. > > Cheers, > Longman Hi, Longman, sorry the confused message. Do you mean this patch is acceptable if I update the message? I don't think we need to add a check in the cgroup_transfer_tasks function. Because no process(except for writing cpuset.cpus, which has been reoved) will need 'kn->active' to involve cgroup_transfer_tasks now. Best regards, Ridong