Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1480127 > unrolled thread

[PATCH] staging: lustre: lustre/ldlm: Fixed sparse warnings

Back to article view | Back to linux.kernel

Contents

  [PATCH] staging: lustre: lustre/ldlm: Fixed sparse warnings Nayeemahmed Badebade <itachi.opsrc@gmail.com> - 2016-09-09 17:30 +0200
    Re: [PATCH] staging: lustre: lustre/ldlm: Fixed sparse warnings Greg KH <gregkh@linuxfoundation.org> - 2016-09-12 12:30 +0200
      Re: [PATCH] staging: lustre: lustre/ldlm: Fixed sparse warnings "Dilger, Andreas" <andreas.dilger@intel.com> - 2016-09-14 07:20 +0200
        Re: [PATCH] staging: lustre: lustre/ldlm: Fixed sparse warnings nayeem <itachi.opsrc@gmail.com> - 2016-09-15 20:40 +0200
          Re: [PATCH] staging: lustre: lustre/ldlm: Fixed sparse warnings "Dilger, Andreas" <andreas.dilger@intel.com> - 2016-09-16 10:10 +0200
            Re: [PATCH] staging: lustre: lustre/ldlm: Fixed sparse warnings nayeem <itachi.opsrc@gmail.com> - 2016-09-18 22:30 +0200
              Re: [PATCH] staging: lustre: lustre/ldlm: Fixed sparse warnings "Dilger, Andreas" <andreas.dilger@intel.com> - 2016-09-18 22:40 +0200
                [PATCH v2] staging: lustre: lustre/ldlm: Fixed sparse warnings Nayeemahmed Badebade <itachi.opsrc@gmail.com> - 2016-09-18 23:20 +0200
                  Re: [PATCH v2] staging: lustre: lustre/ldlm: Fixed sparse warnings "Dilger, Andreas" <andreas.dilger@intel.com> - 2016-09-18 23:30 +0200
                  Re: [PATCH v2] staging: lustre: lustre/ldlm: Fixed sparse warnings James Simmons <jsimmons@infradead.org> - 2016-09-19 22:50 +0200

#1480127 — [PATCH] staging: lustre: lustre/ldlm: Fixed sparse warnings

Added __acquires / __releases sparse locking annotations
to lock_res_and_lock and unlock_res_and_lock functions in
l_lock.c, to fix below sparse warnings:

 l_lock.c:47:22: warning: context imbalance in 'lock_res_and_lock' - wrong count at exit
 l_lock.c:62:6: warning: context imbalance in 'unlock_res_and_lock' - unexpected unlock

Signed-off-by: Nayeemahmed Badebade <itachi.opsrc@gmail.com>
---
 drivers/staging/lustre/lustre/ldlm/l_lock.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/staging/lustre/lustre/ldlm/l_lock.c b/drivers/staging/lustre/lustre/ldlm/l_lock.c
index ea8840c..c4b9612 100644
--- a/drivers/staging/lustre/lustre/ldlm/l_lock.c
+++ b/drivers/staging/lustre/lustre/ldlm/l_lock.c
@@ -45,6 +45,8 @@
  * being an atomic operation.
  */
 struct ldlm_resource *lock_res_and_lock(struct ldlm_lock *lock)
+				__acquires(&lock->l_lock)
+				__acquires(lock->l_resource)
 {
 	spin_lock(&lock->l_lock);

@@ -59,6 +61,8 @@ EXPORT_SYMBOL(lock_res_and_lock);
  * Unlock a lock and its resource previously locked with lock_res_and_lock
  */
 void unlock_res_and_lock(struct ldlm_lock *lock)
+		__releases(lock->l_resource)
+		__releases(&lock->l_lock)
 {
 	/* on server-side resource of lock doesn't change */
 	ldlm_clear_res_locked(lock);
--
1.9.1

[toc] | [next] | [standalone]

#1481122

On Fri, Sep 09, 2016 at 08:50:35PM +0530, Nayeemahmed Badebade wrote:
> Added __acquires / __releases sparse locking annotations
> to lock_res_and_lock and unlock_res_and_lock functions in
> l_lock.c, to fix below sparse warnings:
> 
>  l_lock.c:47:22: warning: context imbalance in 'lock_res_and_lock' - wrong count at exit
>  l_lock.c:62:6: warning: context imbalance in 'unlock_res_and_lock' - unexpected unlock
> 
> Signed-off-by: Nayeemahmed Badebade <itachi.opsrc@gmail.com>
> ---
>  drivers/staging/lustre/lustre/ldlm/l_lock.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/staging/lustre/lustre/ldlm/l_lock.c b/drivers/staging/lustre/lustre/ldlm/l_lock.c
> index ea8840c..c4b9612 100644
> --- a/drivers/staging/lustre/lustre/ldlm/l_lock.c
> +++ b/drivers/staging/lustre/lustre/ldlm/l_lock.c
> @@ -45,6 +45,8 @@
>   * being an atomic operation.
>   */
>  struct ldlm_resource *lock_res_and_lock(struct ldlm_lock *lock)
> +				__acquires(&lock->l_lock)
> +				__acquires(lock->l_resource)

Hm, these are tricky, I don't want to take this type of change without
an ack from the lustre developers...

thanks,

greg k-h

[toc] | [prev] | [next] | [standalone]

#1482929

On Sep 12, 2016, at 04:27, Greg KH <gregkh@linuxfoundation.org> wrote:
> 
> On Fri, Sep 09, 2016 at 08:50:35PM +0530, Nayeemahmed Badebade wrote:
>> Added __acquires / __releases sparse locking annotations
>> to lock_res_and_lock and unlock_res_and_lock functions in
>> l_lock.c, to fix below sparse warnings:
>> 
>> l_lock.c:47:22: warning: context imbalance in 'lock_res_and_lock' - wrong count at exit
>> l_lock.c:62:6: warning: context imbalance in 'unlock_res_and_lock' - unexpected unlock
>> 
>> Signed-off-by: Nayeemahmed Badebade <itachi.opsrc@gmail.com>
>> ---
>> drivers/staging/lustre/lustre/ldlm/l_lock.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>> 
>> diff --git a/drivers/staging/lustre/lustre/ldlm/l_lock.c b/drivers/staging/lustre/lustre/ldlm/l_lock.c
>> index ea8840c..c4b9612 100644
>> --- a/drivers/staging/lustre/lustre/ldlm/l_lock.c
>> +++ b/drivers/staging/lustre/lustre/ldlm/l_lock.c
>> @@ -45,6 +45,8 @@
>>  * being an atomic operation.
>>  */
>> struct ldlm_resource *lock_res_and_lock(struct ldlm_lock *lock)
>> +				__acquires(&lock->l_lock)
>> +				__acquires(lock->l_resource)
> 
> Hm, these are tricky, I don't want to take this type of change without
> an ack from the lustre developers...

The "__acquires(&lock->l_lock)" line here looks correct, along with the
corresponding "__releases(&lock->l_lock)" at unlock_res_and_lock().

The problem, however, is that "l_resource" is not a lock, but rather a
struct.  The call to "lock_res(lock->l_resource)" is actually locking
"lr_lock" internally.

It would be better to add "__acquires(&res->lr_lock)" at lock_res() and
"__releases(&res->lr_lock)" at unlock_res().  That will also forestall
any other warnings about an imbalance with lock_res()/unlock_res() or
their callsites.

Cheers, Andreas

[toc] | [prev] | [next] | [standalone]

#1484456

[Multipart message — attachments visible in raw view] — view raw

On Wednesday 14 September 2016 10:44 AM, Dilger, Andreas wrote:
> On Sep 12, 2016, at 04:27, Greg KH <gregkh@linuxfoundation.org> wrote:
>>
>> On Fri, Sep 09, 2016 at 08:50:35PM +0530, Nayeemahmed Badebade wrote:
>>> Added __acquires / __releases sparse locking annotations
>>> to lock_res_and_lock and unlock_res_and_lock functions in
>>> l_lock.c, to fix below sparse warnings:
>>>
>>> l_lock.c:47:22: warning: context imbalance in 'lock_res_and_lock' - wrong count at exit
>>> l_lock.c:62:6: warning: context imbalance in 'unlock_res_and_lock' - unexpected unlock
>>>
>>> Signed-off-by: Nayeemahmed Badebade <itachi.opsrc@gmail.com>
>>> ---
>>> drivers/staging/lustre/lustre/ldlm/l_lock.c | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/staging/lustre/lustre/ldlm/l_lock.c b/drivers/staging/lustre/lustre/ldlm/l_lock.c
>>> index ea8840c..c4b9612 100644
>>> --- a/drivers/staging/lustre/lustre/ldlm/l_lock.c
>>> +++ b/drivers/staging/lustre/lustre/ldlm/l_lock.c
>>> @@ -45,6 +45,8 @@
>>>   * being an atomic operation.
>>>   */
>>> struct ldlm_resource *lock_res_and_lock(struct ldlm_lock *lock)
>>> +				__acquires(&lock->l_lock)
>>> +				__acquires(lock->l_resource)
>>
>> Hm, these are tricky, I don't want to take this type of change without
>> an ack from the lustre developers...
>
> The "__acquires(&lock->l_lock)" line here looks correct, along with the
> corresponding "__releases(&lock->l_lock)" at unlock_res_and_lock().
>
> The problem, however, is that "l_resource" is not a lock, but rather a
> struct.  The call to "lock_res(lock->l_resource)" is actually locking
> "lr_lock" internally.
>
> It would be better to add "__acquires(&res->lr_lock)" at lock_res() and
> "__releases(&res->lr_lock)" at unlock_res().  That will also forestall
> any other warnings about an imbalance with lock_res()/unlock_res() or
> their callsites.
>
> Cheers, Andreas
>

Hi Andreas,

Thank you for your review comments. I did the change according to your 
comments and the diff is attached to mail. But this change doesn't seem 
to fix the sparse warning.
With this change when i compile the code "make C=2 
./drivers/staging/lustre/lustre/", sparse warning still comes:
{{{
   CHECK   drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/l_lock.c
drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/l_lock.c:47:22: 
warning: context imbalance in 'lock_res_and_lock' - wrong count at exit
drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/l_lock.c:62:6: 
warning: context imbalance in 'unlock_res_and_lock' - unexpected unlock
   CC [M]  drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/l_lock.o
}}}

Would it be a good idea to add "__acquires(&lock->l_resource->lr_lock)" 
& "__acquires(&lock->l_lock)" at lock_res_and_lock() and 
"__releases(&lock->l_resource->lr_lock)" & "__releases(&lock->l_lock)" 
at unlock_res_and_lock() ?
Because with that change the sparse warning is fixed.
{{{
   CHECK   drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/l_lock.c
   CC [M]  drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/l_lock.o
}}}
Could you please confirm this.

Regards,
Nayeem

[toc] | [prev] | [next] | [standalone]

#1484767

On Sep 15, 2016, at 12:33, nayeem <itachi.opsrc@gmail.com> wrote:
> On Wednesday 14 September 2016 10:44 AM, Dilger, Andreas wrote:
>> On Sep 12, 2016, at 04:27, Greg KH <gregkh@linuxfoundation.org> wrote:
>>> 
>>> On Fri, Sep 09, 2016 at 08:50:35PM +0530, Nayeemahmed Badebade wrote:
>>>> Added __acquires / __releases sparse locking annotations
>>>> to lock_res_and_lock and unlock_res_and_lock functions in
>>>> l_lock.c, to fix below sparse warnings:
>>>> 
>>>> l_lock.c:47:22: warning: context imbalance in 'lock_res_and_lock' - wrong count at exit
>>>> l_lock.c:62:6: warning: context imbalance in 'unlock_res_and_lock' - unexpected unlock
>>>> 
>>>> Signed-off-by: Nayeemahmed Badebade <itachi.opsrc@gmail.com>
>>>> ---
>>>> drivers/staging/lustre/lustre/ldlm/l_lock.c | 4 ++++
>>>> 1 file changed, 4 insertions(+)
>>>> 
>>>> diff --git a/drivers/staging/lustre/lustre/ldlm/l_lock.c b/drivers/staging/lustre/lustre/ldlm/l_lock.c
>>>> index ea8840c..c4b9612 100644
>>>> --- a/drivers/staging/lustre/lustre/ldlm/l_lock.c
>>>> +++ b/drivers/staging/lustre/lustre/ldlm/l_lock.c
>>>> @@ -45,6 +45,8 @@
>>>>  * being an atomic operation.
>>>>  */
>>>> struct ldlm_resource *lock_res_and_lock(struct ldlm_lock *lock)
>>>> +				__acquires(&lock->l_lock)
>>>> +				__acquires(lock->l_resource)
>>> 
>>> Hm, these are tricky, I don't want to take this type of change without
>>> an ack from the lustre developers...
>> 
>> The "__acquires(&lock->l_lock)" line here looks correct, along with the
>> corresponding "__releases(&lock->l_lock)" at unlock_res_and_lock().
>> 
>> The problem, however, is that "l_resource" is not a lock, but rather a
>> struct.  The call to "lock_res(lock->l_resource)" is actually locking
>> "lr_lock" internally.
>> 
>> It would be better to add "__acquires(&res->lr_lock)" at lock_res() and
>> "__releases(&res->lr_lock)" at unlock_res().  That will also forestall
>> any other warnings about an imbalance with lock_res()/unlock_res() or
>> their callsites.
>> 
>> Cheers, Andreas
>> 
> 
> Hi Andreas,
> 
> Thank you for your review comments. I did the change according to your comments and the diff is attached to mail. But this change doesn't seem to fix the sparse warning.
> With this change when i compile the code "make C=2 ./drivers/staging/lustre/lustre/", sparse warning still comes:

> {{{
>  CHECK   drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/l_lock.c
> drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/l_lock.c:47:22: warning: context imbalance in 'lock_res_and_lock' - wrong count at exit
> drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/l_lock.c:62:6: warning: context imbalance in 'unlock_res_and_lock' - unexpected unlock
>  CC [M]  drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/l_lock.o
> }}}

Strange, one would think that your patch should work properly.  Maybe the
__acquires() label doesn't work on inline functions?

> Would it be a good idea to add "__acquires(&lock->l_resource->lr_lock)" & "__acquires(&lock->l_lock)" at lock_res_and_lock() and "__releases(&lock->l_resource->lr_lock)" & "__releases(&lock->l_lock)" at unlock_res_and_lock() ?
> Because with that change the sparse warning is fixed.
> {{{
>  CHECK   drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/l_lock.c
>  CC [M]  drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/l_lock.o
> }}}

This would also be possible, but then it exposes any callers of lock_res()
and unlock() res to similar compiler warnings in the future.  I'm not
against this in principle, but it is worthwhile to see why sparse is not
handling this case correctly.

Cheers, Andreas

> Could you please confirm this.
> 
> Regards,
> Nayeem
> 
> <sparse-warnings-fix-patch-v2.patch>

[toc] | [prev] | [next] | [standalone]

#1485938

On Friday 16 September 2016 01:30 PM, Dilger, Andreas wrote:
> On Sep 15, 2016, at 12:33, nayeem <itachi.opsrc@gmail.com> wrote:
>> On Wednesday 14 September 2016 10:44 AM, Dilger, Andreas wrote:
>>> On Sep 12, 2016, at 04:27, Greg KH <gregkh@linuxfoundation.org> wrote:
>>>>
>>>> On Fri, Sep 09, 2016 at 08:50:35PM +0530, Nayeemahmed Badebade wrote:
>>>>> Added __acquires / __releases sparse locking annotations
>>>>> to lock_res_and_lock and unlock_res_and_lock functions in
>>>>> l_lock.c, to fix below sparse warnings:
>>>>>
>>>>> l_lock.c:47:22: warning: context imbalance in 'lock_res_and_lock' - wrong count at exit
>>>>> l_lock.c:62:6: warning: context imbalance in 'unlock_res_and_lock' - unexpected unlock
>>>>>
>>>>> Signed-off-by: Nayeemahmed Badebade <itachi.opsrc@gmail.com>
>>>>> ---
>>>>> drivers/staging/lustre/lustre/ldlm/l_lock.c | 4 ++++
>>>>> 1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/drivers/staging/lustre/lustre/ldlm/l_lock.c b/drivers/staging/lustre/lustre/ldlm/l_lock.c
>>>>> index ea8840c..c4b9612 100644
>>>>> --- a/drivers/staging/lustre/lustre/ldlm/l_lock.c
>>>>> +++ b/drivers/staging/lustre/lustre/ldlm/l_lock.c
>>>>> @@ -45,6 +45,8 @@
>>>>>   * being an atomic operation.
>>>>>   */
>>>>> struct ldlm_resource *lock_res_and_lock(struct ldlm_lock *lock)
>>>>> +				__acquires(&lock->l_lock)
>>>>> +				__acquires(lock->l_resource)
>>>>
>>>> Hm, these are tricky, I don't want to take this type of change without
>>>> an ack from the lustre developers...
>>>
>>> The "__acquires(&lock->l_lock)" line here looks correct, along with the
>>> corresponding "__releases(&lock->l_lock)" at unlock_res_and_lock().
>>>
>>> The problem, however, is that "l_resource" is not a lock, but rather a
>>> struct.  The call to "lock_res(lock->l_resource)" is actually locking
>>> "lr_lock" internally.
>>>
>>> It would be better to add "__acquires(&res->lr_lock)" at lock_res() and
>>> "__releases(&res->lr_lock)" at unlock_res().  That will also forestall
>>> any other warnings about an imbalance with lock_res()/unlock_res() or
>>> their callsites.
>>>
>>> Cheers, Andreas
>>>
>>
>> Hi Andreas,
>>
>> Thank you for your review comments. I did the change according to your comments and the diff is attached to mail. But this change doesn't seem to fix the sparse warning.
>> With this change when i compile the code "make C=2 ./drivers/staging/lustre/lustre/", sparse warning still comes:
>
>> {{{
>>   CHECK   drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/l_lock.c
>> drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/l_lock.c:47:22: warning: context imbalance in 'lock_res_and_lock' - wrong count at exit
>> drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/l_lock.c:62:6: warning: context imbalance in 'unlock_res_and_lock' - unexpected unlock
>>   CC [M]  drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/l_lock.o
>> }}}
>
> Strange, one would think that your patch should work properly.  Maybe the
> __acquires() label doesn't work on inline functions?
>

I think sparse works on inline functions.
I ran sparse on a hello world kernel module in different cases explained 
below


>> Would it be a good idea to add "__acquires(&lock->l_resource->lr_lock)" & "__acquires(&lock->l_lock)" at lock_res_and_lock() and "__releases(&lock->l_resource->lr_lock)" & "__releases(&lock->l_lock)" at unlock_res_and_lock() ?
>> Because with that change the sparse warning is fixed.
>> {{{
>>   CHECK   drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/l_lock.c
>>   CC [M]  drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/l_lock.o
>> }}}
>
> This would also be possible, but then it exposes any callers of lock_res()
> and unlock() res to similar compiler warnings in the future.  I'm not
> against this in principle, but it is worthwhile to see why sparse is not
> handling this case correctly.
>
> Cheers, Andreas
>

case 1:
-------
hello.c, where spin_lock() and spin_unlock() are called indirectly via 
foo_lock() and foo_unlock() in the same function i.e "say_hello()" in 
below code.

The following code when checked with sparse doesn't give any warning

#include<linux/module.h>
#include<linux/init.h>

static DEFINE_SPINLOCK(my_lock);

static inline void foo_lock(spinlock_t *spl)
{
         spin_lock(spl);
}

static inline void foo_unlock(spinlock_t *spl)
{
         spin_unlock(spl);
}

static int __init say_hello(void)
{
         foo_lock(&my_lock);
         pr_info("Hello World!\n");
         foo_unlock(&my_lock);
         return 0;
}

static void __exit cleanup(void)
{
}

module_init(say_hello);
module_exit(cleanup);



case 2.
------
The above code when slightly modified so that, spin_lock() is called 
indirectly via foo_lock() in say_hello() and spin_unlock() via 
foo_unlock() in cleanup()

static int __init say_hello(void)
{
         foo_lock(&my_lock);
         pr_info("Hello World!\n");

         return 0;
}

static void __exit cleanup(void)
{
         foo_unlock(&my_lock);
}

Then sparse gives the warning:
{{{
test-module/hello.c:16:19: warning: context imbalance in 'say_hello' - 
wrong count at exit
test-module/hello.c:23:20: warning: context imbalance in 'cleanup' - 
unexpected unlock
}}}
To fix this if we put sparse annotations __acquires() at foo_lock() and 
__releases() at foo_unlock(), then also sparse warnings comes, which is 
exactly the case with l_lock.c in lustre code.

The warning will still be thrown if these functions are not inline.
I think this kind of case sparse is not able to handle, irrespective of 
whether function is inline or not.

case 3:
-------
Instead of putting sparse annotations at foo_lock and foo_unlock, if we 
put them at say_hello() and cleanup()

static int __init say_hello(void)
                 __acquires(&my_lock)
{
         foo_lock(&my_lock);
         pr_info("Hello World!\n");
         return 0;
}

static void __exit cleanup(void)
                 __releases(&my_lock)
{
         foo_unlock(&my_lock);
}

Then sparse seems to work properly and warning doesn't come.

So i think in case of l_lock.c in lustre, both "lock_res_and_lock()" and 
"unlock_res_and_lock" needs to have sparse annotations.

Please provide your inputs on this.

Thanks & Regards,
Nayeem

[toc] | [prev] | [next] | [standalone]

#1485940

On Sep 18, 2016, at 14:21, nayeem <itachi.opsrc@gmail.com> wrote:
> On Friday 16 September 2016 01:30 PM, Dilger, Andreas wrote:
>> On Sep 15, 2016, at 12:33, nayeem <itachi.opsrc@gmail.com> wrote:
>>> On Wednesday 14 September 2016 10:44 AM, Dilger, Andreas wrote:
>>>> On Sep 12, 2016, at 04:27, Greg KH <gregkh@linuxfoundation.org> wrote:
>>>>> 
>>>>> On Fri, Sep 09, 2016 at 08:50:35PM +0530, Nayeemahmed Badebade wrote:
>>>>>> Added __acquires / __releases sparse locking annotations
>>>>>> to lock_res_and_lock and unlock_res_and_lock functions in
>>>>>> l_lock.c, to fix below sparse warnings:
>>>>>> 
>>>>>> l_lock.c:47:22: warning: context imbalance in 'lock_res_and_lock' - wrong count at exit
>>>>>> l_lock.c:62:6: warning: context imbalance in 'unlock_res_and_lock' - unexpected unlock
>>>>>> 
>>>>>> Signed-off-by: Nayeemahmed Badebade <itachi.opsrc@gmail.com>
>>>>>> ---
>>>>>> drivers/staging/lustre/lustre/ldlm/l_lock.c | 4 ++++
>>>>>> 1 file changed, 4 insertions(+)
>>>>>> 
>>>>>> diff --git a/drivers/staging/lustre/lustre/ldlm/l_lock.c b/drivers/staging/lustre/lustre/ldlm/l_lock.c
>>>>>> index ea8840c..c4b9612 100644
>>>>>> --- a/drivers/staging/lustre/lustre/ldlm/l_lock.c
>>>>>> +++ b/drivers/staging/lustre/lustre/ldlm/l_lock.c
>>>>>> @@ -45,6 +45,8 @@
>>>>>>  * being an atomic operation.
>>>>>>  */
>>>>>> struct ldlm_resource *lock_res_and_lock(struct ldlm_lock *lock)
>>>>>> +				__acquires(&lock->l_lock)
>>>>>> +				__acquires(lock->l_resource)
>>>>> 
>>>>> Hm, these are tricky, I don't want to take this type of change without
>>>>> an ack from the lustre developers...
>>>> 
>>>> The "__acquires(&lock->l_lock)" line here looks correct, along with the
>>>> corresponding "__releases(&lock->l_lock)" at unlock_res_and_lock().
>>>> 
>>>> The problem, however, is that "l_resource" is not a lock, but rather a
>>>> struct.  The call to "lock_res(lock->l_resource)" is actually locking
>>>> "lr_lock" internally.
>>>> 
>>>> It would be better to add "__acquires(&res->lr_lock)" at lock_res() and
>>>> "__releases(&res->lr_lock)" at unlock_res().  That will also forestall
>>>> any other warnings about an imbalance with lock_res()/unlock_res() or
>>>> their callsites.
>>>> 
>>>> Cheers, Andreas
>>>> 
>>> 
>>> Hi Andreas,
>>> 
>>> Thank you for your review comments. I did the change according to your comments and the diff is attached to mail. But this change doesn't seem to fix the sparse warning.
>>> With this change when i compile the code "make C=2 ./drivers/staging/lustre/lustre/", sparse warning still comes:
>> 
>>> {{{
>>>  CHECK   drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/l_lock.c
>>> drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/l_lock.c:47:22: warning: context imbalance in 'lock_res_and_lock' - wrong count at exit
>>> drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/l_lock.c:62:6: warning: context imbalance in 'unlock_res_and_lock' - unexpected unlock
>>>  CC [M]  drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/l_lock.o
>>> }}}
>> 
>> Strange, one would think that your patch should work properly.  Maybe the
>> __acquires() label doesn't work on inline functions?
>> 
> 
> I think sparse works on inline functions.
> I ran sparse on a hello world kernel module in different cases explained below
> 
> 
>>> Would it be a good idea to add "__acquires(&lock->l_resource->lr_lock)" & "__acquires(&lock->l_lock)" at lock_res_and_lock() and "__releases(&lock->l_resource->lr_lock)" & "__releases(&lock->l_lock)" at unlock_res_and_lock() ?
>>> Because with that change the sparse warning is fixed.
>>> {{{
>>>  CHECK   drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/l_lock.c
>>>  CC [M]  drivers/staging/lustre/lustre/ptlrpc/../../lustre/ldlm/l_lock.o
>>> }}}
>> 
>> This would also be possible, but then it exposes any callers of lock_res()
>> and unlock() res to similar compiler warnings in the future.  I'm not
>> against this in principle, but it is worthwhile to see why sparse is not
>> handling this case correctly.
>> 
>> Cheers, Andreas
>> 
> 
> case 1:
> -------
> hello.c, where spin_lock() and spin_unlock() are called indirectly via foo_lock() and foo_unlock() in the same function i.e "say_hello()" in below code.
> 
> The following code when checked with sparse doesn't give any warning
> 
> #include<linux/module.h>
> #include<linux/init.h>
> 
> static DEFINE_SPINLOCK(my_lock);
> 
> static inline void foo_lock(spinlock_t *spl)
> {
>        spin_lock(spl);
> }
> 
> static inline void foo_unlock(spinlock_t *spl)
> {
>        spin_unlock(spl);
> }
> 
> static int __init say_hello(void)
> {
>        foo_lock(&my_lock);
>        pr_info("Hello World!\n");
>        foo_unlock(&my_lock);
>        return 0;
> }
> 
> static void __exit cleanup(void)
> {
> }
> 
> module_init(say_hello);
> module_exit(cleanup);
> 
> 
> 
> case 2.
> ------
> The above code when slightly modified so that, spin_lock() is called indirectly via foo_lock() in say_hello() and spin_unlock() via foo_unlock() in cleanup()
> 
> static int __init say_hello(void)
> {
>        foo_lock(&my_lock);
>        pr_info("Hello World!\n");
> 
>        return 0;
> }
> 
> static void __exit cleanup(void)
> {
>        foo_unlock(&my_lock);
> }
> 
> Then sparse gives the warning:
> {{{
> test-module/hello.c:16:19: warning: context imbalance in 'say_hello' - wrong count at exit
> test-module/hello.c:23:20: warning: context imbalance in 'cleanup' - unexpected unlock
> }}}
> To fix this if we put sparse annotations __acquires() at foo_lock() and __releases() at foo_unlock(), then also sparse warnings comes, which is exactly the case with l_lock.c in lustre code.
> 
> The warning will still be thrown if these functions are not inline.
> I think this kind of case sparse is not able to handle, irrespective of whether function is inline or not.
> 
> case 3:
> -------
> Instead of putting sparse annotations at foo_lock and foo_unlock, if we put them at say_hello() and cleanup()
> 
> static int __init say_hello(void)
>                __acquires(&my_lock)
> {
>        foo_lock(&my_lock);
>        pr_info("Hello World!\n");
>        return 0;
> }
> 
> static void __exit cleanup(void)
>                __releases(&my_lock)
> {
>        foo_unlock(&my_lock);
> }
> 
> Then sparse seems to work properly and warning doesn't come.
> 
> So i think in case of l_lock.c in lustre, both "lock_res_and_lock()" and "unlock_res_and_lock" needs to have sparse annotations.
> 
> Please provide your inputs on this.

Originally I was thinking that there may be a bug in sparse to report,
but I think I can agree with your argument.  It isn't "foo_lock()" that has
the locking imbalance (it would always be imbalanced since it is the one
getting the lock in the first place).  Rather, it is say_hello() and
cleanup() that are the unusual functions with the imbalanced locking,
and should be the ones with the annotation.

Thanks for following through with this.  Please submit a patch with your
proposal from two emails ago, with annotations at lock_res_and_lock()
and unlock_res_and_lock().

Cheers, Andreas

[toc] | [prev] | [next] | [standalone]

#1486071 — [PATCH v2] staging: lustre: lustre/ldlm: Fixed sparse warnings

Added __acquires / __releases sparse locking annotations
to lock_res_and_lock() and unlock_res_and_lock() functions
in l_lock.c, to fix below sparse warnings:

l_lock.c:47:22: warning: context imbalance in 'lock_res_and_lock' - wrong count at exit
l_lock.c:61:6: warning: context imbalance in 'unlock_res_and_lock' - unexpected unlock

Signed-off-by: Nayeemahmed Badebade <itachi.opsrc@gmail.com>
---

Changes in v2:
 * Corrected sparse annotations for the lock
   lock->l_resource->lr_lock

 drivers/staging/lustre/lustre/ldlm/l_lock.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/staging/lustre/lustre/ldlm/l_lock.c b/drivers/staging/lustre/lustre/ldlm/l_lock.c
index ea8840c..3845f38 100644
--- a/drivers/staging/lustre/lustre/ldlm/l_lock.c
+++ b/drivers/staging/lustre/lustre/ldlm/l_lock.c
@@ -45,6 +45,8 @@
  * being an atomic operation.
  */
 struct ldlm_resource *lock_res_and_lock(struct ldlm_lock *lock)
+				__acquires(&lock->l_lock)
+				__acquires(&lock->l_resource->lr_lock)
 {
 	spin_lock(&lock->l_lock);

@@ -59,6 +61,8 @@ struct ldlm_resource *lock_res_and_lock(struct ldlm_lock *lock)
  * Unlock a lock and its resource previously locked with lock_res_and_lock
  */
 void unlock_res_and_lock(struct ldlm_lock *lock)
+		__releases(&lock->l_resource->lr_lock)
+		__releases(&lock->l_lock)
 {
 	/* on server-side resource of lock doesn't change */
 	ldlm_clear_res_locked(lock);
--
1.9.1

[toc] | [prev] | [next] | [standalone]

#1486073 — Re: [PATCH v2] staging: lustre: lustre/ldlm: Fixed sparse warnings

> On Sep 18, 2016, at 23:18, Nayeemahmed Badebade <itachi.opsrc@gmail.com> wrote:
> 
> Added __acquires / __releases sparse locking annotations
> to lock_res_and_lock() and unlock_res_and_lock() functions
> in l_lock.c, to fix below sparse warnings:
> 
> l_lock.c:47:22: warning: context imbalance in 'lock_res_and_lock' - wrong count at exit
> l_lock.c:61:6: warning: context imbalance in 'unlock_res_and_lock' - unexpected unlock
> 
> Signed-off-by: Nayeemahmed Badebade <itachi.opsrc@gmail.com>

Reviewed-by: Andreas Dilger <andreas.dilger@intel.com>

> ---
> 
> Changes in v2:
> * Corrected sparse annotations for the lock
>   lock->l_resource->lr_lock
> 
> drivers/staging/lustre/lustre/ldlm/l_lock.c | 4 ++++
> 1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/staging/lustre/lustre/ldlm/l_lock.c b/drivers/staging/lustre/lustre/ldlm/l_lock.c
> index ea8840c..3845f38 100644
> --- a/drivers/staging/lustre/lustre/ldlm/l_lock.c
> +++ b/drivers/staging/lustre/lustre/ldlm/l_lock.c
> @@ -45,6 +45,8 @@
>  * being an atomic operation.
>  */
> struct ldlm_resource *lock_res_and_lock(struct ldlm_lock *lock)
> +				__acquires(&lock->l_lock)
> +				__acquires(&lock->l_resource->lr_lock)
> {
> 	spin_lock(&lock->l_lock);
> 
> @@ -59,6 +61,8 @@ struct ldlm_resource *lock_res_and_lock(struct ldlm_lock *lock)
>  * Unlock a lock and its resource previously locked with lock_res_and_lock
>  */
> void unlock_res_and_lock(struct ldlm_lock *lock)
> +		__releases(&lock->l_resource->lr_lock)
> +		__releases(&lock->l_lock)
> {
> 	/* on server-side resource of lock doesn't change */
> 	ldlm_clear_res_locked(lock);
> --
> 1.9.1
> 

[toc] | [prev] | [next] | [standalone]

#1486845 — Re: [PATCH v2] staging: lustre: lustre/ldlm: Fixed sparse warnings

> Added __acquires / __releases sparse locking annotations
> to lock_res_and_lock() and unlock_res_and_lock() functions
> in l_lock.c, to fix below sparse warnings:
> 
> l_lock.c:47:22: warning: context imbalance in 'lock_res_and_lock' - wrong count at exit
> l_lock.c:61:6: warning: context imbalance in 'unlock_res_and_lock' - unexpected unlock
> 
> Signed-off-by: Nayeemahmed Badebade <itachi.opsrc@gmail.com>

Reviewed-by: James Simmons <jsimmons@infradead.org>

> ---
> 
> Changes in v2:
>  * Corrected sparse annotations for the lock
>    lock->l_resource->lr_lock
> 
>  drivers/staging/lustre/lustre/ldlm/l_lock.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/staging/lustre/lustre/ldlm/l_lock.c b/drivers/staging/lustre/lustre/ldlm/l_lock.c
> index ea8840c..3845f38 100644
> --- a/drivers/staging/lustre/lustre/ldlm/l_lock.c
> +++ b/drivers/staging/lustre/lustre/ldlm/l_lock.c
> @@ -45,6 +45,8 @@
>   * being an atomic operation.
>   */
>  struct ldlm_resource *lock_res_and_lock(struct ldlm_lock *lock)
> +				__acquires(&lock->l_lock)
> +				__acquires(&lock->l_resource->lr_lock)
>  {
>  	spin_lock(&lock->l_lock);
> 
> @@ -59,6 +61,8 @@ struct ldlm_resource *lock_res_and_lock(struct ldlm_lock *lock)
>   * Unlock a lock and its resource previously locked with lock_res_and_lock
>   */
>  void unlock_res_and_lock(struct ldlm_lock *lock)
> +		__releases(&lock->l_resource->lr_lock)
> +		__releases(&lock->l_lock)
>  {
>  	/* on server-side resource of lock doesn't change */
>  	ldlm_clear_res_locked(lock);
> --
> 1.9.1
> 
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web