[PATCH] generic-ipi: add lock context annotations

[PATCH] generic-ipi: add lock context annotations

[Namhyung Kim]

The ipi_call_[un]lock[_irq] functions grab/release a spin lock
but were missing proper annotations. Add it.

Signed-off-by: Namhyung Kim <namhyung@gmail.com>
---
 kernel/smp.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/kernel/smp.c b/kernel/smp.c
index 12ed8b0..5a62f1f 100644
--- a/kernel/smp.c
+++ b/kernel/smp.c
@@ -511,21 +511,25 @@ int smp_call_function(smp_call_func_t func, void *info, int wait)
 EXPORT_SYMBOL(smp_call_function);
 
 void ipi_call_lock(void)
+	__acquires(call_function.lock)
 {
 	raw_spin_lock(&call_function.lock);
 }
 
 void ipi_call_unlock(void)
+	__releases(call_function.lock)
 {
 	raw_spin_unlock(&call_function.lock);
 }
 
 void ipi_call_lock_irq(void)
+	__acquires(call_function.lock)
 {
 	raw_spin_lock_irq(&call_function.lock);
 }
 
 void ipi_call_unlock_irq(void)
+	__releases(call_function.lock)
 {
 	raw_spin_unlock_irq(&call_function.lock);
 }
-- 
1.7.0.4

Re: [PATCH] generic-ipi: add lock context annotations

[Peter Zijlstra]

On Mon, 2010-11-22 at 16:33 +0900, Namhyung Kim wrote:
> The ipi_call_[un]lock[_irq] functions grab/release a spin lock
> but were missing proper annotations. Add it.

I really have to ask why bother? Why not add some smarts to whatever
uses these annotations?

> Signed-off-by: Namhyung Kim <namhyung@gmail.com>
> ---
>  kernel/smp.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/kernel/smp.c b/kernel/smp.c
> index 12ed8b0..5a62f1f 100644
> --- a/kernel/smp.c
> +++ b/kernel/smp.c
> @@ -511,21 +511,25 @@ int smp_call_function(smp_call_func_t func, void *info, int wait)
>  EXPORT_SYMBOL(smp_call_function);
>  
>  void ipi_call_lock(void)
> +	__acquires(call_function.lock)
>  {
>  	raw_spin_lock(&call_function.lock);
>  }
>  
>  void ipi_call_unlock(void)
> +	__releases(call_function.lock)
>  {
>  	raw_spin_unlock(&call_function.lock);
>  }
>  
>  void ipi_call_lock_irq(void)
> +	__acquires(call_function.lock)
>  {
>  	raw_spin_lock_irq(&call_function.lock);
>  }
>  
>  void ipi_call_unlock_irq(void)
> +	__releases(call_function.lock)
>  {
>  	raw_spin_unlock_irq(&call_function.lock);
>  }

Re: [PATCH] generic-ipi: add lock context annotations

[Namhyung Kim]

2010-11-22 (월), 13:46 +0100, Peter Zijlstra:
> On Mon, 2010-11-22 at 16:33 +0900, Namhyung Kim wrote:
> > The ipi_call_[un]lock[_irq] functions grab/release a spin lock
> > but were missing proper annotations. Add it.
> 
> I really have to ask why bother? Why not add some smarts to whatever
> uses these annotations?

I just thought that removing bogus warnings from sparse helps us focus
on real issues when using it. Currently sparse emits too many messages
and some (many?) of them might be removed trivially (or by adding bit of
ugliness. :( )

BTW, I didn't get what you mean about "some smarts". Could you explain
them little more?

Thanks.

-- 
Regards,
Namhyung Kim

Re: [PATCH] generic-ipi: add lock context annotations

[Jens Axboe]

On 2010-11-23 04:19, Namhyung Kim wrote:
> 2010-11-22 (월), 13:46 +0100, Peter Zijlstra:
>> On Mon, 2010-11-22 at 16:33 +0900, Namhyung Kim wrote:
>>> The ipi_call_[un]lock[_irq] functions grab/release a spin lock
>>> but were missing proper annotations. Add it.
>>
>> I really have to ask why bother? Why not add some smarts to whatever
>> uses these annotations?
> 
> I just thought that removing bogus warnings from sparse helps us focus
> on real issues when using it. Currently sparse emits too many messages
> and some (many?) of them might be removed trivially (or by adding bit of
> ugliness. :( )

It's not too big a deal, I have no problem adding the annotation.

> BTW, I didn't get what you mean about "some smarts". Could you explain
> them little more?

I guess what Peter means is that the fact that the function grabs the
lock is apparent in the code, if sparse was a bit "smarter", it would
see and note this itself.

-- 
Jens Axboe

Re: [PATCH] generic-ipi: add lock context annotations

[Dan Carpenter]

On Tue, Nov 23, 2010 at 09:49:56AM +0100, Jens Axboe wrote:
> I guess what Peter means is that the fact that the function grabs the
> lock is apparent in the code, if sparse was a bit "smarter", it would
> see and note this itself.

The sparse warning messages should have been included in the commit
message.  Here they are:

kernel/smp.c:513:6: warning: context imbalance in 'ipi_call_lock' - wrong count at exit
kernel/smp.c:518:6: warning: context imbalance in 'ipi_call_unlock' - unexpected unlock
kernel/smp.c:523:6: warning: context imbalance in 'ipi_call_lock_irq' - wrong count at exit
kernel/smp.c:528:6: warning: context imbalance in 'ipi_call_unlock_irq' - unexpected unlock

What happens is that sparse *does* see that the locks are unlocked and
that's what it complains about.

Sparse works before the code is linked so this change doesn't affect
anything outside kernel/smp.c.  If we added these annotations to
include/linux/smp.h then they would be used for checking
kernel/smpboot.c

regards,
dan carpenter

Re: [PATCH] generic-ipi: add lock context annotations

[Namhyung Kim]

2010-11-23 (화), 12:40 +0300, Dan Carpenter: 
> On Tue, Nov 23, 2010 at 09:49:56AM +0100, Jens Axboe wrote:
> > I guess what Peter means is that the fact that the function grabs the
> > lock is apparent in the code, if sparse was a bit "smarter", it would
> > see and note this itself.
> 
> The sparse warning messages should have been included in the commit
> message.  Here they are:
> 
> kernel/smp.c:513:6: warning: context imbalance in 'ipi_call_lock' - wrong count at exit
> kernel/smp.c:518:6: warning: context imbalance in 'ipi_call_unlock' - unexpected unlock
> kernel/smp.c:523:6: warning: context imbalance in 'ipi_call_lock_irq' - wrong count at exit
> kernel/smp.c:528:6: warning: context imbalance in 'ipi_call_unlock_irq' - unexpected unlock
> 
> What happens is that sparse *does* see that the locks are unlocked and
> that's what it complains about.
> 
> Sparse works before the code is linked so this change doesn't affect
> anything outside kernel/smp.c.  If we added these annotations to
> include/linux/smp.h then they would be used for checking
> kernel/smpboot.c
> 
> regards,
> dan carpenter
> 

I tried to annotate declations in include/linux/smp.h but it didn't work
well. Maybe that's what we need to fix the sparse?

Anyway, Jens, do you want new patch which includes above warnings?

Thanks.

-- 
Regards,
Namhyung Kim

Re: [PATCH] generic-ipi: add lock context annotations

[Dan Carpenter]

On Wed, Nov 24, 2010 at 02:24:40PM +0900, Namhyung Kim wrote:
> 2010-11-23 (화), 12:40 +0300, Dan Carpenter: 
> > On Tue, Nov 23, 2010 at 09:49:56AM +0100, Jens Axboe wrote:
> 
> I tried to annotate declations in include/linux/smp.h but it didn't work
> well. Maybe that's what we need to fix the sparse?
> 

It worked for me earlier when I tested it.  Just add the exact same
annotations that you added to the .c file.

regards,
dan carpenter

Re: [PATCH] generic-ipi: add lock context annotations

[Namhyung Kim]

2010-11-24 (수), 08:43 +0300, Dan Carpenter:
> On Wed, Nov 24, 2010 at 02:24:40PM +0900, Namhyung Kim wrote:
> > 2010-11-23 (화), 12:40 +0300, Dan Carpenter: 
> > > On Tue, Nov 23, 2010 at 09:49:56AM +0100, Jens Axboe wrote:
> > 
> > I tried to annotate declations in include/linux/smp.h but it didn't work
> > well. Maybe that's what we need to fix the sparse?
> > 
> 
> It worked for me earlier when I tested it.  Just add the exact same
> annotations that you added to the .c file.
> 
> regards,
> dan carpenter
> 

OK. You mean annotating both .c and .h, right? Will send v2 soon.

Thanks.

-- 
Regards,
Namhyung Kim

Re: [PATCH] generic-ipi: add lock context annotations

[Peter Zijlstra]

On Wed, 2010-11-24 at 23:29 +0900, Namhyung Kim wrote:
> 2010-11-24 (수), 08:43 +0300, Dan Carpenter:
> > On Wed, Nov 24, 2010 at 02:24:40PM +0900, Namhyung Kim wrote:
> > > 2010-11-23 (화), 12:40 +0300, Dan Carpenter: 
> > > > On Tue, Nov 23, 2010 at 09:49:56AM +0100, Jens Axboe wrote:
> > > 
> > > I tried to annotate declations in include/linux/smp.h but it didn't work
> > > well. Maybe that's what we need to fix the sparse?
> > > 
> > 
> > It worked for me earlier when I tested it.  Just add the exact same
> > annotations that you added to the .c file.
> > 
> > regards,
> > dan carpenter
> > 
> 
> OK. You mean annotating both .c and .h, right? Will send v2 soon.

Aside from complain, do these sparse annotations ever catch a bug?

I don't particularly like the __acquire() and __release() tags, but
could possibly live with them when they only need to be in headers, but
the __cond_lock() crap is just revolting.

Re: [PATCH] generic-ipi: add lock context annotations

[Namhyung Kim]

2010-11-24 (수), 15:33 +0100, Peter Zijlstra:
> Aside from complain, do these sparse annotations ever catch a bug?
> 

Dunno, sorry. I only have very limited experience of kernel development.

> I don't particularly like the __acquire() and __release() tags, but
> could possibly live with them when they only need to be in headers, but
> the __cond_lock() crap is just revolting.

Yes, it's very ugly. But some people told me it's a better way to
describe conditional lock acquisition from complicated functions. It
helped sparse recognize normal usage of such functions and suppress
warnings but only warn themselves.


-- 
Regards,
Namhyung Kim