* [PATCH] staging: lustre: lustre/ldlm: Fixed sparse warnings @ 2016-09-09 15:20 Nayeemahmed Badebade 2016-09-12 10:27 ` [lustre-devel] " Greg KH 0 siblings, 1 reply; 16+ messages in thread From: Nayeemahmed Badebade @ 2016-09-09 15:20 UTC (permalink / raw) To: oleg.drokin, andreas.dilger, jsimmons Cc: gregkh, bruce.korb, Keith.Mannthey, lustre-devel, devel, linux-kernel 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] staging: lustre: lustre/ldlm: Fixed sparse warnings 2016-09-09 15:20 [PATCH] staging: lustre: lustre/ldlm: Fixed sparse warnings Nayeemahmed Badebade @ 2016-09-12 10:27 ` Greg KH 0 siblings, 0 replies; 16+ messages in thread From: Greg KH @ 2016-09-12 10:27 UTC (permalink / raw) To: Nayeemahmed Badebade Cc: oleg.drokin, andreas.dilger, jsimmons, Keith.Mannthey, devel, linux-kernel, bruce.korb, lustre-devel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [lustre-devel] [PATCH] staging: lustre: lustre/ldlm: Fixed sparse warnings @ 2016-09-12 10:27 ` Greg KH 0 siblings, 0 replies; 16+ messages in thread From: Greg KH @ 2016-09-12 10:27 UTC (permalink / raw) To: Nayeemahmed Badebade Cc: oleg.drokin, andreas.dilger, jsimmons, Keith.Mannthey, devel, linux-kernel, bruce.korb, lustre-devel 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] staging: lustre: lustre/ldlm: Fixed sparse warnings 2016-09-12 10:27 ` [lustre-devel] " Greg KH @ 2016-09-14 5:14 ` Dilger, Andreas -1 siblings, 0 replies; 16+ messages in thread From: Dilger, Andreas @ 2016-09-14 5:14 UTC (permalink / raw) To: Greg KH Cc: Nayeemahmed Badebade, devel, Linux Kernel Mailing List, Drokin, Oleg, James Simmons, Lustre Development List 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [lustre-devel] [PATCH] staging: lustre: lustre/ldlm: Fixed sparse warnings @ 2016-09-14 5:14 ` Dilger, Andreas 0 siblings, 0 replies; 16+ messages in thread From: Dilger, Andreas @ 2016-09-14 5:14 UTC (permalink / raw) To: Greg KH Cc: Nayeemahmed Badebade, devel, Linux Kernel Mailing List, Drokin, Oleg, James Simmons, Lustre Development List 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] staging: lustre: lustre/ldlm: Fixed sparse warnings 2016-09-14 5:14 ` [lustre-devel] " Dilger, Andreas (?) @ 2016-09-15 18:33 ` nayeem 2016-09-16 8:00 ` [lustre-devel] " Dilger, Andreas -1 siblings, 1 reply; 16+ messages in thread From: nayeem @ 2016-09-15 18:33 UTC (permalink / raw) To: Dilger, Andreas, Greg KH Cc: devel, Linux Kernel Mailing List, Drokin, Oleg, James Simmons, Lustre Development List [-- Attachment #1: Type: text/plain, Size: 3133 bytes --] 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 [-- Attachment #2: sparse-warnings-fix-patch-v2.patch --] [-- Type: text/x-patch, Size: 1401 bytes --] 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); ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH] staging: lustre: lustre/ldlm: Fixed sparse warnings 2016-09-15 18:33 ` nayeem @ 2016-09-16 8:00 ` Dilger, Andreas 0 siblings, 0 replies; 16+ messages in thread From: Dilger, Andreas @ 2016-09-16 8:00 UTC (permalink / raw) To: nayeem Cc: Greg KH, devel, Linux Kernel Mailing List, Drokin, Oleg, James Simmons, Lustre Development List 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> ^ permalink raw reply [flat|nested] 16+ messages in thread
* [lustre-devel] [PATCH] staging: lustre: lustre/ldlm: Fixed sparse warnings @ 2016-09-16 8:00 ` Dilger, Andreas 0 siblings, 0 replies; 16+ messages in thread From: Dilger, Andreas @ 2016-09-16 8:00 UTC (permalink / raw) To: nayeem Cc: Greg KH, devel, Linux Kernel Mailing List, Drokin, Oleg, James Simmons, Lustre Development List 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> ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] staging: lustre: lustre/ldlm: Fixed sparse warnings 2016-09-16 8:00 ` [lustre-devel] " Dilger, Andreas (?) @ 2016-09-18 20:21 ` nayeem 2016-09-18 20:29 ` [lustre-devel] " Dilger, Andreas -1 siblings, 1 reply; 16+ messages in thread From: nayeem @ 2016-09-18 20:21 UTC (permalink / raw) To: Dilger, Andreas Cc: Greg KH, devel, Linux Kernel Mailing List, Drokin, Oleg, James Simmons, Lustre Development List 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] staging: lustre: lustre/ldlm: Fixed sparse warnings 2016-09-18 20:21 ` nayeem @ 2016-09-18 20:29 ` Dilger, Andreas 0 siblings, 0 replies; 16+ messages in thread From: Dilger, Andreas @ 2016-09-18 20:29 UTC (permalink / raw) To: nayeem Cc: Greg KH, devel, Linux Kernel Mailing List, Drokin, Oleg, James Simmons, Lustre Development List 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [lustre-devel] [PATCH] staging: lustre: lustre/ldlm: Fixed sparse warnings @ 2016-09-18 20:29 ` Dilger, Andreas 0 siblings, 0 replies; 16+ messages in thread From: Dilger, Andreas @ 2016-09-18 20:29 UTC (permalink / raw) To: nayeem Cc: Greg KH, devel, Linux Kernel Mailing List, Drokin, Oleg, James Simmons, Lustre Development List 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 ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH v2] staging: lustre: lustre/ldlm: Fixed sparse warnings 2016-09-18 20:29 ` [lustre-devel] " Dilger, Andreas (?) @ 2016-09-18 21:18 ` Nayeemahmed Badebade 2016-09-18 21:27 ` [lustre-devel] " Dilger, Andreas 2016-09-19 20:43 ` [lustre-devel] " James Simmons -1 siblings, 2 replies; 16+ messages in thread From: Nayeemahmed Badebade @ 2016-09-18 21:18 UTC (permalink / raw) To: andreas.dilger, gregkh Cc: oleg.drokin, jsimmons, bruce.korb, lustre-devel, devel, linux-kernel 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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH v2] staging: lustre: lustre/ldlm: Fixed sparse warnings 2016-09-18 21:18 ` [PATCH v2] " Nayeemahmed Badebade @ 2016-09-18 21:27 ` Dilger, Andreas 2016-09-19 20:43 ` [lustre-devel] " James Simmons 1 sibling, 0 replies; 16+ messages in thread From: Dilger, Andreas @ 2016-09-18 21:27 UTC (permalink / raw) To: Nayeemahmed Badebade Cc: Greg KH, Drokin, Oleg, James Simmons, Lustre Development List, devel, Linux Kernel Mailing List > 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 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* [lustre-devel] [PATCH v2] staging: lustre: lustre/ldlm: Fixed sparse warnings @ 2016-09-18 21:27 ` Dilger, Andreas 0 siblings, 0 replies; 16+ messages in thread From: Dilger, Andreas @ 2016-09-18 21:27 UTC (permalink / raw) To: Nayeemahmed Badebade Cc: Greg KH, Drokin, Oleg, James Simmons, Lustre Development List, devel, Linux Kernel Mailing List > 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 > ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH v2] staging: lustre: lustre/ldlm: Fixed sparse warnings 2016-09-18 21:18 ` [PATCH v2] " Nayeemahmed Badebade @ 2016-09-19 20:43 ` James Simmons 2016-09-19 20:43 ` [lustre-devel] " James Simmons 1 sibling, 0 replies; 16+ messages in thread From: James Simmons @ 2016-09-19 20:43 UTC (permalink / raw) To: Nayeemahmed Badebade Cc: andreas.dilger, gregkh, oleg.drokin, bruce.korb, lustre-devel, devel, linux-kernel > 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 > > ^ permalink raw reply [flat|nested] 16+ messages in thread
* [lustre-devel] [PATCH v2] staging: lustre: lustre/ldlm: Fixed sparse warnings @ 2016-09-19 20:43 ` James Simmons 0 siblings, 0 replies; 16+ messages in thread From: James Simmons @ 2016-09-19 20:43 UTC (permalink / raw) To: Nayeemahmed Badebade Cc: andreas.dilger, gregkh, oleg.drokin, bruce.korb, lustre-devel, devel, linux-kernel > 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 > > ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2016-09-19 20:43 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-09-09 15:20 [PATCH] staging: lustre: lustre/ldlm: Fixed sparse warnings Nayeemahmed Badebade 2016-09-12 10:27 ` Greg KH 2016-09-12 10:27 ` [lustre-devel] " Greg KH 2016-09-14 5:14 ` Dilger, Andreas 2016-09-14 5:14 ` [lustre-devel] " Dilger, Andreas 2016-09-15 18:33 ` nayeem 2016-09-16 8:00 ` Dilger, Andreas 2016-09-16 8:00 ` [lustre-devel] " Dilger, Andreas 2016-09-18 20:21 ` nayeem 2016-09-18 20:29 ` Dilger, Andreas 2016-09-18 20:29 ` [lustre-devel] " Dilger, Andreas 2016-09-18 21:18 ` [PATCH v2] " Nayeemahmed Badebade 2016-09-18 21:27 ` Dilger, Andreas 2016-09-18 21:27 ` [lustre-devel] " Dilger, Andreas 2016-09-19 20:43 ` James Simmons 2016-09-19 20:43 ` [lustre-devel] " James Simmons
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.