Messages in this thread | | | Subject | Re: [RFC,PATCH 14/14] utrace core | From | Peter Zijlstra <> | Date | Tue, 08 Dec 2009 16:29:23 +0100 |
| |
On Tue, 2009-12-08 at 16:04 +0100, Oleg Nesterov wrote: > > > > > + /* > > > > > + * In theory spin_lock() doesn't imply rcu_read_lock(). > > > > > + * Once we clear ->utrace_flags this task_struct can go away > > > > > + * because tracehook_prepare_release_task() path does not take > > > > > + * utrace->lock when ->utrace_flags == 0. > > > > > + */ > > > > > + rcu_read_lock(); > > > > > + task->utrace_flags = flags; > > > > > + spin_unlock(&utrace->lock); > > > > > + rcu_read_unlock(); > > > > > > > > yuck! > > > > > > > > why not simply keep a task reference over the utrace_reset call? > > > > > > Yes, we could use get_task_struct() instead. Not sure this would > > > be more clean, though. > > > > For one it would allow getting rid of that insane assymetric locking. > > 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); }
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); }
| |