On 10/28, Kirill Tkhai wrote: > > Shouldn't we do that in separate patch? How about this?
Up to Peter, but I think a separate patch is fine. > [PATCH]sched: Remove lockdep check in sched_move_task() > > sched_move_task() is the only interface to change sched_task_group: > cpu_cgrp_subsys methods and autogroup_move_group() use it. Yes, but... > Everything is synchronized by task_rq_lock(), so cpu_cgroup_attach() > is ordered with other users of sched_move_task(). This means we do > no need RCU here: if we've dereferenced a tg here, the .attach method > hasn't been called for it yet. > > Thus, we should pass "true" to task_css_check() to silence lockdep > warnings. In theory, I am not sure. However, I never really understood this code and today I forgot everything, please correct me. > @@ -7403,8 +7403,12 @@ void sched_move_task(struct task_struct *tsk) > if (unlikely(running)) > put_prev_task(rq, tsk); > > - tg = container_of(task_css_check(tsk, cpu_cgrp_id, > - lockdep_is_held(&tsk->sighand->siglock)), > + /* > + * All callers are synchronized by task_rq_lock(); we do not use RCU > + * which is pointless here. Thus, we pass "true" to task_css_check() > + * to prevent lockdep warnings. > + */ > + tg = container_of(task_css_check(tsk, cpu_cgrp_id, true), > struct task_group, css); Why this can't race with cgroup_task_migrate() if it is called by cgroup_post_fork() ? And cgroup_task_migrate() can free ->cgroups via call_rcu(). Of course, in practice raw_spin_lock_irq() should also act as rcu_read_lock(), but we should not rely on implementation details. task_group = tsk->cgroups[cpu_cgrp_id] can't go away because yes, if we race with migrate then ->attach() was not called. But it seems that in theory it is not safe to dereference tsk->cgroups. Oleg. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
