On 12/08, Peter Zijlstra wrote: > > On Tue, 2009-12-08 at 16:04 +0100, Oleg Nesterov wrote: > > > > Well, this is subjective, but I don't agree that > > > > get_task_struct(task); > > task->utrace_flags = flags; > > spin_unlock(&utrace->lock); > > put_task_struct(task); > > > > looks better. > > No, what I mean by assymetric locking is that utrace_reset() and > utrace_reap() drop the utrace->lock where their caller acquired it, > resulting in non-obvious like: > > utrace_control() > { > > ... > spin_lock(&utrace->lock); > > ... > > if (reset) > utrace_reset(utrace); > else > spin_unlock(&utrace->lock); > }
Agreed, the code like this never looks good. > If you take a task ref you can write the much saner: > > utrace_control() > { > ... > spin_lock(&utrace->lock); > ... > if (reset) > utrace_reset(utrace); > > spin_unlock(&utrace->lock); > } No, get_task_struct() in utrace_reset() can't help, we should move it into utrace_control() then. And in this case it becomes even more subtle: it is needed because ->utrace_flags may be cleared inside utrace_reset() and after that utrace_control()->spin_unlock() becomes unsafe. Also. utrace_reset() drops utrace->lock to call put_detached_list() lockless. If we want to avoid the assymetric locking, every caller should pass "struct list_head *detached" to utrace_reset(), drop utrace->lock, and call put_detached_list(). Oleg.