Re: Lockdep is less useful than it was
On Fri, Dec 08, 2017 at 10:14:38AM -0800, Matthew Wilcox wrote: > At the moment, the radix tree actively disables the RCU checking that > enabling lockdep would give us. It has to, because it has no idea what > lock protects any individual access to the radix tree. The XArray can > use the RCU checking because it knows that every reference is protected > by either the spinlock or the RCU lock. > > Dave was saying that he has a tree which has to be protected by a mutex > because of where it is in the locking hierarchy, and I was vigorously > declining his proposal of allowing him to skip taking the spinlock. Oh, I wasn't suggesting that you remove the internal tree locking because we need external locking. I was trying to point out that the internal locking doesn't remove the need for external locking, and that there are cases where smearing the internal lock outside the XA tree doesn't work, either. i.e. internal locking doesn't replace all the cases where external locking is required, and so it's less efficient than the existing radix tree code. What I was questioning was the value of replacing the radix tree code with a less efficient structure just to add lockdep validation to a tree that doesn't actually need any extra locking validation... Cheers, Dave. -- Dave Chinner da...@fromorbit.com
Re: Lockdep is less useful than it was
On Fri, Dec 08, 2017 at 10:14:38AM -0800, Matthew Wilcox wrote: > At the moment, the radix tree actively disables the RCU checking that > enabling lockdep would give us. It has to, because it has no idea what > lock protects any individual access to the radix tree. The XArray can > use the RCU checking because it knows that every reference is protected > by either the spinlock or the RCU lock. > > Dave was saying that he has a tree which has to be protected by a mutex > because of where it is in the locking hierarchy, and I was vigorously > declining his proposal of allowing him to skip taking the spinlock. Oh, I wasn't suggesting that you remove the internal tree locking because we need external locking. I was trying to point out that the internal locking doesn't remove the need for external locking, and that there are cases where smearing the internal lock outside the XA tree doesn't work, either. i.e. internal locking doesn't replace all the cases where external locking is required, and so it's less efficient than the existing radix tree code. What I was questioning was the value of replacing the radix tree code with a less efficient structure just to add lockdep validation to a tree that doesn't actually need any extra locking validation... Cheers, Dave. -- Dave Chinner da...@fromorbit.com
Re: Lockdep is less useful than it was
On Fri, Dec 08, 2017 at 10:27:17AM -0500, Theodore Ts'o wrote: > So if you are adding complexity to the kernel with the argument, > "lockdep will save us", I'm with Dave --- it's just not a believable > argument. I think that's a gross misrepresentation of what I'm doing. At the moment, the radix tree actively disables the RCU checking that enabling lockdep would give us. It has to, because it has no idea what lock protects any individual access to the radix tree. The XArray can use the RCU checking because it knows that every reference is protected by either the spinlock or the RCU lock. Dave was saying that he has a tree which has to be protected by a mutex because of where it is in the locking hierarchy, and I was vigorously declining his proposal of allowing him to skip taking the spinlock. And yes, we have bugs today that I assume we only stumble across every few billion years (or only on alpha, or only if our compiler gets more aggressive) because we have missing rcu_dereference annotations.
Re: Lockdep is less useful than it was
On Fri, Dec 08, 2017 at 10:27:17AM -0500, Theodore Ts'o wrote: > So if you are adding complexity to the kernel with the argument, > "lockdep will save us", I'm with Dave --- it's just not a believable > argument. I think that's a gross misrepresentation of what I'm doing. At the moment, the radix tree actively disables the RCU checking that enabling lockdep would give us. It has to, because it has no idea what lock protects any individual access to the radix tree. The XArray can use the RCU checking because it knows that every reference is protected by either the spinlock or the RCU lock. Dave was saying that he has a tree which has to be protected by a mutex because of where it is in the locking hierarchy, and I was vigorously declining his proposal of allowing him to skip taking the spinlock. And yes, we have bugs today that I assume we only stumble across every few billion years (or only on alpha, or only if our compiler gets more aggressive) because we have missing rcu_dereference annotations.
Re: Lockdep is less useful than it was
On Thu, Dec 07, 2017 at 02:38:03PM -0800, Matthew Wilcox wrote:
> I think it was a mistake to force these on for everybody; they have a
> much higher false-positive rate than the rest of lockdep, so as you say
> forcing them on leads to fewer people using *any* of lockdep.
>
> The bug you're hitting isn't Byungchul's fault; it's an annotation
> problem. The same kind of annotation problem that we used to have with
> dozens of other places in the kernel which are now fixed.
The question is whose responsibility is it to annotate the code? On
another thread it was suggested it was ext4's responsibility to add
annotations to avoid the false positives --- never the mind the fact
that every single file system is going to have add annotations.
Also note that the documentation for how to add annotations is
***horrible***. It's mostly, "try to figure out how other people
added magic cargo cult code which is not well defined (look at the
definitions of lockdep_set_class, lockdep_set_class_and_name,
lockdep_set_class_and_subclass, and lockdep_set_subclass, and weep) in
other subsystems and hope and pray it works for you."
And the explanation when there are failures, either false positives,
or not, are completely opaque. For example:
[ 16.190198] ext4lazyinit/648 is trying to acquire lock:
[ 16.191201] ((gendisk_completion)1 << part_shift(NUMA_NO_NODE)){+.+.}, at:
[<8a1ebe9d>] wait_for_completion_io+0x12/0x20
Just try to tell me that:
((gendisk_completion)1 << part_shift(NUMA_NO_NODE)){+.+.}
is human comprehensible with a straight face. And since the messages
don't even include the subclass/class/name key annotations, as lockdep
tries to handle things that are more and more complex, I'd argue it
has already crossed the boundary where unless you are a lockdep
developer, good luck trying to understand what is going on or how to
add annotations.
So if you are adding complexity to the kernel with the argument,
"lockdep will save us", I'm with Dave --- it's just not a believable
argument.
Cheers,
- Ted
Re: Lockdep is less useful than it was
On Thu, Dec 07, 2017 at 02:38:03PM -0800, Matthew Wilcox wrote:
> I think it was a mistake to force these on for everybody; they have a
> much higher false-positive rate than the rest of lockdep, so as you say
> forcing them on leads to fewer people using *any* of lockdep.
>
> The bug you're hitting isn't Byungchul's fault; it's an annotation
> problem. The same kind of annotation problem that we used to have with
> dozens of other places in the kernel which are now fixed.
The question is whose responsibility is it to annotate the code? On
another thread it was suggested it was ext4's responsibility to add
annotations to avoid the false positives --- never the mind the fact
that every single file system is going to have add annotations.
Also note that the documentation for how to add annotations is
***horrible***. It's mostly, "try to figure out how other people
added magic cargo cult code which is not well defined (look at the
definitions of lockdep_set_class, lockdep_set_class_and_name,
lockdep_set_class_and_subclass, and lockdep_set_subclass, and weep) in
other subsystems and hope and pray it works for you."
And the explanation when there are failures, either false positives,
or not, are completely opaque. For example:
[ 16.190198] ext4lazyinit/648 is trying to acquire lock:
[ 16.191201] ((gendisk_completion)1 << part_shift(NUMA_NO_NODE)){+.+.}, at:
[<8a1ebe9d>] wait_for_completion_io+0x12/0x20
Just try to tell me that:
((gendisk_completion)1 << part_shift(NUMA_NO_NODE)){+.+.}
is human comprehensible with a straight face. And since the messages
don't even include the subclass/class/name key annotations, as lockdep
tries to handle things that are more and more complex, I'd argue it
has already crossed the boundary where unless you are a lockdep
developer, good luck trying to understand what is going on or how to
add annotations.
So if you are adding complexity to the kernel with the argument,
"lockdep will save us", I'm with Dave --- it's just not a believable
argument.
Cheers,
- Ted
Re: Lockdep is less useful than it was
On Thu, Dec 07, 2017 at 02:38:03PM -0800, Matthew Wilcox wrote: > On Thu, Dec 07, 2017 at 11:06:34AM -0500, Theodore Ts'o wrote: > > The problem is that if it has too many false positives --- and it's > > gotten *way* worse with the completion callback "feature", people will > > just stop using Lockdep as being too annyoing and a waste of developer > > time when trying to figure what is a legitimate locking bug versus > > lockdep getting confused. > > > > I can't even disable the new Lockdep feature which is throwing > > lots of new false positives --- it's just all or nothing. > > You *can* ... but it's way more hacking Kconfig than you ought to have > to do (which is a separate rant ...) > > You need to get LOCKDEP_CROSSRELEASE off. I'd revert patches > e26f34a407aec9c65bce2bc0c838fabe4f051fc6 and > b483cf3bc249d7af706390efa63d6671e80d1c09 > > I think it was a mistake to force these on for everybody; they have a > much higher false-positive rate than the rest of lockdep, so as you say > forcing them on leads to fewer people using *any* of lockdep. > > The bug you're hitting isn't Byungchul's fault; it's an annotation > problem. The same kind of annotation problem that we used to have with > dozens of other places in the kernel which are now fixed. That's one of the fundamental problem with lockdep - it throws the difficulty of solving all these new false positives onto the developers who know nothing about lockdep and don't follow it's development. And until they do solve them - especially in critical subsystems that everyone uses like the storage stack - lockdep is essentially worthless. > If you didn't > have to hack Kconfig to get rid of this problem, you'd be happier, right? I'd be much happier if it wasn't turned on by default in the first place. We gave plenty of warnings that there were still unsolved false positive problems with the new checks in the storage stack. Cheers, Dave. -- Dave Chinner da...@fromorbit.com
Re: Lockdep is less useful than it was
On Thu, Dec 07, 2017 at 02:38:03PM -0800, Matthew Wilcox wrote: > On Thu, Dec 07, 2017 at 11:06:34AM -0500, Theodore Ts'o wrote: > > The problem is that if it has too many false positives --- and it's > > gotten *way* worse with the completion callback "feature", people will > > just stop using Lockdep as being too annyoing and a waste of developer > > time when trying to figure what is a legitimate locking bug versus > > lockdep getting confused. > > > > I can't even disable the new Lockdep feature which is throwing > > lots of new false positives --- it's just all or nothing. > > You *can* ... but it's way more hacking Kconfig than you ought to have > to do (which is a separate rant ...) > > You need to get LOCKDEP_CROSSRELEASE off. I'd revert patches > e26f34a407aec9c65bce2bc0c838fabe4f051fc6 and > b483cf3bc249d7af706390efa63d6671e80d1c09 > > I think it was a mistake to force these on for everybody; they have a > much higher false-positive rate than the rest of lockdep, so as you say > forcing them on leads to fewer people using *any* of lockdep. > > The bug you're hitting isn't Byungchul's fault; it's an annotation > problem. The same kind of annotation problem that we used to have with > dozens of other places in the kernel which are now fixed. That's one of the fundamental problem with lockdep - it throws the difficulty of solving all these new false positives onto the developers who know nothing about lockdep and don't follow it's development. And until they do solve them - especially in critical subsystems that everyone uses like the storage stack - lockdep is essentially worthless. > If you didn't > have to hack Kconfig to get rid of this problem, you'd be happier, right? I'd be much happier if it wasn't turned on by default in the first place. We gave plenty of warnings that there were still unsolved false positive problems with the new checks in the storage stack. Cheers, Dave. -- Dave Chinner da...@fromorbit.com
Re: Lockdep is less useful than it was
On Thu, Dec 07, 2017 at 02:38:03PM -0800, Matthew Wilcox wrote: > You need to get LOCKDEP_CROSSRELEASE off. I'd revert patches > e26f34a407aec9c65bce2bc0c838fabe4f051fc6 and > b483cf3bc249d7af706390efa63d6671e80d1c09 Oops. I meant to revert 2dcd5adfb7401b762ddbe4b86dcacc2f3de6b97b. Or you could cherry-pick b483cf3bc249d7af706390efa63d6671e80d1c09.
Re: Lockdep is less useful than it was
On Thu, Dec 07, 2017 at 02:38:03PM -0800, Matthew Wilcox wrote: > You need to get LOCKDEP_CROSSRELEASE off. I'd revert patches > e26f34a407aec9c65bce2bc0c838fabe4f051fc6 and > b483cf3bc249d7af706390efa63d6671e80d1c09 Oops. I meant to revert 2dcd5adfb7401b762ddbe4b86dcacc2f3de6b97b. Or you could cherry-pick b483cf3bc249d7af706390efa63d6671e80d1c09.
Lockdep is less useful than it was
On Thu, Dec 07, 2017 at 11:06:34AM -0500, Theodore Ts'o wrote: > The problem is that if it has too many false positives --- and it's > gotten *way* worse with the completion callback "feature", people will > just stop using Lockdep as being too annyoing and a waste of developer > time when trying to figure what is a legitimate locking bug versus > lockdep getting confused. > > I can't even disable the new Lockdep feature which is throwing > lots of new false positives --- it's just all or nothing. You *can* ... but it's way more hacking Kconfig than you ought to have to do (which is a separate rant ...) You need to get LOCKDEP_CROSSRELEASE off. I'd revert patches e26f34a407aec9c65bce2bc0c838fabe4f051fc6 and b483cf3bc249d7af706390efa63d6671e80d1c09 I think it was a mistake to force these on for everybody; they have a much higher false-positive rate than the rest of lockdep, so as you say forcing them on leads to fewer people using *any* of lockdep. The bug you're hitting isn't Byungchul's fault; it's an annotation problem. The same kind of annotation problem that we used to have with dozens of other places in the kernel which are now fixed. If you didn't have to hack Kconfig to get rid of this problem, you'd be happier, right?
Lockdep is less useful than it was
On Thu, Dec 07, 2017 at 11:06:34AM -0500, Theodore Ts'o wrote: > The problem is that if it has too many false positives --- and it's > gotten *way* worse with the completion callback "feature", people will > just stop using Lockdep as being too annyoing and a waste of developer > time when trying to figure what is a legitimate locking bug versus > lockdep getting confused. > > I can't even disable the new Lockdep feature which is throwing > lots of new false positives --- it's just all or nothing. You *can* ... but it's way more hacking Kconfig than you ought to have to do (which is a separate rant ...) You need to get LOCKDEP_CROSSRELEASE off. I'd revert patches e26f34a407aec9c65bce2bc0c838fabe4f051fc6 and b483cf3bc249d7af706390efa63d6671e80d1c09 I think it was a mistake to force these on for everybody; they have a much higher false-positive rate than the rest of lockdep, so as you say forcing them on leads to fewer people using *any* of lockdep. The bug you're hitting isn't Byungchul's fault; it's an annotation problem. The same kind of annotation problem that we used to have with dozens of other places in the kernel which are now fixed. If you didn't have to hack Kconfig to get rid of this problem, you'd be happier, right?
