> > 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:
signature.asc
Description: This is a digitally signed message part
