Re: [PATCH] staging: lustre: lustre/ldlm: Fixed sparse warnings
On Sep 18, 2016, at 14:21, nayeem wrote: > On Friday 16 September 2016 01:30 PM, Dilger, Andreas wrote: >> On Sep 15, 2016, at 12:33, nayeem wrote: >>> On Wednesday 14 September 2016 10:44 AM, Dilger, Andreas wrote: On Sep 12, 2016, at 04:27, Greg KH 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 >> --- >> 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 > #include > > 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 c
Re: [PATCH] staging: lustre: lustre/ldlm: Fixed sparse warnings
On Friday 16 September 2016 01:30 PM, Dilger, Andreas wrote: On Sep 15, 2016, at 12:33, nayeem wrote: On Wednesday 14 September 2016 10:44 AM, Dilger, Andreas wrote: On Sep 12, 2016, at 04:27, Greg KH 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 --- 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 #include 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
Re: [PATCH] staging: lustre: lustre/ldlm: Fixed sparse warnings
On Sep 15, 2016, at 12:33, nayeem wrote: > On Wednesday 14 September 2016 10:44 AM, Dilger, Andreas wrote: >> On Sep 12, 2016, at 04:27, Greg KH 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 --- 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 > > ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: lustre: lustre/ldlm: Fixed sparse warnings
On Wednesday 14 September 2016 10:44 AM, Dilger, Andreas wrote: On Sep 12, 2016, at 04:27, Greg KH 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 --- 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 diff --git a/drivers/staging/lustre/lustre/include/lustre_dlm.h b/drivers/staging/lustre/lustre/include/lustre_dlm.h index 1ec4231..2ae463a 100644 --- a/drivers/staging/lustre/lustre/include/lustre_dlm.h +++ b/drivers/staging/lustre/lustre/include/lustre_dlm.h @@ -1293,6 +1293,7 @@ enum lock_res_type { /** Lock resource. */ static inline void lock_res(struct ldlm_resource *res) + __acquires(&res->lr_lock) { spin_lock(&res->lr_lock); } @@ -1306,6 +1307,7 @@ static inline void lock_res_nested(struct ldlm_resource *res, /** Unlock resource. */ static inline void unlock_res(struct ldlm_resource *res) + __releases(&res->lr_lock) { spin_unlock(&res->lr_lock); } diff --git a/drivers/staging/lustre/lustre/ldlm/l_lock.c b/drivers/staging/lustre/lustre/ldlm/l_lock.c index ea8840c..a887d9f 100644 --- a/drivers/staging/lustre/lustre/ldlm/l_lock.c +++ b/drivers/staging/lustre/lustre/ldlm/l_lock.c @@ -45,6 +45,7 @@ * being an atomic operation. */ struct ldlm_resource *lock_res_and_lock(struct ldlm_lock *lock) +__acquires(&lock->l_lock) { spin_lock(&lock->l_lock); @@ -59,6 +60,7 @@ 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_lock) { /* on server-side resource of lock doesn't change */ ldlm_clear_res_locked(lock); ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: lustre: lustre/ldlm: Fixed sparse warnings
On Sep 12, 2016, at 04:27, Greg KH 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 >> --- >> 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH] staging: lustre: lustre/ldlm: Fixed sparse warnings
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 > --- > 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 ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel