(2/7/12 1:23 AM), Anton Vorontsov wrote:
On Tue, Feb 07, 2012 at 12:38:58AM -0500, KOSAKI Motohiro wrote:
task struct only have allock_lock, not alloc_loc.
Funnily, but sparse does not care. :-) __release(foo) will work as well. Seems like sparse counts locking balance globally.
This is now fixed in the patch down below, thanks for catching.
Moreover we don't release the lock in this code path. Seems odd.
Indeed. That's exactly what sparse seeing is as well. We exit without releasing the lock, which is bad (in sparse' eyes). So we lie to sparse, telling it that we do release, so it shut ups.
Hmmm....
To be honest, I really dislike any lie annotation. Why? It is very fragile and easily become broken from unrelated but near line changes. Please consider to enhance sparse at first.
I somewhat doubt that it is possible to "enhance it". We keep the lock held conditionaly, so we need too place the hint in the code itself. I believe the best we can do would be something like this:
diff --git a/include/linux/compiler.h b/include/linux/compiler.h index 4a24354..61d91f2 100644 --- a/include/linux/compiler.h +++ b/include/linux/compiler.h @@ -14,6 +14,7 @@ # define __releases(x) __attribute__((context(x,1,0))) # define __acquire(x) __context__(x,1) # define __release(x) __context__(x,-1) +# define __ret_with_lock(x) __context__(x,-1) # define __cond_lock(x,c) ((c) ? ({ __acquire(x); 1; }) : 0) # define __percpu __attribute__((noderef, address_space(3))) #ifdef CONFIG_SPARSE_RCU_POINTER @@ -37,6 +38,7 @@ extern void __chk_io_ptr(const volatile void __iomem *); # define __releases(x) # define __acquire(x) (void)0 # define __release(x) (void)0 +# define __ret_with_lock(x) # define __cond_lock(x,c) (c) # define __percpu # define __rcu
And then use it instead of __release().
Hmmm...
I still dislike this lie annotation. But maybe it's better than nothing (dunno, just guess). Go ahead.