A question about sparse: how to use __acquires() and __releases() correctly ?

A question about sparse: how to use __acquires() and __releases() correctly ?

[Bart Van Assche]

Hello,

I'm helping to prepare the SCST source code for inclusion in the Linux
kernel by a.o. cleaning up sparse warnings. Although most of the SCST
source code has been annotated by this time it's still not clear to me
how to use __acquires() and __releases() correctly.

I will illustrate my questions via the following code from
net/core/dev.c, Linux kernel version 2.6.27.7:

void dev_seq_stop(struct seq_file *seq, void *v)
        __releases(dev_base_lock)
{
        read_unlock(&dev_base_lock);
}

The command "make C=2 M=net/core" produces the following output for
the above function (using a sparse binary built from the sparse git
repository, last updated on August 26, 2008):

net/core/dev.c:2579:2: warning: context problem in 'dev_seq_stop':
'_read_unlock' expected different context
net/core/dev.c:2579:2:    context 'lock': wanted >= 1, got 0

My questions are as follows:
* Which argument type should be passed to __releases() -- a pointer to
a lock structure or the lock strucure itself ? In the header file
include/linux/spinlock_api_smp.h a pointer is passed to __acquires()
and __releases(), while other code (like the above) passes the lock
structure itself to the __acquires() and __releases() annotations.
* If the __releases() annotation is used correctly in net/core/dev.c,
why does sparse complain about a context problem ?

Please keep me in CC -- I'm not subscribed to the LKML.

Bart.

Re: A question about sparse: how to use __acquires() and __releases() correctly ?

[Bart Van Assche]

[ping]

Is there anyone who can help me with the question below ?

Thanks,

Bart.

On Tue, Dec 2, 2008 at 8:59 PM, Bart Van Assche
<bart.vanassche@gmail.com> wrote:
> Hello,
>
> I'm helping to prepare the SCST source code for inclusion in the Linux
> kernel by a.o. cleaning up sparse warnings. Although most of the SCST
> source code has been annotated by this time it's still not clear to me
> how to use __acquires() and __releases() correctly.
>
> I will illustrate my questions via the following code from
> net/core/dev.c, Linux kernel version 2.6.27.7:
>
> void dev_seq_stop(struct seq_file *seq, void *v)
>        __releases(dev_base_lock)
> {
>        read_unlock(&dev_base_lock);
> }
>
> The command "make C=2 M=net/core" produces the following output for
> the above function (using a sparse binary built from the sparse git
> repository, last updated on August 26, 2008):
>
> net/core/dev.c:2579:2: warning: context problem in 'dev_seq_stop':
> '_read_unlock' expected different context
> net/core/dev.c:2579:2:    context 'lock': wanted >= 1, got 0
>
> My questions are as follows:
> * Which argument type should be passed to __releases() -- a pointer to
> a lock structure or the lock strucure itself ? In the header file
> include/linux/spinlock_api_smp.h a pointer is passed to __acquires()
> and __releases(), while other code (like the above) passes the lock
> structure itself to the __acquires() and __releases() annotations.
> * If the __releases() annotation is used correctly in net/core/dev.c,
> why does sparse complain about a context problem ?
>
> Please keep me in CC -- I'm not subscribed to the LKML.
>
> Bart.
>

Re: A question about sparse: how to use __acquires() and __releases() correctly ?

[Johannes Berg]

On Thu, 4 Dec 2008 13:12:58 +0100, "Bart Van Assche"
<bart.vanassche@gmail.com> wrote:
> [ping]
> 
> Is there anyone who can help me with the question below ?


>> void dev_seq_stop(struct seq_file *seq, void *v)
>>        __releases(dev_base_lock)
>> {
>>        read_unlock(&dev_base_lock);
>> }
>>
>> The command "make C=2 M=net/core" produces the following output for
>> the above function (using a sparse binary built from the sparse git
>> repository, last updated on August 26, 2008):
>>
>> net/core/dev.c:2579:2: warning: context problem in 'dev_seq_stop':
>> '_read_unlock' expected different context
>> net/core/dev.c:2579:2:    context 'lock': wanted >= 1, got 0

I don't think sparse can properly handle this yet, at least not in a way
you'd expect it to. I've extended sparse to handle it, but the current git
tree has only a partial set of my changes applied, and the remaining ones
have been contested. (I still think my initial changes should be reverted
in the meantime)

>> My questions are as follows:
>> * Which argument type should be passed to __releases() -- a pointer to
>> a lock structure or the lock strucure itself ? In the header file
>> include/linux/spinlock_api_smp.h a pointer is passed to __acquires()
>> and __releases(), while other code (like the above) passes the lock
>> structure itself to the __acquires() and __releases() annotations.

sparse prett much ignores the first argument anyway, this isn't defined
yet.

>> * If the __releases() annotation is used correctly in net/core/dev.c,
>> why does sparse complain about a context problem ?

Maybe it is? I don't know off-hand.

johannes

Re: A question about sparse: how to use __acquires() and __releases() correctly ?

[Eric Dumazet]

Bart Van Assche a écrit :
> [ping]
> 
> Is there anyone who can help me with the question below ?
> 
> Thanks,
> 
> Bart.
> 
> On Tue, Dec 2, 2008 at 8:59 PM, Bart Van Assche
> <bart.vanassche@gmail.com> wrote:
>> Hello,
>>
>> I'm helping to prepare the SCST source code for inclusion in the Linux
>> kernel by a.o. cleaning up sparse warnings. Although most of the SCST
>> source code has been annotated by this time it's still not clear to me
>> how to use __acquires() and __releases() correctly.
>>
>> I will illustrate my questions via the following code from
>> net/core/dev.c, Linux kernel version 2.6.27.7:
>>
>> void dev_seq_stop(struct seq_file *seq, void *v)
>>        __releases(dev_base_lock)
>> {
>>        read_unlock(&dev_base_lock);
>> }
>>
>> The command "make C=2 M=net/core" produces the following output for
>> the above function (using a sparse binary built from the sparse git
>> repository, last updated on August 26, 2008):
>>
>> net/core/dev.c:2579:2: warning: context problem in 'dev_seq_stop':
>> '_read_unlock' expected different context
>> net/core/dev.c:2579:2:    context 'lock': wanted >= 1, got 0
>>
>> My questions are as follows:
>> * Which argument type should be passed to __releases() -- a pointer to
>> a lock structure or the lock strucure itself ? In the header file
>> include/linux/spinlock_api_smp.h a pointer is passed to __acquires()
>> and __releases(), while other code (like the above) passes the lock
>> structure itself to the __acquires() and __releases() annotations.
>> * If the __releases() annotation is used correctly in net/core/dev.c,
>> why does sparse complain about a context problem ?
>>
>> Please keep me in CC -- I'm not subscribed to the LKML.
>>
>> Bart.
>>

I added __releases() annotation in dev_seq_stop() in January 1st

^1da177e (Linus Torvalds                 2005-04-16 15:20:36 -0700 2617) }
^1da177e (Linus Torvalds                 2005-04-16 15:20:36 -0700 2618)
^1da177e (Linus Torvalds                 2005-04-16 15:20:36 -0700 2619) void dev_seq_stop(struct seq_file *seq, void *v)
9a429c49 (Eric Dumazet                   2008-01-01 21:58:02 -0800 2620)        __releases(dev_base_lock)
^1da177e (Linus Torvalds                 2005-04-16 15:20:36 -0700 2621) {
^1da177e (Linus Torvalds                 2005-04-16 15:20:36 -0700 2622)        read_unlock(&dev_base_lock);
^1da177e (Linus Torvalds                 2005-04-16 15:20:36 -0700 2623) }


The sparse version I have here doesnt complain on dev_seq_stop() in net/core/dev.c

  CHECK   net/core/dev.c
net/core/dev.c:2060:29: warning: symbol 'br_fdb_get_hook' was not declared. Should it be static?
net/core/dev.c:2062:6: warning: symbol 'br_fdb_put_hook' was not declared. Should it be static?
net/core/dev.c:1889:4: warning: context imbalance in 'dev_queue_xmit' - different lock contexts for basic block
net/core/dev.c:2022:3: warning: context imbalance in 'net_tx_action' - different lock contexts for basic block
include/linux/netpoll.h:94:2: warning: context imbalance in 'net_rx_action' - different lock contexts for basic block
  CC      net/core/dev.o

# sparse --version
# sparse -V
# type sparse
sparse is hashed (/root/bin/sparse)
# ls -l /root/bin/sparse
-rwxr-xr-x  1 root root 947324 déc 17  2007 /root/bin/sparse
# cd /usr/src/sparse
# git log | head
commit a02aeb329d5a8f9047c0b75b7e7f64ee2db3ffcf
Author: Josh Triplett <josh@freedesktop.org>
Date:   Tue Nov 13 04:15:13 2007 -0800

    Makefile: VERSION=0.4.1

    Signed-off-by: Josh Triplett <josh@freedesktop.org>

Re: A question about sparse: how to use __acquires() and __releases() correctly ?

[Bart Van Assche]

On Thu, Dec 4, 2008 at 2:29 PM, Eric Dumazet <dada1@cosmosbay.com> wrote:
> The sparse version I have here doesn't complain on dev_seq_stop() in
> net/core/dev.c

I have been running sparse on a tree built with a .config file
generated by 'make allmodconfig'. Could this be an explanation ?

Bart.

Re: A question about sparse: how to use __acquires() and __releases() correctly ?

[Eric Dumazet]

Bart Van Assche a écrit :
> On Thu, Dec 4, 2008 at 2:29 PM, Eric Dumazet <dada1@cosmosbay.com> wrote:
>> The sparse version I have here doesn't complain on dev_seq_stop() in
>> net/core/dev.c
> 
> I have been running sparse on a tree built with a .config file
> generated by 'make allmodconfig'. Could this be an explanation ?

No. this only happens because your sparse is more recent than mine

# git pull
Updating a02aeb3..8f208e2
Fast forward
 cgcc                                             |   14 +
 evaluate.c                                       |   40 +-
 expression.c                                     |    2 +
 ident-list.h                                     |    2 +
 inline.c                                         |   12 +-
 lib.c                                            |  115 +++++
 lib.h                                            |    1 +
 linearize.c                                      |   37 +-
 linearize.h                                      |    7 +-
 parse.c                                          |  153 ++++++-
 parse.h                                          |    3 +-
 scope.c                                          |    2 +-
 show-parse.c                                     |   98 ++--
 simplify.c                                       |   57 +++-
 sparse.1                                         |   53 ++-
 sparse.c                                         |  424 +++++++++++++++--
 symbol.c                                         |    4 +-
 symbol.h                                         |    6 +-
 tokenize.c                                       |    2 +
 validation/compare-null-to-int.c                 |    2 +-
 validation/cond_expr.c                           |    2 +-
 validation/context-dynamic.c                     |  171 +++++++
 validation/context-named.c                       |  553 ++++++++++++++++++++++
 validation/context-statement.c                   |   69 +++
 validation/context.c                             |  114 ++++-
 validation/declaration-after-statement-ansi.c    |   12 +
 validation/declaration-after-statement-c89.c     |   12 +
 validation/declaration-after-statement-c99.c     |    9 +
 validation/declaration-after-statement-default.c |    9 +
 validation/dubious-bitwise-with-not.c            |    9 +
 30 files changed, 1811 insertions(+), 183 deletions(-)
 create mode 100644 validation/context-dynamic.c
 create mode 100644 validation/context-named.c
 create mode 100644 validation/context-statement.c
 create mode 100644 validation/declaration-after-statement-ansi.c
 create mode 100644 validation/declaration-after-statement-c89.c
 create mode 100644 validation/declaration-after-statement-c99.c
 create mode 100644 validation/declaration-after-statement-default.c
 create mode 100644 validation/dubious-bitwise-with-not.c

After this update I have same warnings

# make C=2 net/core/dev.o
  CHK     include/linux/version.h
  CHK     include/linux/utsrelease.h
  SYMLINK include/asm -> include/asm-x86
  CALL    scripts/checksyscalls.sh
  CHECK   scripts/mod/empty.c
  CHECK   net/core/dev.c
net/core/dev.c:2060:29: warning: symbol 'br_fdb_get_hook' was not declared. Should it be static?
net/core/dev.c:2062:6: warning: symbol 'br_fdb_put_hook' was not declared. Should it be static?
net/core/dev.c:626:9: warning: context imbalance in 'dev_get_by_name': wrong count at exit
net/core/dev.c:626:9:    context 'lock': wanted 0, got 1
net/core/dev.c:675:9: warning: context imbalance in 'dev_get_by_index': wrong count at exit
net/core/dev.c:675:9:    context 'lock': wanted 0, got 1
net/core/dev.c:763:9: warning: context imbalance in 'dev_get_by_flags': wrong count at exit
net/core/dev.c:763:9:    context 'lock': wanted 0, got 1
net/core/dev.c:1045:24: warning: context imbalance in 'dev_load': wrong count at exit
net/core/dev.c:1045:24:    context 'lock': wanted 0, got 1
net/core/dev.c:1909:9: warning: context imbalance in 'dev_queue_xmit': wrong count at exit
net/core/dev.c:1909:9:    context 'lock': wanted 0, got 1
net/core/dev.c:2150:9: warning: context imbalance in 'ing_filter': wrong count at exit
net/core/dev.c:2150:9:    context 'lock': wanted 0, got 1
net/core/dev.c:2469:2: warning: context imbalance in 'net_rx_action': wrong count at exit
net/core/dev.c:2469:2:    context 'lock': wanted 0, got 1
net/core/dev.c:2527:9: warning: context imbalance in 'dev_ifname': wrong count at exit
net/core/dev.c:2527:9:    context 'lock': wanted 0, got 1
net/core/dev.c:2608:9: warning: context imbalance in 'dev_seq_start': unexpected unlock
net/core/dev.c:2608:9:    context 'dev_base_lock': wanted 1, got 0
net/core/dev.c:2622:2: warning: context imbalance in 'dev_seq_stop': wrong count at exit
net/core/dev.c:2622:2:    context 'dev_base_lock': wanted 0, got 1
include/linux/netdevice.h:1767:2: warning: context imbalance in 'dev_unicast_unsync': wrong count at exit
include/linux/netdevice.h:1767:2:    context 'lock': wanted 0, got 1