Another sparse warning...

Another sparse warning...

[Anton Altaparmakov]

Hi,

Again, using latest code from git://git.kernel.org/pub/scm/linux/ 
kernel/git/josh/sparse.git

When I run:  make CHECKFLAGS="-D__CHECK_ENDIAN__" C=2 modules

I get this warning:

   CHECK   fs/ntfs/super.c
fs/ntfs/super.c:3003:2: warning: context imbalance in  
'ntfs_fill_super' - unexpected unlock

And ntfs_fill_super does on entry:

unlock_kernel();

And then does "lock_kernel();" on exit.

Which is what causes the warning as the warning goes away if I remove  
it...

The unlock+lock was added by Ingo Molnar IIRC because ntfs_fill_super 
() was causing a huge latency which is bad for RT code...

It is a perfectly legitimate thing to do as the kernel lock is held  
by the VFS but NTFS does not need it as it locks everything  
appropriately internally.

What can I do to tell sparse that this code is ok and there should  
not be a warning?  Is there a "__ignore_context_imbalance" or  
something that I can mark up the {un,}lock_kernel() with or something  
like that?

Thanks a lot in advance!

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer, http://www.linux-ntfs.org/

Re: Another sparse warning...

[Linus Torvalds]

On Mon, 12 Feb 2007, Anton Altaparmakov wrote:
> 
> What can I do to tell sparse that this code is ok and there should not be a
> warning?  Is there a "__ignore_context_imbalance" or something that I can mark
> up the {un,}lock_kernel() with or something like that?

No, you should annotate the function as already having the lock when 
entered.

What sparse warns about is not a locking imbalance (it's all balanced) but 
the fact that locking counts go negative (which is always a bug). And they 
go negative because you drop a lock that git didn't even know about 
(because it doesn't do global analysis - just per-function).

Now, you can either try to make sparsedo global analysis (which some 
people apparently really _are_ trying to do), or you can tell sparse that 
it holds a certain lock on entry, and is _supposed_ to release it. You can 
do that by annotating the function with __acquires/__releases():

	int myfunction(..)
		__releases(kernel_lock)
		__acquires(kernel_lock)
	{
		unlock_kernel();
		..
		lock_kernel();
	}

which does two things:

 - it shows the *programmer* that the function is doing somethign 
   "strange" (not really strange, but still: it's basically a fairly 
   readable way that it's doing locking in a weird way).

 - it tells sparse that the function is actually supposed to release the 
   lock and then re-acquire it.

This would also be how you annotate functions that really *are* 
unbalanced. Some functions get entered with a lock held, and release it, 
or the other way around (the simplest example of that is obviously the 
"__raw_spin_[un]lock()" functions themselves, but it's pretty common in 
"locking helper functions" too).

		Linus

Re: Another sparse warning...

[Anton Altaparmakov]

Hi Linus,

On 12 Feb 2007, at 15:50, Linus Torvalds wrote:
> On Mon, 12 Feb 2007, Anton Altaparmakov wrote:
>>
>> What can I do to tell sparse that this code is ok and there should  
>> not be a
>> warning?  Is there a "__ignore_context_imbalance" or something  
>> that I can mark
>> up the {un,}lock_kernel() with or something like that?
>
> No, you should annotate the function as already having the lock when
> entered.
[snip]
> 	int myfunction(..)
> 		__releases(kernel_lock)
> 		__acquires(kernel_lock)
> 	{
> 		unlock_kernel();
> 		..
> 		lock_kernel();
> 	}

Great, thanks!  That works a treat. (-:

Best regards,

	Anton
-- 
Anton Altaparmakov <aia21 at cam.ac.uk> (replace at with @)
Unix Support, Computing Service, University of Cambridge, CB2 3QH, UK
Linux NTFS maintainer, http://www.linux-ntfs.org/

Re: Another sparse warning...

[Christopher Li]

On Mon, Feb 12, 2007 at 07:50:24AM -0800, Linus Torvalds wrote:
> 
> Now, you can either try to make sparsedo global analysis (which some 
> people apparently really _are_ trying to do), or you can tell sparse that 
> it holds a certain lock on entry, and is _supposed_ to release it. You can 
> do that by annotating the function with __acquires/__releases():
> 
> 	int myfunction(..)
> 		__releases(kernel_lock)
> 		__acquires(kernel_lock)

Aha. That is the purpose of ctype member "context_list *context".
I was puzzling about it when I am reading the context checking code.

Without knowing the input/output context stuff, this syntax is
really cryptic.

# define __acquire(x)	__context__(x,1)
# define __acquires(x)	__attribute__((context(x,0,1)))
# define __releases(x)	__attribute__((context(x,1,0)))

>  - it shows the *programmer* that the function is doing somethign 
>    "strange" (not really strange, but still: it's basically a fairly 
>    readable way that it's doing locking in a weird way).

Should the function declare in the header file has that as well?
When call one of those functions, it can know that function will change
context.  That might be a way to solve the problem that some of the
spinlock function is not a inline function at all.

Chris

Re: Another sparse warning...

[Linus Torvalds]

On Mon, 12 Feb 2007, Christopher Li wrote:
>
> >  - it shows the *programmer* that the function is doing somethign 
> >    "strange" (not really strange, but still: it's basically a fairly 
> >    readable way that it's doing locking in a weird way).
> 
> Should the function declare in the header file has that as well?

Yes.

> When call one of those functions, it can know that function will change
> context.  That might be a way to solve the problem that some of the
> spinlock function is not a inline function at all.

I thought we did that already. I'm fairly sure I had this working at some 
point - exactly by having the calls just add up the (known) lock/unlock 
offsets.

		Linus

Re: Another sparse warning...

[Christopher Li]

On Mon, Feb 12, 2007 at 07:48:18PM -0800, Linus Torvalds wrote:
> > When call one of those functions, it can know that function will change
> > context.  That might be a way to solve the problem that some of the
> > spinlock function is not a inline function at all.
> 
> I thought we did that already. I'm fairly sure I had this working at some 
> point - exactly by having the calls just add up the (known) lock/unlock 
> offsets.
>

You are right. It is already there. I never see it before because my ctags
get confused about the context annotation:

void __lockfunc _spin_lock(spinlock_t *lock)		__acquires(lock); 

It generate tags for "lock" instead of "_spin_lock".

When I look up _spin_lock, it only shows the UP version. I never see
the SMP version of the _spin_lock.

Exactly why I want to have a ctags from sparse.

Chris
 

Re: Another sparse warning...

[Josh Triplett]

[-- Attachment #1: Type: text/plain, Size: 1471 bytes --]

Christopher Li wrote:
> On Mon, Feb 12, 2007 at 07:48:18PM -0800, Linus Torvalds wrote:
>>> When call one of those functions, it can know that function will change
>>> context.  That might be a way to solve the problem that some of the
>>> spinlock function is not a inline function at all.
>> I thought we did that already. I'm fairly sure I had this working at some 
>> point - exactly by having the calls just add up the (known) lock/unlock 
>> offsets.
> 
> You are right. It is already there. I never see it before because my ctags
> get confused about the context annotation:
> 
> void __lockfunc _spin_lock(spinlock_t *lock)		__acquires(lock); 
> 
> It generate tags for "lock" instead of "_spin_lock".

Interesting behavior. :)  That makes sense, given ctags' regex-based parsing.

About that syntax: I chose to use the parameter name there because I wanted to
use arbitrary expressions (like param_struct->lock) for a context, rather than
a parameter number (as used in printf annotations).  I'd love to see any
thoughts you might have on how best to parse that expression into something
with "placeholders" for formal parameters, and then easily substitute the
actual parameters at each call site to determine the expression for the
acquired or released context.  Then we need a solution for the more difficult
problem: unifying and tracking context expressions in the face of changing
data in those expressions.

- Josh Triplett


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]

Re: Another sparse warning...

[Josh Triplett]

[-- Attachment #1: Type: text/plain, Size: 1498 bytes --]

Linus Torvalds wrote:
> On Mon, 12 Feb 2007, Christopher Li wrote:
>>>  - it shows the *programmer* that the function is doing somethign 
>>>    "strange" (not really strange, but still: it's basically a fairly 
>>>    readable way that it's doing locking in a weird way).
>> Should the function declare in the header file has that as well?
> 
> Yes.
> 
>> When call one of those functions, it can know that function will change
>> context.  That might be a way to solve the problem that some of the
>> spinlock function is not a inline function at all.
> 
> I thought we did that already. I'm fairly sure I had this working at some 
> point - exactly by having the calls just add up the (known) lock/unlock 
> offsets.

On the other hand, the code currently does not check the context preconditions
of the callee; Sparse *could* check that any function requiring you to hold a
lock (have a context of 1) must only get called from a context in which you
hold that lock (have that context).  Instead, Sparse only checks the context
change, rather than the incoming context value at the call site.  It seems
trivial to add a check for context preconditions, and Linux could then have a
new annotation for __must_hold(lock) (usable on both functions and data), but
that check would also add numerous false positives caused by lack of
annotation, and that problem seems better solved by interprocedural analysis
than pervasive annotation.

Thoughts?

- Josh Triplett



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 252 bytes --]