Messages in this thread | | | Date | Tue, 8 Dec 2009 17:31:31 +0100 | From | Oleg Nesterov <> | Subject | Re: [RFC,PATCH 14/14] utrace core |
| |
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.
| |