sparse context warning problem ...

sparse context warning problem ...

[David Brownell]

I just noticed this with some obviously correct code... and verified
that it's a regression in current GIT version of "sparse" since it
now rejects code which previously passed just fine.

The issue is a not-uncommon idiom, where a function must be called
with a lock held, and briefly drops it.  The way this has previously
been addressed, originally suggested by Linus and used in various
places in the kernel (but, I observe, not in the "sparse" internal
validation test cases) is:

  static void
  finish_urb(struct ohci_hcd *ohci, struct urb *urb, int status)
  __releases(ohci->lock)
  __acquires(ohci->lock)
  {
	...
  }

But current versions of "sparse" complain (wrongly):

  drivers/usb/host/ohci-q.c:66:2: warning: context imbalance in 'finish_urb': __context__ statement expected different context
  drivers/usb/host/ohci-q.c:66:2:    context '<noident>': wanted >= 0, got -1

Presumably this is this a known bug ... is a fix on the way?
(Meanwhile, I can just ignore this bogus output.)

- Dave

Re: sparse context warning problem ...

[Johannes Berg]

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

On Sat, 2008-05-10 at 17:24 -0700, David Brownell wrote:
> I just noticed this with some obviously correct code... and verified
> that it's a regression in current GIT version of "sparse" since it
> now rejects code which previously passed just fine.
> 
> The issue is a not-uncommon idiom, where a function must be called
> with a lock held, and briefly drops it.  The way this has previously
> been addressed, originally suggested by Linus and used in various
> places in the kernel (but, I observe, not in the "sparse" internal
> validation test cases) is:
> 
>   static void
>   finish_urb(struct ohci_hcd *ohci, struct urb *urb, int status)
>   __releases(ohci->lock)
>   __acquires(ohci->lock)
>   {
> 	...
>   }
> 
> But current versions of "sparse" complain (wrongly):
> 
>   drivers/usb/host/ohci-q.c:66:2: warning: context imbalance in 'finish_urb': __context__ statement expected different context
>   drivers/usb/host/ohci-q.c:66:2:    context '<noident>': wanted >= 0, got -1
> 
> Presumably this is this a known bug ... is a fix on the way?
> (Meanwhile, I can just ignore this bogus output.)

This is probably my mistake.

However, I took __releases and __acquires to mean that this function
*changed* the context, doing both doesn't really make much sense. I
think the function should actually be declared

static void
finish_urb(...)
__requires(ohci->lock)
{...}

where __requires is (for sparse) defined as

#define __requires(x) __attribute__((context(x,1,1)))

It's probably possible to merge the __acquires and __releases into one
though.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: sparse context warning problem ...

[David Brownell]

On Saturday 10 May 2008, Johannes Berg wrote:
> This is probably my mistake.
> 
> However, I took __releases and __acquires to mean that this function
> *changed* the context, doing both doesn't really make much sense. I
> think the function should actually be declared
> 
> static void
> finish_urb(...)
> __requires(ohci->lock)
> {...}
> 
> where __requires is (for sparse) defined as
> 
> #define __requires(x) __attribute__((context(x,1,1)))

ISTR suggesting special syntax for this to Linus (this was way
back when "sparse" was just starting) and he wanted to just do
it by having those two attributes.

So at this point, I'd want to see the regression fixed (and the
tests updated to avoid this in the future) before exploring any
alternative syntax for kernel annotations.


On the plus side, having syntax *IS* more general.  It can serve
as an annotation that the function requires particular locking
context, whether or not that context is explicitly accessed.

- Dave



> It's probably possible to merge the __acquires and __releases into one
> though.

Re: sparse context warning problem ...

[Johannes Berg]

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

On Sat, 2008-05-10 at 20:18 -0700, David Brownell wrote:
> On Saturday 10 May 2008, Johannes Berg wrote:
> > This is probably my mistake.
> > 
> > However, I took __releases and __acquires to mean that this function
> > *changed* the context, doing both doesn't really make much sense. I
> > think the function should actually be declared
> > 
> > static void
> > finish_urb(...)
> > __requires(ohci->lock)
> > {...}
> > 
> > where __requires is (for sparse) defined as
> > 
> > #define __requires(x) __attribute__((context(x,1,1)))
> 
> ISTR suggesting special syntax for this to Linus (this was way
> back when "sparse" was just starting) and he wanted to just do
> it by having those two attributes.
> 
> So at this point, I'd want to see the regression fixed (and the
> tests updated to avoid this in the future) before exploring any
> alternative syntax for kernel annotations.

Yeah, true, on the other hand, if/when Josh merges my other patches then
most kernel annotations will have to be changed anyway because up to
before the patch that is already in sparse wouldn't flag this:

spin_lock(&lock);
rcu_read_unlock();

because it didn't care *at all* about the expression inside __acquire()
and thus a lot of usage crept in that isn't really usable.

With my future patches, I'm even binding the symbols, so that

void spin_lock(spinlock_t l) __acquire(l);

will flag errors with

spin_lock(&a);
spin_unlock(&b);

while right now it would just treat them both as the context "l".

So I expect that it will not be possible to not "regress" in that sense
because the current annotations are simply messed up since sparse didn't
care about the name inside __acquire()/__release() at all!


However, it's probably fairly easy right now to treat both of them as
being merged together which seems the only sensible things to do anyway
if such a situation is encountered. I'll look into it.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: sparse context warning problem ...

[Johannes Berg]

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


> >   static void
> >   finish_urb(struct ohci_hcd *ohci, struct urb *urb, int status)
> >   __releases(ohci->lock)
> >   __acquires(ohci->lock)

> > But current versions of "sparse" complain (wrongly):
> > 
> >   drivers/usb/host/ohci-q.c:66:2: warning: context imbalance in 'finish_urb': __context__ statement expected different context
> >   drivers/usb/host/ohci-q.c:66:2:    context '<noident>': wanted >= 0, got -1


> However, I took __releases and __acquires to mean that this function
> *changed* the context, doing both doesn't really make much sense. I
> think the function should actually be declared
> 
> static void
> finish_urb(...)
> __requires(ohci->lock)
> {...}
> 
> where __requires is (for sparse) defined as
> 
> #define __requires(x) __attribute__((context(x,1,1)))

Actually, it turns out my analysis was completely wrong.

This is exactly the issue I pointed out in my other mail. You have:

__acquires(ohci->lock)
           ^^^^^^^^^^
but, on the other hand:

spin_unlock (&ohci->lock);
             ^

I think you can fix this particular case by adding the & in the
__acquires(), but that will only work for UP, for actual spinlocks my
other patches will be needed, because w/o my patches sparse will, on
SMP, not be able to see that

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

means to lock "&ohci->lock" when doing "spin_lock(&ohci->lock);" but
will treat it as locking the abstractly-named "lock" context, while on
UP/no-preempt the "spin_lock" macro is expanded by the preprocessor and
you will get "&ohci->lock" as the expression.

Ultimately, this whole problem comes from the fact that sparse accepted
adding an expression, documented it, but never complained if they
slightly mismatched as above.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: sparse context warning problem ...

[David Brownell]

On Tuesday 13 May 2008, Johannes Berg wrote:

> Actually, it turns out my analysis was completely wrong.
> 
> This is exactly the issue I pointed out in my other mail. You have:
> 
> __acquires(ohci->lock)
>            ^^^^^^^^^^
> but, on the other hand:
> 
> spin_unlock (&ohci->lock);
>              ^
> 
> I think you can fix this particular case by adding the & in the
> __acquires(), but that will only work for UP,

I used the OHCI example because it was a clear and readily
available/reproducible example of how this is a regression;
but I came across the problem with a different driver.  In
that driver, I just made sure that the strings were now
identical ... and the failures still came up with "sparse".

That's on a UP build.  (Albeit with PREEMPT and all the
debug options available ... since I'm debugging!)


> for actual spinlocks my 
> other patches will be needed, because w/o my patches sparse will, on
> SMP, not be able to see that
> 
> void __lockfunc _spin_lock(spinlock_t *lock)            __acquires(lock);
> 
> means to lock "&ohci->lock" when doing "spin_lock(&ohci->lock);" but
> will treat it as locking the abstractly-named "lock" context, while on
> UP/no-preempt the "spin_lock" macro is expanded by the preprocessor and
> you will get "&ohci->lock" as the expression.

Yeah, well the lock being acquired or released *IS* "ohci->lock",
but the spinlock calls don't take the lock, they take pointers
to it!

If you were to argue that understanding pointers like that is a
lot to demand of "sparse", I might agree.  But that won't change
the fact that locks themselves are not pointers to locks.  ;)


> Ultimately, this whole problem comes from the fact that sparse accepted
> adding an expression, documented it, but never complained if they
> slightly mismatched as above.

This still doesn't quite add up, though...

- Dave

--
To unsubscribe from this list: send the line "unsubscribe linux-sparse" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: sparse context warning problem ...

[Johannes Berg]

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


> I used the OHCI example because it was a clear and readily
> available/reproducible example of how this is a regression;
> but I came across the problem with a different driver.  In
> that driver, I just made sure that the strings were now
> identical ... and the failures still came up with "sparse".
> 
> That's on a UP build.  (Albeit with PREEMPT and all the
> debug options available ... since I'm debugging!)

I guess you get the spinlock_api_up.h version when building w/o SMP and
w/o CONFIG_DEBUG_SPINLOCK, but it doesn't really matter. The mismatch is
there one way or another.

> Yeah, well the lock being acquired or released *IS* "ohci->lock",
> but the spinlock calls don't take the lock, they take pointers
> to it!
> 
> If you were to argue that understanding pointers like that is a
> lot to demand of "sparse", I might agree.  But that won't change
> the fact that locks themselves are not pointers to locks.  ;)

Well, yes, but for purposes of comparing the expression it would be a
lot simpler to change the code since otherwise sparse would have to be
aware of whether a function takes a pointer to a lock or not. I don't
see why one couldn't use the context tracking stuff for something not
passed via pointers when the callee doesn't need to take/modify the
context.

> > Ultimately, this whole problem comes from the fact that sparse accepted
> > adding an expression, documented it, but never complained if they
> > slightly mismatched as above.
> 
> This still doesn't quite add up, though...

?

Take this to an old sparse:

__acquire(FOO);
__release(BAR);

and it will not complain about a context violation. My patches attempt
to change that, with fallouts.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: sparse context warning problem ...

[Johannes Berg]

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


> > means to lock "&ohci->lock" when doing "spin_lock(&ohci->lock);" but
> > will treat it as locking the abstractly-named "lock" context, while on
> > UP/no-preempt the "spin_lock" macro is expanded by the preprocessor and
> > you will get "&ohci->lock" as the expression.
> 
> Yeah, well the lock being acquired or released *IS* "ohci->lock",
> but the spinlock calls don't take the lock, they take pointers
> to it!
> 
> If you were to argue that understanding pointers like that is a
> lot to demand of "sparse", I might agree.  But that won't change
> the fact that locks themselves are not pointers to locks.  ;)

Yeah, I agree the locks aren't pointers, but really, it is a lot easier
for you to do that annotation than for sparse to figure it out.

I blame these problems on whoever implemented sparse context tracking in
the first place, documented the optional first expression parameter and
then didn't write any code for looking at the expression at all.

The patch series I have posted a while ago (I'll repost) would fix the
existing problems with the context tracking patch Josh put in
(especially the problem that

spin_lock_irqsave(&a);
spin_unlock_irqrestore(&a);

currently gives a warning on some kernel builds because one is a macro
and the other is not), but has its own problems again with the RESULT
"macro" (you can only use it on a declaration, not an implementation),
but that is completely new so not really a problem I think.


A problem my patch series introduces, though, is the static inline one:
Currently, static inlines are inlined right into the function when
testing for locks but that is a bit complicated with expression tracking
because the expressions inside of the static inline look completely
different due to variable passing. Hence, I change that to not go in
there but evaluate the static inlines themselves. However, this means
that this

static inline void netif_tx_lock_bh(struct net_device *dev)
{
        spin_lock_bh(&dev->_xmit_lock);
        dev->xmit_lock_owner = smp_processor_id();
}


will now trigger a warning and you have to annotate inlines as well
(also to get the user checked properly):

static inline void netif_tx_lock_bh(struct net_device *dev)
	__acquires(&dev->_xmit_lock)
{
        spin_lock_bh(&dev->_xmit_lock);
        dev->xmit_lock_owner = smp_processor_id();
}

Conceptually, I think this is cleaner because inlines are treated more
like real functions then, but you may disagree.


The status quo, however, has turned out to be unacceptable to users
because of the spinlock problem I mentioned above. Can we, based on an
evaluation of the problems I just outlined, either apply the patches I'm
about to send, or revert them to go back to the completely unchecked
contexts?

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: sparse context warning problem ...

[David Brownell]

On Thursday 29 May 2008, Johannes Berg wrote:
> 
> > > means to lock "&ohci->lock" when doing "spin_lock(&ohci->lock);" but
> > > will treat it as locking the abstractly-named "lock" context, while on
> > > UP/no-preempt the "spin_lock" macro is expanded by the preprocessor and
> > > you will get "&ohci->lock" as the expression.
> > 
> > Yeah, well the lock being acquired or released *IS* "ohci->lock",
> > but the spinlock calls don't take the lock, they take pointers
> > to it!
> > 
> > If you were to argue that understanding pointers like that is a
> > lot to demand of "sparse", I might agree.  But that won't change
> > the fact that locks themselves are not pointers to locks.  ;)
> 
> Yeah, I agree the locks aren't pointers, but really, it is a lot easier
> for you to do that annotation than for sparse to figure it out.

I was fairly sure you'd argue that!


> I blame these problems on whoever implemented sparse context tracking in
> the first place, documented the optional first expression parameter and
> then didn't write any code for looking at the expression at all.

Yeah, that Torvalds clown is always leaving stuff unfinished.  ;)


> static inline void netif_tx_lock_bh(struct net_device *dev)
> 	__acquires(&dev->_xmit_lock)
> {
>         spin_lock_bh(&dev->_xmit_lock);
>         dev->xmit_lock_owner = smp_processor_id();
> }
> 
> Conceptually, I think this is cleaner because inlines are treated more
> like real functions then, but you may disagree.

I disagree.  They *are* real functions, so treating them
as such is the only reasonable option!

(Note by the way a convenience there:  the lock is easily
described using just the function parameters.  I suspect
that will be common, but not always true ...)


Will it be possible to put annotations on methods declared
in ops vectors?  Just today I was cleaning up some locking
issues and had to start using a lock.  But I couldn't find
any clear statement about the call contexts for some of the
new lock/unlock calls ... I had to go look at other drivers
to see an answer (and hope they weren't buggy).  It would
have been nicer to just have "sparse" tell me the answers.

(If right now that seems more like a research problem, I'd
not be too surprised.  But I'd like to think that someday
it should be practical to have kernel lockdep tools tell
us about such things, and have such constraints verified
during builds.)

 
> The status quo, however, has turned out to be unacceptable to users
> because of the spinlock problem I mentioned above. Can we, based on an
> evaluation of the problems I just outlined, either apply the patches I'm
> about to send, or revert them to go back to the completely unchecked
> contexts?

Haven't seen the patches, but in principle I have no objection
to finishing the lock context code.  Fixing all the kernel code
to get better lock annotations will take time, but that's OK.

What's probably more important right now is to make sure the
annotations are simple and correct, so they can be built on
over time.

- Dave

Re: sparse context warning problem ...

[Johannes Berg]

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


> > Yeah, I agree the locks aren't pointers, but really, it is a lot easier
> > for you to do that annotation than for sparse to figure it out.
> 
> I was fairly sure you'd argue that!

Heh.

> > I blame these problems on whoever implemented sparse context tracking in
> > the first place, documented the optional first expression parameter and
> > then didn't write any code for looking at the expression at all.
> 
> Yeah, that Torvalds clown is always leaving stuff unfinished.  ;)

:P

> > static inline void netif_tx_lock_bh(struct net_device *dev)
> > 	__acquires(&dev->_xmit_lock)
> > {
> >         spin_lock_bh(&dev->_xmit_lock);
> >         dev->xmit_lock_owner = smp_processor_id();
> > }
> > 
> > Conceptually, I think this is cleaner because inlines are treated more
> > like real functions then, but you may disagree.
> 
> I disagree.  They *are* real functions, so treating them
> as such is the only reasonable option!

Well they used to be inlined in the context tracker too, and thus
typically do not have any annotations at all. So with these patches,
many new missing annotations show up.

> (Note by the way a convenience there:  the lock is easily
> described using just the function parameters.  I suspect
> that will be common, but not always true ...)

Yeah you can do this too though:

static inline lock_device(struct net_device *dev)
	__acquires(&dev->somelock)
{
	spin_lock(&dev->somelock);
}

and then "lock_device(a);" will increase the context "&a->somelock".

> Will it be possible to put annotations on methods declared
> in ops vectors?  Just today I was cleaning up some locking
> issues and had to start using a lock.  But I couldn't find
> any clear statement about the call contexts for some of the
> new lock/unlock calls ... I had to go look at other drivers
> to see an answer (and hope they weren't buggy).  It would
> have been nicer to just have "sparse" tell me the answers.
> 
> (If right now that seems more like a research problem, I'd
> not be too surprised.  But I'd like to think that someday
> it should be practical to have kernel lockdep tools tell
> us about such things, and have such constraints verified
> during builds.)

I'll have to try. Let's see... Nope, doesn't work, even with the
variable context tracking. I agree that tt should, in theory, be
possible to do this, but I haven't figured out the magic yet. However,
it seems it could be an extension of the third patch I just posted
("allow context() attribute on variables")
 
> > The status quo, however, has turned out to be unacceptable to users
> > because of the spinlock problem I mentioned above. Can we, based on an
> > evaluation of the problems I just outlined, either apply the patches I'm
> > about to send, or revert them to go back to the completely unchecked
> > contexts?
> 
> Haven't seen the patches, but in principle I have no objection
> to finishing the lock context code.  Fixing all the kernel code
> to get better lock annotations will take time, but that's OK.
> 
> What's probably more important right now is to make sure the
> annotations are simple and correct, so they can be built on
> over time.

Well, the foremost important thing is probably to figure out how we want
to name contexts, I'm using expressions such as "&dev->lock" for that,
and this doesn't actually track things like this properly:

dev = a;
spin_lock(&dev->lock);
dev = b;
spin_unlock(&dev->lock);

and will not complain in this case because the expression is different.
I didn't really think of that as much of a problem, but maybe it is? In
any case, if we want to allow that we'd have to use the pseudos for
context tracking and I fear that would be a lot more complex especially
wrt. function pointer passing, almost to the point where we'd have to
pretty much run the code.

johannes

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]