diff mbox

"unexpected unlock" when unlocking, conditional, lock in loop

Message ID 20121007194552.GA909@leaf (mailing list archive)
State Not Applicable, archived
Headers show

Commit Message

Josh Triplett Oct. 7, 2012, 7:45 p.m. UTC 
On Sun, Oct 07, 2012 at 07:49:25AM -0500, Ed Cashin wrote:
> On Oct 6, 2012, at 10:39 PM, Josh Triplett wrote:
> 
> > On Sat, Oct 06, 2012 at 08:56:57PM -0500, Ed Cashin wrote:
> ...
> >> OK.  From the sparse man page section on context, along with
> >> include/linux/compiler.h, it sounds like the way to do exactly that 
> >> would be something unusual:
> >> 
> >> int demofn(void) __attribute__((context(&lk,1,1)))
> >> 
> >> ... but using that in demo.c causes sparse to warn me that it's 
> >> ignoring that attribute, so I doubt that can be what you mean.
> > 
> > I did mean precisely that; I don't know why Sparse complains about that
> > syntax.
> 
> Maybe there's a header I need.  Searching with cscope and ctags for definitions of "context" doesn't seem to be the right kind of searching.
> 
> The complaint looks like:
> 
>   CC [M]  drivers/block/aoe/demo.o
> drivers/block/aoe/demo.c:9: warning: `context' attribute directive ignored
> drivers/block/aoe/demo.c:9: error: expected `,' or `;' before `{' token
> make[1]: *** [drivers/block/aoe/demo.o] Error 1
> make: *** [drivers/block/aoe/aoe.ko] Error 2

Oh, that complaint doesn't come from Sparse; that comes from GCC, since
GCC doesn't understand the context attribute.  Look at
include/linux/compiler.h; it has wrapper macros for the various Sparse
attributes, and defines them to nothing when not compiling with Sparse.
If you want to use the context attribute to denote a lock held through a
function, you need a patch like this:

From 0b862fc1a131a874d157420e9443f16a714596ef Mon Sep 17 00:00:00 2001
From: Josh Triplett <josh@joshtriplett.org>
Date: Sun, 7 Oct 2012 12:41:13 -0700
Subject: [PATCH] linux/compiler.h: Add __must_hold macro for functions called with a lock held

linux/compiler.h has macros to denote functions that acquire or release
locks, but not to denote functions called with a lock held that return
with the lock still held.  Add a __must_hold macro to cover this case.

Signed-off-by: Josh Triplett <josh@joshtriplett.org>
---
 include/linux/compiler.h |    2 ++
 1 file changed, 2 insertions(+)

--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Ed Cashin Oct. 7, 2012, 9:28 p.m. UTC | #1 
On Oct 7, 2012, at 3:45 PM, Josh Triplett wrote:

> On Sun, Oct 07, 2012 at 07:49:25AM -0500, Ed Cashin wrote:
...
>>  CC [M]  drivers/block/aoe/demo.o
>> drivers/block/aoe/demo.c:9: warning: `context' attribute directive ignored
>> drivers/block/aoe/demo.c:9: error: expected `,' or `;' before `{' token
>> make[1]: *** [drivers/block/aoe/demo.o] Error 1
>> make: *** [drivers/block/aoe/aoe.ko] Error 2
> 
> Oh, that complaint doesn't come from Sparse; that comes from GCC, since
> GCC doesn't understand the context attribute.  Look at
> include/linux/compiler.h; it has wrapper macros for the various Sparse
> attributes, and defines them to nothing when not compiling with Sparse.
> If you want to use the context attribute to denote a lock held through a
> function, you need a patch like this:
> 
> From 0b862fc1a131a874d157420e9443f16a714596ef Mon Sep 17 00:00:00 2001
> From: Josh Triplett <josh@joshtriplett.org>
> Date: Sun, 7 Oct 2012 12:41:13 -0700
> Subject: [PATCH] linux/compiler.h: Add __must_hold macro for functions called with a lock held

Ah.  OK.  So... would you like me to submit your patch to the LKML, or are you doing that?

It seems like a needed addition.

> linux/compiler.h has macros to denote functions that acquire or release
> locks, but not to denote functions called with a lock held that return
> with the lock still held.  Add a __must_hold macro to cover this case.
> 
> Signed-off-by: Josh Triplett <josh@joshtriplett.org>
> ---
> include/linux/compiler.h |    2 ++
> 1 file changed, 2 insertions(+)
> 
> diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> index f430e41..b121554 100644
> --- a/include/linux/compiler.h
> +++ b/include/linux/compiler.h
> @@ -10,6 +10,7 @@
> # define __force	__attribute__((force))
> # define __nocast	__attribute__((nocast))
> # define __iomem	__attribute__((noderef, address_space(2)))
> +# define __must_hold(x)	__attribute__((context(x,1,1)))
> # define __acquires(x)	__attribute__((context(x,0,1)))
> # define __releases(x)	__attribute__((context(x,1,0)))
> # define __acquire(x)	__context__(x,1)
> @@ -33,6 +34,7 @@ extern void __chk_io_ptr(const volatile void __iomem *);
> # define __chk_user_ptr(x) (void)0
> # define __chk_io_ptr(x) (void)0
> # define __builtin_warning(x, y...) (1)
> +# define __must_hold(x)
> # define __acquires(x)
> # define __releases(x)
> # define __acquire(x) (void)0
Josh Triplett Oct. 7, 2012, 11:30 p.m. UTC | #2 
On Sun, Oct 07, 2012 at 04:28:16PM -0500, Ed Cashin wrote:
> On Oct 7, 2012, at 3:45 PM, Josh Triplett wrote:
> 
> > On Sun, Oct 07, 2012 at 07:49:25AM -0500, Ed Cashin wrote:
> ...
> >>  CC [M]  drivers/block/aoe/demo.o
> >> drivers/block/aoe/demo.c:9: warning: `context' attribute directive ignored
> >> drivers/block/aoe/demo.c:9: error: expected `,' or `;' before `{' token
> >> make[1]: *** [drivers/block/aoe/demo.o] Error 1
> >> make: *** [drivers/block/aoe/aoe.ko] Error 2
> > 
> > Oh, that complaint doesn't come from Sparse; that comes from GCC, since
> > GCC doesn't understand the context attribute.  Look at
> > include/linux/compiler.h; it has wrapper macros for the various Sparse
> > attributes, and defines them to nothing when not compiling with Sparse.
> > If you want to use the context attribute to denote a lock held through a
> > function, you need a patch like this:
> > 
> > From 0b862fc1a131a874d157420e9443f16a714596ef Mon Sep 17 00:00:00 2001
> > From: Josh Triplett <josh@joshtriplett.org>
> > Date: Sun, 7 Oct 2012 12:41:13 -0700
> > Subject: [PATCH] linux/compiler.h: Add __must_hold macro for functions called with a lock held
> 
> Ah.  OK.  So... would you like me to submit your patch to the LKML, or are you doing that?
> 
> It seems like a needed addition.

If you can confirm that it works for you, I'll send it to LKML with a
Tested-by from you.

- Josh Triplett
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Ed Cashin Oct. 8, 2012, 12:35 a.m. UTC | #3 
On Oct 7, 2012, at 7:30 PM, Josh Triplett wrote:

> On Sun, Oct 07, 2012 at 04:28:16PM -0500, Ed Cashin wrote:
>> On Oct 7, 2012, at 3:45 PM, Josh Triplett wrote:
...
>>> From 0b862fc1a131a874d157420e9443f16a714596ef Mon Sep 17 00:00:00 2001
>>> From: Josh Triplett <josh@joshtriplett.org>
>>> Date: Sun, 7 Oct 2012 12:41:13 -0700
>>> Subject: [PATCH] linux/compiler.h: Add __must_hold macro for functions called with a lock held
>> 
>> Ah.  OK.  So... would you like me to submit your patch to the LKML, or are you doing that?
>> 
>> It seems like a needed addition.
> 
> If you can confirm that it works for you, I'll send it to LKML with a
> Tested-by from you.

Does my usage in demo.c (as shown below) look correct to you?  If so, then I can confirm that it eliminates the warnings as intended.

Similar usage in the original code that motivated this inquiry also passes sparse when using __must_hold() with your patch applied.

[ecashin@marino linux]$ nl -b a drivers/block/aoe/demo.c
     1  #include <linux/netdevice.h>
     2  #include <linux/compiler.h>
     3  
     4  static spinlock_t lk;
     5  static struct sk_buff_head q;
     6  int demofn(void);
     7  
     8  /* enters with lk held */
     9  int demofn(void) __must_hold(&lk)
    10  {
    11          struct sk_buff *skb;
    12  
    13          while ((skb = skb_dequeue(&q))) {
    14                  spin_unlock_irq(&lk);
    15                  if (dev_queue_xmit(skb) == NET_XMIT_DROP && net_ratelimit())
    16                          pr_warn("informative warning\n");
    17                  spin_lock_irq(&lk);
    18          }
    19          return 0;
    20  }
[ecashin@marino linux]$ 

Thanks much!
Josh Triplett Oct. 8, 2012, 2:01 a.m. UTC | #4 
On Sun, Oct 07, 2012 at 07:35:44PM -0500, Ed Cashin wrote:
> On Oct 7, 2012, at 7:30 PM, Josh Triplett wrote:
> 
> > On Sun, Oct 07, 2012 at 04:28:16PM -0500, Ed Cashin wrote:
> >> On Oct 7, 2012, at 3:45 PM, Josh Triplett wrote:
> ...
> >>> From 0b862fc1a131a874d157420e9443f16a714596ef Mon Sep 17 00:00:00 2001
> >>> From: Josh Triplett <josh@joshtriplett.org>
> >>> Date: Sun, 7 Oct 2012 12:41:13 -0700
> >>> Subject: [PATCH] linux/compiler.h: Add __must_hold macro for functions called with a lock held
> >> 
> >> Ah.  OK.  So... would you like me to submit your patch to the LKML, or are you doing that?
> >> 
> >> It seems like a needed addition.
> > 
> > If you can confirm that it works for you, I'll send it to LKML with a
> > Tested-by from you.
> 
> Does my usage in demo.c (as shown below) look correct to you?  If so, then I can confirm that it eliminates the warnings as intended.
> 
> Similar usage in the original code that motivated this inquiry also passes sparse when using __must_hold() with your patch applied.
> 
> [ecashin@marino linux]$ nl -b a drivers/block/aoe/demo.c
>      1  #include <linux/netdevice.h>
>      2  #include <linux/compiler.h>
>      3  
>      4  static spinlock_t lk;
>      5  static struct sk_buff_head q;
>      6  int demofn(void);
>      7  
>      8  /* enters with lk held */
>      9  int demofn(void) __must_hold(&lk)
>     10  {
>     11          struct sk_buff *skb;
>     12  
>     13          while ((skb = skb_dequeue(&q))) {
>     14                  spin_unlock_irq(&lk);
>     15                  if (dev_queue_xmit(skb) == NET_XMIT_DROP && net_ratelimit())
>     16                          pr_warn("informative warning\n");
>     17                  spin_lock_irq(&lk);
>     18          }
>     19          return 0;
>     20  }
> [ecashin@marino linux]$ 

Yes, that looks right to me; glad to hear it works for you.  I'll send
the patch to LKML shortly.

Meanwhile, you've still found at least one bug in Sparse, since it
doesn't give the context warning if you leave out lines 15 and 16.

- Josh Triplett
--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/include/linux/compiler.h b/include/linux/compiler.h
index f430e41..b121554 100644
--- a/include/linux/compiler.h
+++ b/include/linux/compiler.h
@@ -10,6 +10,7 @@ 
 # define __force	__attribute__((force))
 # define __nocast	__attribute__((nocast))
 # define __iomem	__attribute__((noderef, address_space(2)))
+# define __must_hold(x)	__attribute__((context(x,1,1)))
 # define __acquires(x)	__attribute__((context(x,0,1)))
 # define __releases(x)	__attribute__((context(x,1,0)))
 # define __acquire(x)	__context__(x,1)
@@ -33,6 +34,7 @@  extern void __chk_io_ptr(const volatile void __iomem *);
 # define __chk_user_ptr(x) (void)0
 # define __chk_io_ptr(x) (void)0
 # define __builtin_warning(x, y...) (1)
+# define __must_hold(x)
 # define __acquires(x)
 # define __releases(x)
 # define __acquire(x) (void)0