The enable path uses three big locks - scx_fork_rwsem, scx_cgroup_rwsem and cpus_read_lock. Currently, the locks are grabbed together which is prone to locking order problems. For example, currently, there is a possible deadlock involving scx_fork_rwsem and cpus_read_lock. cpus_read_lock has to nest inside scx_fork_rwsem due to locking order existing in other subsystems. However, there exists a dependency in the other direction during hotplug if hotplug needs to fork a new task, which happens in some cases. This leads to the following deadlock: scx_ops_enable() hotplug percpu_down_write(&cpu_hotplug_lock) percpu_down_write(&scx_fork_rwsem) block on cpu_hotplug_lock kthread_create() waits for kthreadd kthreadd blocks on scx_fork_rwsem Note that this doesn't trigger lockdep because the hotplug side dependency bounces through kthreadd. With the preceding scx_cgroup_enabled change, this can be solved by decoupling cpus_read_lock, which is needed for static_key manipulations, from the other two locks. - Move the first block of static_key manipulations outside of scx_fork_rwsem and scx_cgroup_rwsem. This is now safe with the preceding scx_cgroup_enabled change. - Drop scx_cgroup_rwsem and scx_fork_rwsem between the two task iteration blocks so that __scx_ops_enabled static_key enabling is outside the two rwsems.
Showing the changes to kernel (1 of 1 file). Show all
Unified · Split
| @@ -5049,7 +5049,8 @@ static int scx_ops_enable(struct sched_ext_ops *ops, struct bpf_link *link) · 5048 unmodified lines | ||
| 5049 | 5049 | ret = SCX_CALL_OP_RET(SCX_KF_UNLOCKED, init); |
| 5050 | 5050 | if (ret) { |
| 5051 | 5051 | ret = ops_sanitize_err("init", ret); |
| 5052 | − goto err_disable_unlock_cpus; | |
| 5052 | + cpus_read_unlock(); | |
| 5053 | + goto err_disable; | |
| 5053 | 5054 | } |
| 5054 | 5055 | } |
| 5055 | 5056 | |
| @@ -5092,54 +5093,30 @@ static int scx_ops_enable(struct sched_ext_ops *ops, struct bpf_link *link) · 36 unmodified lines | ||
| 5092 | 5093 | */ |
| 5093 | 5094 | scx_ops_bypass(true); |
| 5094 | 5095 | |
| 5095 | − /* | |
| 5096 | − * Lock out forks, cgroup on/offlining and moves before opening the | |
| 5097 | − * floodgate so that they don't wander into the operations prematurely. | |
| 5098 | − * | |
| 5099 | − * We don't need to keep the CPUs stable but static_branch_*() requires | |
| 5100 | − * cpus_read_lock() and scx_cgroup_rwsem must nest inside | |
| 5101 | − * cpu_hotplug_lock because of the following dependency chain: | |
| 5102 | − * | |
| 5103 | − * cpu_hotplug_lock --> cgroup_threadgroup_rwsem --> scx_cgroup_rwsem | |
| 5104 | − * | |
| 5105 | − * So, we need to do cpus_read_lock() before scx_cgroup_lock() and use | |
| 5106 | − * static_branch_*_cpuslocked(). | |
| 5107 | − * | |
| 5108 | − * Note that cpu_hotplug_lock must nest inside scx_fork_rwsem due to the | |
| 5109 | − * following dependency chain: | |
| 5110 | − * | |
| 5111 | − * scx_fork_rwsem --> pernet_ops_rwsem --> cpu_hotplug_lock | |
| 5112 | − */ | |
| 5113 | − percpu_down_write(&scx_fork_rwsem); | |
| 5114 | − cpus_read_lock(); | |
| 5115 | − scx_cgroup_lock(); | |
| 5116 | − | |
| 5117 | 5096 | for (i = SCX_OPI_NORMAL_BEGIN; i < SCX_OPI_NORMAL_END; i++) |
| 5118 | 5097 | if (((void (**)(void))ops)[i]) |
| 5119 | − static_branch_enable_cpuslocked(&scx_has_op[i]); | |
| 5098 | + static_branch_enable(&scx_has_op[i]); | |
| 5120 | 5099 | |
| 5121 | 5100 | if (ops->flags & SCX_OPS_ENQ_LAST) |
| 5122 | − static_branch_enable_cpuslocked(&scx_ops_enq_last); | |
| 5101 | + static_branch_enable(&scx_ops_enq_last); | |
| 5123 | 5102 | |
| 5124 | 5103 | if (ops->flags & SCX_OPS_ENQ_EXITING) |
| 5125 | − static_branch_enable_cpuslocked(&scx_ops_enq_exiting); | |
| 5104 | + static_branch_enable(&scx_ops_enq_exiting); | |
| 5126 | 5105 | if (scx_ops.cpu_acquire || scx_ops.cpu_release) |
| 5127 | − static_branch_enable_cpuslocked(&scx_ops_cpu_preempt); | |
| 5106 | + static_branch_enable(&scx_ops_cpu_preempt); | |
| 5128 | 5107 | |
| 5129 | 5108 | if (!ops->update_idle || (ops->flags & SCX_OPS_KEEP_BUILTIN_IDLE)) { |
| 5130 | 5109 | reset_idle_masks(); |
| 5131 | − static_branch_enable_cpuslocked(&scx_builtin_idle_enabled); | |
| 5110 | + static_branch_enable(&scx_builtin_idle_enabled); | |
| 5132 | 5111 | } else { |
| 5133 | − static_branch_disable_cpuslocked(&scx_builtin_idle_enabled); | |
| 5112 | + static_branch_disable(&scx_builtin_idle_enabled); | |
| 5134 | 5113 | } |
| 5135 | 5114 | |
| 5136 | 5115 | /* |
| 5137 | − * All cgroups should be initialized before letting in tasks. cgroup | |
| 5138 | − * on/offlining and task migrations are already locked out. | |
| 5116 | + * Lock out forks, cgroup on/offlining and moves before opening the | |
| 5117 | + * floodgate so that they don't wander into the operations prematurely. | |
| 5139 | 5118 | */ |
| 5140 | − ret = scx_cgroup_init(); | |
| 5141 | − if (ret) | |
| 5142 | − goto err_disable_unlock_all; | |
| 5119 | + percpu_down_write(&scx_fork_rwsem); | |
| 5143 | 5120 | |
| 5144 | 5121 | WARN_ON_ONCE(scx_ops_init_task_enabled); |
| 5145 | 5122 | scx_ops_init_task_enabled = true; |
| @@ -5150,7 +5127,18 @@ static int scx_ops_enable(struct sched_ext_ops *ops, struct bpf_link *link) · 4 unmodified lines | ||
| 5150 | 5127 | * leaving as sched_ext_free() can handle both prepped and enabled |
| 5151 | 5128 | * tasks. Prep all tasks first and then enable them with preemption |
| 5152 | 5129 | * disabled. |
| 5130 | + * | |
| 5131 | + * All cgroups should be initialized before scx_ops_init_task() so that | |
| 5132 | + * the BPF scheduler can reliably track each task's cgroup membership | |
| 5133 | + * from scx_ops_init_task(). Lock out cgroup on/offlining and task | |
| 5134 | + * migrations while tasks are being initialized so that | |
| 5135 | + * scx_cgroup_can_attach() never sees uninitialized tasks. | |
| 5153 | 5136 | */ |
| 5137 | + scx_cgroup_lock(); | |
| 5138 | + ret = scx_cgroup_init(); | |
| 5139 | + if (ret) | |
| 5140 | + goto err_disable_unlock_all; | |
| 5141 | + | |
| 5154 | 5142 | spin_lock_irq(&scx_tasks_lock); |
| 5155 | 5143 | scx_task_iter_init(&sti); |
| 5156 | 5144 | while ((p = scx_task_iter_next_locked(&sti))) { |
| @@ -5183,19 +5171,22 @@ static int scx_ops_enable(struct sched_ext_ops *ops, struct bpf_link *link) · 26 unmodified lines | ||
| 5183 | 5171 | } |
| 5184 | 5172 | scx_task_iter_exit(&sti); |
| 5185 | 5173 | spin_unlock_irq(&scx_tasks_lock); |
| 5174 | + scx_cgroup_unlock(); | |
| 5175 | + percpu_up_write(&scx_fork_rwsem); | |
| 5186 | 5176 | |
| 5187 | 5177 | /* |
| 5188 | 5178 | * All tasks are READY. It's safe to turn on scx_enabled() and switch |
| 5189 | 5179 | * all eligible tasks. |
| 5190 | 5180 | */ |
| 5191 | 5181 | WRITE_ONCE(scx_switching_all, !(ops->flags & SCX_OPS_SWITCH_PARTIAL)); |
| 5192 | − static_branch_enable_cpuslocked(&__scx_ops_enabled); | |
| 5182 | + static_branch_enable(&__scx_ops_enabled); | |
| 5193 | 5183 | |
| 5194 | 5184 | /* |
| 5195 | 5185 | * We're fully committed and can't fail. The task READY -> ENABLED |
| 5196 | 5186 | * transitions here are synchronized against sched_ext_free() through |
| 5197 | 5187 | * scx_tasks_lock. |
| 5198 | 5188 | */ |
| 5189 | + percpu_down_write(&scx_fork_rwsem); | |
| 5199 | 5190 | spin_lock_irq(&scx_tasks_lock); |
| 5200 | 5191 | scx_task_iter_init(&sti); |
| 5201 | 5192 | while ((p = scx_task_iter_next_locked(&sti))) { |
| @@ -5213,10 +5204,8 @@ static int scx_ops_enable(struct sched_ext_ops *ops, struct bpf_link *link) · 11 unmodified lines | ||
| 5213 | 5204 | } |
| 5214 | 5205 | scx_task_iter_exit(&sti); |
| 5215 | 5206 | spin_unlock_irq(&scx_tasks_lock); |
| 5216 | − | |
| 5217 | − scx_cgroup_unlock(); | |
| 5218 | − cpus_read_unlock(); | |
| 5219 | 5207 | percpu_up_write(&scx_fork_rwsem); |
| 5208 | + | |
| 5220 | 5209 | scx_ops_bypass(false); |
| 5221 | 5210 | |
| 5222 | 5211 | /* |
| @@ -5259,8 +5248,6 @@ err_disable_unlock_all: · 36 unmodified lines | ||
| 5259 | 5248 | scx_cgroup_unlock(); |
| 5260 | 5249 | percpu_up_write(&scx_fork_rwsem); |
| 5261 | 5250 | scx_ops_bypass(false); |
| 5262 | −err_disable_unlock_cpus: | |
| 5263 | − cpus_read_unlock(); | |
| 5264 | 5251 | err_disable: |
| 5265 | 5252 | mutex_unlock(&scx_ops_enable_mutex); |
| 5266 | 5253 | /* must be fully disabled before returning */ |