Messages in this thread | | | Date | Sat, 18 Jul 2009 13:19:49 +0200 | From | Ingo Molnar <> | Subject | Re: [PATCH] wireless: wl12xx, fix lock imbalance |
| |
* Johannes Berg <johannes@sipsolutions.net> wrote:
> On Mon, 2009-07-13 at 23:51 +0200, Jiri Slaby wrote: > > On 07/13/2009 11:49 PM, Johannes Berg wrote: > > > On Mon, 2009-07-13 at 23:44 +0200, Jiri Slaby wrote: > > > > > >>> I've had local hacks > > >>> many times to make sparse aware of mutexes, is there a reason they are > > >>> not annotated with __acquire(s)/__release(s) like spinlocks etc.? > > >> > > >> Mutexes are often locked/unlocked interprocedural which I think sparse > > >> can't do much about. > > > > > > Well, you annotate those functions too, of course. > > > > Sorry, I don't understand. What functions I annotate? > > Well those that take the mutex, e.g. > > void acquire_foo(struct foo *f) > { > mutex_lock(&f->mtx); > } > > > turns to > > void acquire_foo(struct foo *f) > __acquires(f->mtx) > { > mutex_lock(&f->mtx); > } > > johannes
Yes. And in fact 'nice' code wants to be either annotated explicitly as 'I am taking locks', or should be balanced.
I was thinking about also using lockdep plus the function-graph tracer for that (in the dynamic lock debugging department).
It would work like this: __acquires()/__releases() would also emit section markers like __lockfunc, and lockdep would warn about functions that return with unbalanced locks, irqs or preempt counts and do not declare themselves as locking related functions.
This would help catch imbalances at their source.
Plus static tools like Jiri is working on are very useful as well. I think Coverty does that too and it's a pity we dont have free tools for that. In fact Covery will sweep clean the kernel of such bugs, giving OSS tools like 'stanse' the false impression that there are no such bugs. There are such bugs - there's a constant influx of them. So please work on this, it looks very useful.
Ingo
| |