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

[Dilger, Andreas]

On Sep 18, 2016, at 14:21, nayeem <itachi.opsrc@xxxxxxxxx> wrote:
> 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.

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