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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On Friday 16 September 2016 01:30 PM, Dilger, Andreas wrote:
On Sep 15, 2016, at 12:33, nayeem <itachi.opsrc@xxxxxxxxx> wrote:
On Wednesday 14 September 2016 10:44 AM, Dilger, Andreas wrote:
On Sep 12, 2016, at 04:27, Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxx>
---
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
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel



[Index of Archives]     [Linux Driver Backports]     [DMA Engine]     [Linux GPIO]     [Linux SPI]     [Video for Linux]     [Linux USB Devel]     [Linux Coverity]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux