Re: About the try to remove cross-release feature entirely by Ingo
On Fri, Jan 05, 2018 at 11:49:41AM -0500, bfields wrote: > On Mon, Jan 01, 2018 at 02:18:55AM -0800, Matthew Wilcox wrote: > > On Sat, Dec 30, 2017 at 06:00:57PM -0500, Theodore Ts'o wrote: > > > On Sat, Dec 30, 2017 at 05:40:28PM -0500, Theodore Ts'o wrote: > > > > On Sat, Dec 30, 2017 at 12:44:17PM -0800, Matthew Wilcox wrote: > > > > > > > > > > I'm not sure I agree with this part. What if we add a new TCP lock > > > > > class > > > > > for connections which are used for filesystems/network block > > > > > devices/...? > > > > > Yes, it'll be up to each user to set the lockdep classification > > > > > correctly, > > > > > but that's a relatively small number of places to add annotations, > > > > > and I don't see why it wouldn't work. > > > > > > > > I was exagerrating a bit for effect, I admit. (but only a bit). > > > > I feel like there's been rather too much of that recently. Can we stick > > to facts as far as possible, please? > > > > > > It can probably be for all TCP connections that are used by kernel > > > > code (as opposed to userspace-only TCP connections). But it would > > > > probably have to be each and every device-mapper instance, each and > > > > every block device, each and every mounted file system, each and every > > > > bdi object, etc. > > > > > > Clarification: all TCP connections that are used by kernel code would > > > need to be in their own separate lock class. All TCP connections used > > > only by userspace could be in their own shared lock class. You can't > > > use a one lock class for all kernel-used TCP connections, because of > > > the Network Block Device mounted on a local file system which is then > > > exported via NFS and squirted out yet another TCP connection problem. > > > > So the false positive you're concerned about is write-comes-in-over-NFS > > (with socket lock held), NFS sends a write request to local filesystem, > > I'm confused, what lock does Ted think the NFS server is holding over > NFS processing? Sorry, I meant "over RPC processing". I'll confess to no understanding of socket locking. The server RPC code doesn't take any itself except in a couple places on setup and tear down of a connection. We wouldn't actually want any exclusive per-connection lock held across RPC processing because we want to be able to handle multiple concurrent RPCs per connection. We do need a little locking just to make sure multiple server threads replying to the same client don't accidentally corrupt their replies by interleaving. But even there we're using our own lock, held only while transmitting the reply (after all the work's done and reply encoded). --b.
Re: About the try to remove cross-release feature entirely by Ingo
On Mon, Jan 01, 2018 at 02:18:55AM -0800, Matthew Wilcox wrote: > On Sat, Dec 30, 2017 at 06:00:57PM -0500, Theodore Ts'o wrote: > > On Sat, Dec 30, 2017 at 05:40:28PM -0500, Theodore Ts'o wrote: > > > On Sat, Dec 30, 2017 at 12:44:17PM -0800, Matthew Wilcox wrote: > > > > > > > > I'm not sure I agree with this part. What if we add a new TCP lock > > > > class > > > > for connections which are used for filesystems/network block > > > > devices/...? > > > > Yes, it'll be up to each user to set the lockdep classification > > > > correctly, > > > > but that's a relatively small number of places to add annotations, > > > > and I don't see why it wouldn't work. > > > > > > I was exagerrating a bit for effect, I admit. (but only a bit). > > I feel like there's been rather too much of that recently. Can we stick > to facts as far as possible, please? > > > > It can probably be for all TCP connections that are used by kernel > > > code (as opposed to userspace-only TCP connections). But it would > > > probably have to be each and every device-mapper instance, each and > > > every block device, each and every mounted file system, each and every > > > bdi object, etc. > > > > Clarification: all TCP connections that are used by kernel code would > > need to be in their own separate lock class. All TCP connections used > > only by userspace could be in their own shared lock class. You can't > > use a one lock class for all kernel-used TCP connections, because of > > the Network Block Device mounted on a local file system which is then > > exported via NFS and squirted out yet another TCP connection problem. > > So the false positive you're concerned about is write-comes-in-over-NFS > (with socket lock held), NFS sends a write request to local filesystem, I'm confused, what lock does Ted think the NFS server is holding over NFS processing? --b.
Re: About the try to remove cross-release feature entirely by Ingo
On 1/3/2018 5:10 PM, Byungchul Park wrote:
On 1/3/2018 4:05 PM, Theodore Ts'o wrote:
On Wed, Jan 03, 2018 at 11:10:37AM +0900, Byungchul Park wrote:
The point I was trying to drive home is that "all we have to do is
just classify everything well or just invalidate the right lock
Just to be sure, we don't have to invalidate lock objects at all but
a problematic waiter only.
So essentially you are proposing that we have to play "whack-a-mole"
as we find false positives, and where we may have to put in ad-hoc
plumbing to only invalidate "a problematic waiter" when it's
problematic --- or to entirely suppress the problematic waiter
If we have too many problematic completions(waiters) to handle it,
then I agree with you. But so far, only one exits and it seems able
to be handled even in the future on my own.
Or if you believe that we have a lot of those kind of completions
making trouble so we cannot handle it, the (4) by Amir would work,
no? I'm asking because I'm really curious about your opinion..
altogether. And in that case, a file system developer might be forced
to invalidate a lock/"waiter"/"completion" in another subsystem.
As I said, with regard to the invalidation, we don't have to
consider locks at all. It's enough to invalidate the waiter only.
I will also remind you that doing this will trigger a checkpatch.pl
*error*:
This is what we decided. And I think the decision is reasonable for
original lockdep. But I wonder if we should apply the same decision
on waiters. I don't insist but just wonder.
What if we adopt the (4) in which waiters are validated one by one
and no explicit invalidation is involved?
ERROR("LOCKDEP", "lockdep_no_validate class is reserved for
device->mutex.\n" . $herecurr);
- Ted
--
Thanks,
Byungchul
Re: About the try to remove cross-release feature entirely by Ingo
On 1/3/2018 4:05 PM, Theodore Ts'o wrote:
On Wed, Jan 03, 2018 at 11:10:37AM +0900, Byungchul Park wrote:
The point I was trying to drive home is that "all we have to do is
just classify everything well or just invalidate the right lock
Just to be sure, we don't have to invalidate lock objects at all but
a problematic waiter only.
So essentially you are proposing that we have to play "whack-a-mole"
as we find false positives, and where we may have to put in ad-hoc
plumbing to only invalidate "a problematic waiter" when it's
problematic --- or to entirely suppress the problematic waiter
If we have too many problematic completions(waiters) to handle it,
then I agree with you. But so far, only one exits and it seems able
to be handled even in the future on my own.
Or if you believe that we have a lot of those kind of completions
making trouble so we cannot handle it, the (4) by Amir would work,
no? I'm asking because I'm really curious about your opinion..
altogether. And in that case, a file system developer might be forced
to invalidate a lock/"waiter"/"completion" in another subsystem.
As I said, with regard to the invalidation, we don't have to
consider locks at all. It's enough to invalidate the waiter only.
I will also remind you that doing this will trigger a checkpatch.pl
*error*:
This is what we decided. And I think the decision is reasonable for
original lockdep. But I wonder if we should apply the same decision
on waiters. I don't insist but just wonder.
ERROR("LOCKDEP", "lockdep_no_validate class is reserved for device->mutex.\n" .
$herecurr);
- Ted
--
Thanks,
Byungchul
Re: About the try to remove cross-release feature entirely by Ingo
On Wed, Jan 03, 2018 at 11:10:37AM +0900, Byungchul Park wrote:
> > The point I was trying to drive home is that "all we have to do is
> > just classify everything well or just invalidate the right lock
>
> Just to be sure, we don't have to invalidate lock objects at all but
> a problematic waiter only.
So essentially you are proposing that we have to play "whack-a-mole"
as we find false positives, and where we may have to put in ad-hoc
plumbing to only invalidate "a problematic waiter" when it's
problematic --- or to entirely suppress the problematic waiter
altogether. And in that case, a file system developer might be forced
to invalidate a lock/"waiter"/"completion" in another subsystem.
I will also remind you that doing this will trigger a checkpatch.pl
*error*:
ERROR("LOCKDEP", "lockdep_no_validate class is reserved for device->mutex.\n" .
$herecurr);
- Ted
Re: About the try to remove cross-release feature entirely by Ingo
On 1/3/2018 11:58 AM, Dave Chinner wrote: On Wed, Jan 03, 2018 at 11:28:44AM +0900, Byungchul Park wrote: On 1/1/2018 7:18 PM, Matthew Wilcox wrote: On Sat, Dec 30, 2017 at 06:00:57PM -0500, Theodore Ts'o wrote: Also, what to do with TCP connections which are created in userspace (with some authentication exchanges happening in userspace), and then passed into kernel space for use in kernel space, is an interesting question. Yes! I'd love to have a lockdep expert weigh in here. I believe it's legitimate to change a lock's class after it's been used, essentially destroying it and reinitialising it. If not, it should be because it's a reasonable design for an object to need different lock classes for different phases of its existance. I also think it should be done ultimately. And I think it's very much hard since it requires to change the dependency graph of lockdep but anyway possible. It's up to lockdep maintainer's will though.. We used to do this in XFS to work around the fact that the memory reclaim context "locks" were too stupid to understand that an object referenced and locked above memory allocation could not be accessed below in memory reclaim because memory reclaim only accesses /unreferenced objects/. We played whack-a-mole with lockdep for years to get most of the false positives sorted out. Hence for a long time we had to re-initialise the lock context for the XFS inode iolock in ->evict_inode() so we could lock it for reclaim processing. Eventually we ended up completely reworking the inode reclaim locking in XFS primarily to get rid of all the nasty lockdep hacks we had strewn throughout the code. It was ~2012 we got rid of the last inode re-init code, IIRC. Yeah: commit 4f59af758f9092bc7b266ca919ce6067170e5172 Author: Christoph Hellwig Date: Wed Jul 4 11:13:33 2012 -0400 xfs: remove iolock lock classes Now that we never take the iolock during inode reclaim we don't need to play games with lock classes. Signed-off-by: Christoph Hellwig Reviewed-by: Rich Johnston Signed-off-by: Ben Myers We still have problems with lockdep false positives w.r.t. memory allocation contexts, mainly with code that can be called from both above and below memory allocation contexts. We've finally got __GFP_NOLOCKDEP to be able to annotate memory allocation points within such code paths, but that doesn't help with locks Byungchul, lockdep has a long, long history of having sharp edges and being very unfriendly to developers. We've all been scarred by lockdep at one time or another and so there's a fair bit of resistance to repeating past mistakes and allowing lockdep to inflict more scars on us As I understand what you suffered from.. I don't really want to force it forward strongly. So far, all problems have been handled by myself including the final one e.i. the completion in submit_bio_wait() with the invalidation if it's allowed. But yes, who knows the future? In the future, that terrible thing you mentioned might or might not happen because of cross-release. I just felt like someone was misunderstanding what the problem came from, what the problem was, how we could avoid it, why cross-release should be removed and so on.. I believe the 3 ways I suggested can help, but I don't want to strongly insist if all of you don't think so. Thanks a lot anyway for your opinion. -- Thanks, Byungchul
Re: About the try to remove cross-release feature entirely by Ingo
On Wed, Jan 03, 2018 at 11:28:44AM +0900, Byungchul Park wrote: > On 1/1/2018 7:18 PM, Matthew Wilcox wrote: > >On Sat, Dec 30, 2017 at 06:00:57PM -0500, Theodore Ts'o wrote: > >>Also, what to do with TCP connections which are created in userspace > >>(with some authentication exchanges happening in userspace), and then > >>passed into kernel space for use in kernel space, is an interesting > >>question. > > > >Yes! I'd love to have a lockdep expert weigh in here. I believe it's > >legitimate to change a lock's class after it's been used, essentially > >destroying it and reinitialising it. If not, it should be because it's > >a reasonable design for an object to need different lock classes for > >different phases of its existance. > > I also think it should be done ultimately. And I think it's very much > hard since it requires to change the dependency graph of lockdep but > anyway possible. It's up to lockdep maintainer's will though.. We used to do this in XFS to work around the fact that the memory reclaim context "locks" were too stupid to understand that an object referenced and locked above memory allocation could not be accessed below in memory reclaim because memory reclaim only accesses /unreferenced objects/. We played whack-a-mole with lockdep for years to get most of the false positives sorted out. Hence for a long time we had to re-initialise the lock context for the XFS inode iolock in ->evict_inode() so we could lock it for reclaim processing. Eventually we ended up completely reworking the inode reclaim locking in XFS primarily to get rid of all the nasty lockdep hacks we had strewn throughout the code. It was ~2012 we got rid of the last inode re-init code, IIRC. Yeah: commit 4f59af758f9092bc7b266ca919ce6067170e5172 Author: Christoph Hellwig Date: Wed Jul 4 11:13:33 2012 -0400 xfs: remove iolock lock classes Now that we never take the iolock during inode reclaim we don't need to play games with lock classes. Signed-off-by: Christoph Hellwig Reviewed-by: Rich Johnston Signed-off-by: Ben Myers We still have problems with lockdep false positives w.r.t. memory allocation contexts, mainly with code that can be called from both above and below memory allocation contexts. We've finally got __GFP_NOLOCKDEP to be able to annotate memory allocation points within such code paths, but that doesn't help with locks Byungchul, lockdep has a long, long history of having sharp edges and being very unfriendly to developers. We've all been scarred by lockdep at one time or another and so there's a fair bit of resistance to repeating past mistakes and allowing lockdep to inflict more scars on us Cheers, Dave. -- Dave Chinner da...@fromorbit.com
Re: About the try to remove cross-release feature entirely by Ingo
On 1/2/2018 1:00 AM, Theodore Ts'o wrote: On Mon, Jan 01, 2018 at 02:18:55AM -0800, Matthew Wilcox wrote: Clarification: all TCP connections that are used by kernel code would need to be in their own separate lock class. All TCP connections used only by userspace could be in their own shared lock class. You can't use a one lock class for all kernel-used TCP connections, because of the Network Block Device mounted on a local file system which is then exported via NFS and squirted out yet another TCP connection problem. So the false positive you're concerned about is write-comes-in-over-NFS (with socket lock held), NFS sends a write request to local filesystem, local filesystem sends write to block device, block device sends a packet to a socket which takes that socket lock. It's not just the socket lock, but any of the locks/mutexes/"waiters" that might be taken in the TCP code path and below, including in the NIC driver. I don't think we need to be as drastic as giving each socket its own lock class to solve this. All NFS sockets can be in lock class A; all NBD sockets can be in lock class B; all user sockets can be in lock class C; etc. But how do you know which of the locks taken in the networking stack are for the NBD versus the NFS sockets? What manner of horrific abstraction violation is going to pass that information all the way down to all of the locks that might be taken at the socket layer and below? How is this "proper clasification" supposed to happen? It's the repeated handwaving which claims this is easy which is rather frustrating. The simple thing is to use a unique ID which is bumped for each struct sock, each struct super, struct block_device, struct request_queue, struct bdi, etc, but that runs into lockdep scalability issues. This is what I mentioned with group ID in an example for you before. To do that, the most important thing is to prevent running into lockdep scalability. -- Thanks, Byungchul
Re: About the try to remove cross-release feature entirely by Ingo
On 1/1/2018 7:18 PM, Matthew Wilcox wrote: On Sat, Dec 30, 2017 at 06:00:57PM -0500, Theodore Ts'o wrote: On Sat, Dec 30, 2017 at 05:40:28PM -0500, Theodore Ts'o wrote: On Sat, Dec 30, 2017 at 12:44:17PM -0800, Matthew Wilcox wrote: I'm not sure I agree with this part. What if we add a new TCP lock class for connections which are used for filesystems/network block devices/...? Yes, it'll be up to each user to set the lockdep classification correctly, but that's a relatively small number of places to add annotations, and I don't see why it wouldn't work. I was exagerrating a bit for effect, I admit. (but only a bit). I feel like there's been rather too much of that recently. Can we stick to facts as far as possible, please? It can probably be for all TCP connections that are used by kernel code (as opposed to userspace-only TCP connections). But it would probably have to be each and every device-mapper instance, each and every block device, each and every mounted file system, each and every bdi object, etc. Clarification: all TCP connections that are used by kernel code would need to be in their own separate lock class. All TCP connections used only by userspace could be in their own shared lock class. You can't use a one lock class for all kernel-used TCP connections, because of the Network Block Device mounted on a local file system which is then exported via NFS and squirted out yet another TCP connection problem. So the false positive you're concerned about is write-comes-in-over-NFS (with socket lock held), NFS sends a write request to local filesystem, local filesystem sends write to block device, block device sends a packet to a socket which takes that socket lock. I don't think we need to be as drastic as giving each socket its own lock class to solve this. All NFS sockets can be in lock class A; all NBD sockets can be in lock class B; all user sockets can be in lock class C; etc. Also, what to do with TCP connections which are created in userspace (with some authentication exchanges happening in userspace), and then passed into kernel space for use in kernel space, is an interesting question. Yes! I'd love to have a lockdep expert weigh in here. I believe it's legitimate to change a lock's class after it's been used, essentially destroying it and reinitialising it. If not, it should be because it's a reasonable design for an object to need different lock classes for different phases of its existance. I also think it should be done ultimately. And I think it's very much hard since it requires to change the dependency graph of lockdep but anyway possible. It's up to lockdep maintainer's will though.. -- Thanks, Byungchul
Re: About the try to remove cross-release feature entirely by Ingo
On 12/31/2017 7:40 AM, Theodore Ts'o wrote: On Sat, Dec 30, 2017 at 12:44:17PM -0800, Matthew Wilcox wrote: I'm not sure I agree with this part. What if we add a new TCP lock class for connections which are used for filesystems/network block devices/...? Yes, it'll be up to each user to set the lockdep classification correctly, but that's a relatively small number of places to add annotations, and I don't see why it wouldn't work. I was exagerrating a bit for effect, I admit. (but only a bit). It can probably be for all TCP connections that are used by kernel code (as opposed to userspace-only TCP connections). But it would probably have to be each and every device-mapper instance, each and every block device, each and every mounted file system, each and every bdi object, etc. The point I was trying to drive home is that "all we have to do is just classify everything well or just invalidate the right lock Just to be sure, we don't have to invalidate lock objects at all but a problematic waiter only. objects" is a massive understatement of the complexity level of what would be required, or the number of locks/completion handlers that would have to be blacklisted. - Ted -- Thanks, Byungchul
Re: About the try to remove cross-release feature entirely by Ingo
On 12/31/2017 12:40 AM, Theodore Ts'o wrote: On Fri, Dec 29, 2017 at 10:16:24PM -0800, Matthew Wilcox wrote: I disagree here. As Ted says, it's the interactions between the subsystems that leads to problems. Everything's goig to work great until somebody does something in a way that's never been tried before. The question what is classified *well* mean? At the extreme, we could put the locks for every single TCP connection into their own lockdep class. But that would blow the limits in terms of the number of locks out of the water super-quickly --- and it would destroy the ability for lockdep to learn what the proper locking order should be. Yet given Lockdep's current implementation, the only way to guarantee that there won't be any interactions between subsystems that cause false positives would be to categorizes locks for each TCP connection into their own class. So this is why I get a little annoyed when you say, "it's just a matter of classification". NO IT IS NOT. We can not possibly classify things "correctly" to completely limit false positives without completely destroying lockdep's scalability as it is currently You seem to admit that it can be solved by proper classification but say that it's *not realistic* because of the limitation of lockdep. Right? I've agreed with you for that point. I also think it's very hard to do it because of the lockdep design and the only way might be to fix lockdep fundamentally, that may be the one we should do ultimately. Is it the best decision to keep it removed until lockdep get fixed fundamentally? If I believe it were, I would have kept quiet. But, I don't think so. Almost other users had already gotten benifit from it except the special case. And it would be appriciated if you remind that I suggested 3 methods + 1 (by Amir) before for that reason. I don't want to force it forward but just want the facts to be shared. I felt like I failed it because of the lack of explanation. As far as the "just invalidate the waiter", the problem is that it requires source level changes to invalidate the waiter, and for Or, no change is needed if we adopt the (4)th option (by Amir), in which we keep waiters invalidated by default and validate waiters explicitly only when it needs. different use cases, we will need to validate different waiters. For example, in the example I gave, we would have to invalidate *all* TCP waiters/locks in order to prevent false positives. But that makes the No. Only invalidating waiters is enough. For now, the waiter in submit_bio_wait() is the only one to invalidate. lockdep useless for all TCP locks. What's the solution? I claim that Even if we invalidate waiters, TCP locks can still work with lockdep. Invalidating waiters *never* affect lockdep checking for typical locks at all. The only way it can work is to either dump it on the reposibility of the people debugging lockdep reports to make source level changes to other subsystems which they aren't the maintainers of to suppress false positives that arise due to how the subsystems are being used together in their particular configuration or you can try to You seem to misunderstand it a lot.. The only thing we have to is to use init_completion_nomap() instead of init_completion() for the problematic completion object. So far, the completion in submit_bio_wait() has been the only one. If you belive that we have a lot of problematic completions(waiters) so that we cannot handle it, the (4) by Amir can be an option. Just to be sure, there were several false positives by cross-release. Something was due to confliction between manual acquire()s added before and automatic cross-release, both of which are for detecting deadlocks by a specific completion(waiter). Or, something was solved by classifying locks properly simply. And this case of submit_bio_wait() is the first case that we cannot classify locks simply and need to consider other options. -- Thanks, Byungchul
Re: About the try to remove cross-release feature entirely by Ingo
On 12/30/2017 3:16 PM, Matthew Wilcox wrote: On Fri, Dec 29, 2017 at 04:28:51PM +0900, Byungchul Park wrote: On Thu, Dec 28, 2017 at 10:51:46PM -0500, Theodore Ts'o wrote: On Fri, Dec 29, 2017 at 10:47:36AM +0900, Byungchul Park wrote: (1) The best way: To classify all waiters correctly. It's really not all waiters, but all *locks*, no? Thanks for your opinion. I will add my opinion on you. I meant *waiters*. Locks are only a sub set of potential waiters, which actually cause deadlocks. Cross-release was designed to consider the super set including all general waiters such as typical locks, wait_for_completion(), and lock_page() and so on.. I think this is a terminology problem. To me (and, I suspect Ted), a waiter is a subject of a verb while a lock is an object. So Ted is asking whether we have to classify the users, while I think you're saying we have extra objects to classify. I'd be comfortable continuing to refer to completions as locks. We could try to come up with a new object name like waitpoints though? Right. Then "event" should be used as an object name than "waiter". The problems come from wrong classification. Waiters either classfied well or invalidated properly won't bitrot. I disagree here. As Ted says, it's the interactions between the As you know, the classification is something already considering the interactions between the subsystems and classified. But, yes. That is just what we have to do untimately but not what we can do right away. That's why I suggested all 3 ways + 1 way (by Amir). subsystems that leads to problems. Everything's goig to work great until somebody does something in a way that's never been tried before. Yes. Everything has worked great so far, since the classification by now is done well enough at least to avoid such problems, not perfect though. IMO, the classification does not have to be perfect but needs to be good enough to work. -- Thanks, Byungchul
Re: About the try to remove cross-release feature entirely by Ingo
On Mon, Jan 01, 2018 at 02:18:55AM -0800, Matthew Wilcox wrote: > > Clarification: all TCP connections that are used by kernel code would > > need to be in their own separate lock class. All TCP connections used > > only by userspace could be in their own shared lock class. You can't > > use a one lock class for all kernel-used TCP connections, because of > > the Network Block Device mounted on a local file system which is then > > exported via NFS and squirted out yet another TCP connection problem. > > So the false positive you're concerned about is write-comes-in-over-NFS > (with socket lock held), NFS sends a write request to local filesystem, > local filesystem sends write to block device, block device sends a > packet to a socket which takes that socket lock. It's not just the socket lock, but any of the locks/mutexes/"waiters" that might be taken in the TCP code path and below, including in the NIC driver. > I don't think we need to be as drastic as giving each socket its own lock > class to solve this. All NFS sockets can be in lock class A; all NBD > sockets can be in lock class B; all user sockets can be in lock class > C; etc. But how do you know which of the locks taken in the networking stack are for the NBD versus the NFS sockets? What manner of horrific abstraction violation is going to pass that information all the way down to all of the locks that might be taken at the socket layer and below? How is this "proper clasification" supposed to happen? It's the repeated handwaving which claims this is easy which is rather frustrating. The simple thing is to use a unique ID which is bumped for each struct sock, each struct super, struct block_device, struct request_queue, struct bdi, etc, but that runs into lockdep scalability issues. Anything else means that you have to somehow pass down through the layers so that, in the general case, the socket knows that it is "an NFS socket" versus "an NBD socket" --- and remember, if there is any kind of completion handling done in the NIC driver, it's going to have to passed down well below the TCP layer all the way down to the network device drivers. Or is the plan to do this add a bit ad hoc of plumbing for each false positive which cross-release lockdep failures are reported? > > Also, what to do with TCP connections which are created in userspace > > (with some authentication exchanges happening in userspace), and then > > passed into kernel space for use in kernel space, is an interesting > > question. > > Yes! I'd love to have a lockdep expert weigh in here. I believe it's > legitimate to change a lock's class after it's been used, essentially > destroying it and reinitialising it. If not, it should be because it's > a reasonable design for an object to need different lock classes for > different phases of its existance. We just also need to be destroy a lock class after the transient object has been deleted. This is especially true for file system testing, since we are constantly mounting and unmounting file systems, and creating and destroying loop devices, potentially hundreds or thousands of times during a test run. So if we have to create a unique lock class for "proper classification" each time a file system is mounted, or loop device or device-mapper device (dm-error, etc.) is created, we'll run into lockdep scalability issues really quickly. So this is yet another example where the handwaving, "all you have to do is proper classification" just doesn't work. > > So "all you have to do is classify the locks 'properly'" is much like > > the apocrophal, "all you have to do is bell the cat"[1]. Or like the > > saying, "colonizing the stars is *easy*; all you have to do is figure > > out faster than light travel." > > This is only computer programming, not rocket surgery :-) Given the current state of the lockdep technology, merging cross-lock certainly feels like requiring the use of sledgehammers to do rocket surgery in order to avoid false positives --- sorry, "proper classification". - Ted
Re: About the try to remove cross-release feature entirely by Ingo
On Sat, Dec 30, 2017 at 06:00:57PM -0500, Theodore Ts'o wrote: > On Sat, Dec 30, 2017 at 05:40:28PM -0500, Theodore Ts'o wrote: > > On Sat, Dec 30, 2017 at 12:44:17PM -0800, Matthew Wilcox wrote: > > > > > > I'm not sure I agree with this part. What if we add a new TCP lock class > > > for connections which are used for filesystems/network block devices/...? > > > Yes, it'll be up to each user to set the lockdep classification correctly, > > > but that's a relatively small number of places to add annotations, > > > and I don't see why it wouldn't work. > > > > I was exagerrating a bit for effect, I admit. (but only a bit). I feel like there's been rather too much of that recently. Can we stick to facts as far as possible, please? > > It can probably be for all TCP connections that are used by kernel > > code (as opposed to userspace-only TCP connections). But it would > > probably have to be each and every device-mapper instance, each and > > every block device, each and every mounted file system, each and every > > bdi object, etc. > > Clarification: all TCP connections that are used by kernel code would > need to be in their own separate lock class. All TCP connections used > only by userspace could be in their own shared lock class. You can't > use a one lock class for all kernel-used TCP connections, because of > the Network Block Device mounted on a local file system which is then > exported via NFS and squirted out yet another TCP connection problem. So the false positive you're concerned about is write-comes-in-over-NFS (with socket lock held), NFS sends a write request to local filesystem, local filesystem sends write to block device, block device sends a packet to a socket which takes that socket lock. I don't think we need to be as drastic as giving each socket its own lock class to solve this. All NFS sockets can be in lock class A; all NBD sockets can be in lock class B; all user sockets can be in lock class C; etc. > Also, what to do with TCP connections which are created in userspace > (with some authentication exchanges happening in userspace), and then > passed into kernel space for use in kernel space, is an interesting > question. Yes! I'd love to have a lockdep expert weigh in here. I believe it's legitimate to change a lock's class after it's been used, essentially destroying it and reinitialising it. If not, it should be because it's a reasonable design for an object to need different lock classes for different phases of its existance. > So "all you have to do is classify the locks 'properly'" is much like > the apocrophal, "all you have to do is bell the cat"[1]. Or like the > saying, "colonizing the stars is *easy*; all you have to do is figure > out faster than light travel." This is only computer programming, not rocket surgery :-)
Re: About the try to remove cross-release feature entirely by Ingo
On Sat, Dec 30, 2017 at 05:40:28PM -0500, Theodore Ts'o wrote: > On Sat, Dec 30, 2017 at 12:44:17PM -0800, Matthew Wilcox wrote: > > > > I'm not sure I agree with this part. What if we add a new TCP lock class > > for connections which are used for filesystems/network block devices/...? > > Yes, it'll be up to each user to set the lockdep classification correctly, > > but that's a relatively small number of places to add annotations, > > and I don't see why it wouldn't work. > > I was exagerrating a bit for effect, I admit. (but only a bit). > > It can probably be for all TCP connections that are used by kernel > code (as opposed to userspace-only TCP connections). But it would > probably have to be each and every device-mapper instance, each and > every block device, each and every mounted file system, each and every > bdi object, etc. Clarification: all TCP connections that are used by kernel code would need to be in their own separate lock class. All TCP connections used only by userspace could be in their own shared lock class. You can't use a one lock class for all kernel-used TCP connections, because of the Network Block Device mounted on a local file system which is then exported via NFS and squirted out yet another TCP connection problem. Also, what to do with TCP connections which are created in userspace (with some authentication exchanges happening in userspace), and then passed into kernel space for use in kernel space, is an interesting question. So "all you have to do is classify the locks 'properly'" is much like the apocrophal, "all you have to do is bell the cat"[1]. Or like the saying, "colonizing the stars is *easy*; all you have to do is figure out faster than light travel." [1] https://en.wikipedia.org/wiki/Belling_the_cat - Ted
Re: About the try to remove cross-release feature entirely by Ingo
On Sat, Dec 30, 2017 at 12:44:17PM -0800, Matthew Wilcox wrote: > > I'm not sure I agree with this part. What if we add a new TCP lock class > for connections which are used for filesystems/network block devices/...? > Yes, it'll be up to each user to set the lockdep classification correctly, > but that's a relatively small number of places to add annotations, > and I don't see why it wouldn't work. I was exagerrating a bit for effect, I admit. (but only a bit). It can probably be for all TCP connections that are used by kernel code (as opposed to userspace-only TCP connections). But it would probably have to be each and every device-mapper instance, each and every block device, each and every mounted file system, each and every bdi object, etc. The point I was trying to drive home is that "all we have to do is just classify everything well or just invalidate the right lock objects" is a massive understatement of the complexity level of what would be required, or the number of locks/completion handlers that would have to be blacklisted. - Ted
Re: About the try to remove cross-release feature entirely by Ingo
On Sat, Dec 30, 2017 at 10:40:41AM -0500, Theodore Ts'o wrote: > On Fri, Dec 29, 2017 at 10:16:24PM -0800, Matthew Wilcox wrote: > > > The problems come from wrong classification. Waiters either classfied > > > well or invalidated properly won't bitrot. > > > > I disagree here. As Ted says, it's the interactions between the > > subsystems that leads to problems. Everything's goig to work great > > until somebody does something in a way that's never been tried before. > > The question what is classified *well* mean? At the extreme, we could > put the locks for every single TCP connection into their own lockdep > class. But that would blow the limits in terms of the number of locks > out of the water super-quickly --- and it would destroy the ability > for lockdep to learn what the proper locking order should be. Yet > given Lockdep's current implementation, the only way to guarantee that > there won't be any interactions between subsystems that cause false > positives would be to categorizes locks for each TCP connection into > their own class. I'm not sure I agree with this part. What if we add a new TCP lock class for connections which are used for filesystems/network block devices/...? Yes, it'll be up to each user to set the lockdep classification correctly, but that's a relatively small number of places to add annotations, and I don't see why it wouldn't work.
Re: About the try to remove cross-release feature entirely by Ingo
On Fri, Dec 29, 2017 at 10:16:24PM -0800, Matthew Wilcox wrote:
>
> I think this is a terminology problem. To me (and, I suspect Ted), a
> waiter is a subject of a verb while a lock is an object. So Ted is asking
> whether we have to classify the users, while I think you're saying we
> have extra objects to classify.
Exactly, the classification is applied when the {lock, mutex,
completion} object is initialized. Not currently at the individual
call points to mutex_lock(), wait_for_completion(), down_write(), etc.
> > The problems come from wrong classification. Waiters either classfied
> > well or invalidated properly won't bitrot.
>
> I disagree here. As Ted says, it's the interactions between the
> subsystems that leads to problems. Everything's goig to work great
> until somebody does something in a way that's never been tried before.
The question what is classified *well* mean? At the extreme, we could
put the locks for every single TCP connection into their own lockdep
class. But that would blow the limits in terms of the number of locks
out of the water super-quickly --- and it would destroy the ability
for lockdep to learn what the proper locking order should be. Yet
given Lockdep's current implementation, the only way to guarantee that
there won't be any interactions between subsystems that cause false
positives would be to categorizes locks for each TCP connection into
their own class.
So this is why I get a little annoyed when you say, "it's just a
matter of classification". NO IT IS NOT. We can not possibly
classify things "correctly" to completely limit false positives
without completely destroying lockdep's scalability as it is currently
designed. Byungchul, you don't acknowledge this, and it makes the
"just classify everything" argument completely suspect as a result.
As far as the "just invalidate the waiter", the problem is that it
requires source level changes to invalidate the waiter, and for
different use cases, we will need to validate different waiters. For
example, in the example I gave, we would have to invalidate *all* TCP
waiters/locks in order to prevent false positives. But that makes the
lockdep useless for all TCP locks. What's the solution? I claim that
until lockdep is fundamentally fixed, there is no way to eliminate
*all* false positives without invalidating *all*
cross-release/cross-locks --- in which case you might as well leave
the cross-release patches as an out of tree patch.
So to claim that we can somehow fix the problem by making source-level
changes outside of lockdep, by "properly classifying" or "properly
invalidating" all locks, just doesn't make sense.
The only way it can work is to either dump it on the reposibility of
the people debugging lockdep reports to make source level changes to
other subsystems which they aren't the maintainers of to suppress
false positives that arise due to how the subsystems are being used
together in their particular configuration or you can try to
claim that there is an "acceptable level" of false positives with
which we can live with forever, and which can not be fixed by "proper
classifying" the locks.
Or you can try to make lockdep scalable enough that if we could put
every single lock for every single object into its own lock class
(e.g., each lock for every single TCP connection gets its own lock
class) which is after all the only way we can "properly classify
everything") and still let lockdep be useful.
If you think that is doable, why don't you work on that, and once that
is done, maybe cross-locks lockdep will be considered more acceptable
for mainstream?
- Ted
Re: About the try to remove cross-release feature entirely by Ingo
On Fri, Dec 29, 2017 at 04:28:51PM +0900, Byungchul Park wrote: > On Thu, Dec 28, 2017 at 10:51:46PM -0500, Theodore Ts'o wrote: > > On Fri, Dec 29, 2017 at 10:47:36AM +0900, Byungchul Park wrote: > > > > > >(1) The best way: To classify all waiters correctly. > > > > It's really not all waiters, but all *locks*, no? > > Thanks for your opinion. I will add my opinion on you. > > I meant *waiters*. Locks are only a sub set of potential waiters, which > actually cause deadlocks. Cross-release was designed to consider the > super set including all general waiters such as typical locks, > wait_for_completion(), and lock_page() and so on.. I think this is a terminology problem. To me (and, I suspect Ted), a waiter is a subject of a verb while a lock is an object. So Ted is asking whether we have to classify the users, while I think you're saying we have extra objects to classify. I'd be comfortable continuing to refer to completions as locks. We could try to come up with a new object name like waitpoints though? > > In addition, the lock classification system is not documented at all, > > so now you also need someone who understands the lockdep code. And > > since some of these classifications involve transient objects, and > > lockdep doesn't have a way of dealing with transient locks, and has a > > hard compile time limit of the number of locks that it supports, to > > expect a subsystem maintainer to figure out all of the interactions, > > plus figure out lockdep, and work around lockdep's limitations > > seems not realistic. > > I have to think it more to find out how to solve it simply enough to be > acceptable. The only solution I come up with for now is too complex. I want to amplify Ted's point here. How to use the existing lockdep functionality is undocumented. And that's not your fault. We have Documentation/locking/lockdep-design.txt which I'm sure is great for someone who's willing to invest a week understanding it, but we need a "here's how to use it" guide. > > Given that once Lockdep reports a locking violation, it doesn't report > > any more lockdep violations, if there are a large number of false > > positives, people will not want to turn on cross-release, since it > > will report the false positive and then turn itself off, so it won't > > report anything useful. So if no one turns it on because of the false > > positives, how does the bitrot problem get resolved? > > The problems come from wrong classification. Waiters either classfied > well or invalidated properly won't bitrot. I disagree here. As Ted says, it's the interactions between the subsystems that leads to problems. Everything's goig to work great until somebody does something in a way that's never been tried before.
Re: About the try to remove cross-release feature entirely by Ingo
On 12/29/2017 5:09 PM, Amir Goldstein wrote: On Fri, Dec 29, 2017 at 3:47 AM, Byungchul Park wrote: On Wed, Dec 13, 2017 at 03:24:29PM +0900, Byungchul Park wrote: Lockdep works, based on the following: (1) Classifying locks properly (2) Checking relationship between the classes If (1) is not good or (2) is not good, then we might get false positives. For (1), we don't have to classify locks 100% properly but need as enough as lockdep works. For (2), we should have a mechanism w/o logical defects. Cross-release added an additional capacity to (2) and requires (1) to get more precisely classified. Since the current classification level is too low for cross-release to work, false positives are being reported frequently with enabling cross-release. Yes. It's a obvious problem. It needs to be off by default until the classification is done by the level that cross-release requires. But, the logic (2) is valid and logically true. Please keep the code, mechanism, and logic. I admit the cross-release feature had introduced several false positives about 4 times(?), maybe. And I suggested roughly 3 ways to solve it. I should have explained each in more detail. The lack might have led some to misunderstand. (1) The best way: To classify all waiters correctly. Ultimately the problems should be solved in this way. But it takes a lot of time so it's not easy to use the way right away. And I need helps from experts of other sub-systems. While talking about this way, I made a trouble.. I still believe that each sub-system expert knows how to solve dependency problems most, since each has own dependency rule, but it was not about responsibility. I've never wanted to charge someone else it but me. (2) The 2nd way: To make cross-release off by default. At the beginning, I proposed cross-release being off by default. Honestly, I was happy and did it when Ingo suggested it on by default once lockdep on. But I shouldn't have done that but kept it off by default. Cross-release can make some happy but some unhappy until problems go away through (1) or (2). (3) The 3rd way: To invalidate waiters making trouble. Of course, this is not the best. Now that you have already spent a lot of time to fix original lockdep's problems since lockdep was introduced in 2006, we don't need to use this way for typical locks except a few special cases. Lockdep is fairly robust by now. And I understand you don't want to spend more time to fix additional problems again. Now that the situation is different from the time, 2006, it's not too bad to use this way to handle the issues. Purely logically, aren't you missing a 4th option: (4) The 4th way: To validate specific waiters. Hello, Thanks for your opinion. I will add my opinion on you. Is it not an option for a subsystem to opt-in for cross-release validation of specific locks/waiters? This may be a much preferred route for cross- Yes. I think it can be a good option. I think we have to choose a better one between (3) and (4) depending on the following: In case that there are few waiters making trouble, it would be better to choose (3). In case that there are a lot of waiter making trouble, it would be better to chosse (4). I think (3) is better for now because there's only one or two cases making us hard to handle it. However, if you don't agree, I also think (4) can be an available option. release. I remember seeing a post from a graphic driver developer that found cross-release useful for finding bugs in his code. For example, many waiters in kernel can be waiting for userspace code, so does that mean the cross-release is going to free the world from userspace deadlocks as well?? Possibly I am missing something. I don't see what you are saying exactly.. but cross-release can be used if we know (a) the spot waiting for an event and (3) the other spot triggering the event. Please explain it more if I miss something. In any way, it seem logical to me that some waiters should particpate in lock chain dependencies, while other waiters should break the chain to avoid false positives and to avoid protecting against user configurable deadlocks (like loop mount over file inside the loop mounted fs). For example, when we had cross-release enabled, the following chain was built and false positives were produced: link 1: ext4 spin lock class A (in a lower fs) -> waiter class B (in submit_bio_wait()) link 2: waiter class B (in submit_bio_wait()) -> ext4 spin lock class A (in an upper fs) Even though conceptually it should have been "class A in lower fs != class A in upper fs", current code registers these two as class A. So we need to correct the chain like, using (1): link 1: ext4 spin lock class A (in a lower fs) -> waiter class B (in submit
Re: About the try to remove cross-release feature entirely by Ingo
On Fri, Dec 29, 2017 at 3:47 AM, Byungchul Park wrote: > On Wed, Dec 13, 2017 at 03:24:29PM +0900, Byungchul Park wrote: >> Lockdep works, based on the following: >> >>(1) Classifying locks properly >>(2) Checking relationship between the classes >> >> If (1) is not good or (2) is not good, then we >> might get false positives. >> >> For (1), we don't have to classify locks 100% >> properly but need as enough as lockdep works. >> >> For (2), we should have a mechanism w/o >> logical defects. >> >> Cross-release added an additional capacity to >> (2) and requires (1) to get more precisely classified. >> >> Since the current classification level is too low for >> cross-release to work, false positives are being >> reported frequently with enabling cross-release. >> Yes. It's a obvious problem. It needs to be off by >> default until the classification is done by the level >> that cross-release requires. >> >> But, the logic (2) is valid and logically true. Please >> keep the code, mechanism, and logic. > > I admit the cross-release feature had introduced several false positives > about 4 times(?), maybe. And I suggested roughly 3 ways to solve it. I > should have explained each in more detail. The lack might have led some > to misunderstand. > >(1) The best way: To classify all waiters correctly. > > Ultimately the problems should be solved in this way. But it > takes a lot of time so it's not easy to use the way right away. > And I need helps from experts of other sub-systems. > > While talking about this way, I made a trouble.. I still believe > that each sub-system expert knows how to solve dependency problems > most, since each has own dependency rule, but it was not about > responsibility. I've never wanted to charge someone else it but me. > >(2) The 2nd way: To make cross-release off by default. > > At the beginning, I proposed cross-release being off by default. > Honestly, I was happy and did it when Ingo suggested it on by > default once lockdep on. But I shouldn't have done that but kept > it off by default. Cross-release can make some happy but some > unhappy until problems go away through (1) or (2). > >(3) The 3rd way: To invalidate waiters making trouble. > > Of course, this is not the best. Now that you have already spent > a lot of time to fix original lockdep's problems since lockdep was > introduced in 2006, we don't need to use this way for typical > locks except a few special cases. Lockdep is fairly robust by now. > > And I understand you don't want to spend more time to fix > additional problems again. Now that the situation is different > from the time, 2006, it's not too bad to use this way to handle > the issues. > Purely logically, aren't you missing a 4th option: (4) The 4th way: To validate specific waiters. Is it not an option for a subsystem to opt-in for cross-release validation of specific locks/waiters? This may be a much preferred route for cross- release. I remember seeing a post from a graphic driver developer that found cross-release useful for finding bugs in his code. For example, many waiters in kernel can be waiting for userspace code, so does that mean the cross-release is going to free the world from userspace deadlocks as well?? Possibly I am missing something. In any way, it seem logical to me that some waiters should particpate in lock chain dependencies, while other waiters should break the chain to avoid false positives and to avoid protecting against user configurable deadlocks (like loop mount over file inside the loop mounted fs). And if you agree that this logic claim is correct, than surely, an inclusive approach is the best way forward. Cheers, Amir.
Re: About the try to remove cross-release feature entirely by Ingo
On Thu, Dec 28, 2017 at 10:51:46PM -0500, Theodore Ts'o wrote: > On Fri, Dec 29, 2017 at 10:47:36AM +0900, Byungchul Park wrote: > > > >(1) The best way: To classify all waiters correctly. > > It's really not all waiters, but all *locks*, no? Thanks for your opinion. I will add my opinion on you. I meant *waiters*. Locks are only a sub set of potential waiters, which actually cause deadlocks. Cross-release was designed to consider the super set including all general waiters such as typical locks, wait_for_completion(), and lock_page() and so on.. > > Ultimately the problems should be solved in this way. But it > > takes a lot of time so it's not easy to use the way right away. > > And I need helps from experts of other sub-systems. > > > > While talking about this way, I made a trouble.. I still believe > > that each sub-system expert knows how to solve dependency problems > > most, since each has own dependency rule, but it was not about > > responsibility. I've never wanted to charge someone else it but me. > > The problem is that it's not one subsystem, but *many*. And it's the > interactions between the subsystems. > > Consider the example I gave of a network block device, on which a > local disk file system is mounted, which is then exported over NFS. > So we have the Networking (TCP) stack involved, the NBD device driver, > the local disk file system, the NFS file system, and the networking > stack a second time. That is *many* subsystem maintainers who have to > get involved. I admit that it's not simple one to solve.. > In addition, the lock classification system is not documented at all, > so now you also need someone who understands the lockdep code. And > since some of these classifications involve transient objects, and > lockdep doesn't have a way of dealing with transient locks, and has a > hard compile time limit of the number of locks that it supports, to > expect a subsystem maintainer to figure out all of the interactions, > plus figure out lockdep, and work around lockdep's limitations > seems not realistic. I have to think it more to find out how to solve it simply enough to be acceptable. The only solution I come up with for now is too complex. > (By the way, I've tried reading the crosslock and crossrelease > documentation --- and I'm lost. Sorry, I'm just not smart enough to > understand how it works, at least not from reading the documentation > that was in the patch series. And honestly, I don't care. All I do I am sorry for that. My english is too bad.. I can explain whatever you wonder if you ask me. > need is some practical instructions for how to "classify locks > properly", and how this interacts with lockdep's limitations.) I see what you consider. As you know, it's not something that I can solve right away. That's why I suggested (2) or (3).. > >(2) The 2nd way: To make cross-release off by default. > > > > At the beginning, I proposed cross-release being off by default. > > Honestly, I was happy and did it when Ingo suggested it on by > > default once lockdep on. But I shouldn't have done that but kept > > it off by default. Cross-release can make some happy but some > > unhappy until problems go away through (1) or (2). ^ should be (3) > Ingo's argument is that (1) is not going to be happening any time > soon, and in the meantime, code which is turned off will bitrot. The root cause of the problem is that locks, generally speaking, waiters are roughly classified. IOW, having the new code with a better classification is worth, even it would be done later. > Given that once Lockdep reports a locking violation, it doesn't report > any more lockdep violations, if there are a large number of false > positives, people will not want to turn on cross-release, since it > will report the false positive and then turn itself off, so it won't > report anything useful. So if no one turns it on because of the false > positives, how does the bitrot problem get resolved? The problems come from wrong classification. Waiters either classfied well or invalidated properly won't bitrot. > And if the answer is that some small number of lockdep experts will be > trying to figure out how to do (1) in a tractable way, then Ingo has > argued it can be handled via an out-of-tree patch. > > >(3) The 3rd way: To invalidate waiters making trouble. > > Hmm, can we make cross-release and cross-lock off by default on a per > lock basis? With a well documented to enable it? I'm still not sure Yes. More precisely speaking, we can make cross-release check off on a per waiter basis, for example, by using init_completion_nomap() or its family which I can provide if needed, leaving other traditional lockdep checking *unchanged*. For that issue we talked about, we could use it in submit_bi
Re: About the try to remove cross-release feature entirely by Ingo
On Fri, Dec 29, 2017 at 10:47:36AM +0900, Byungchul Park wrote: > >(1) The best way: To classify all waiters correctly. It's really not all waiters, but all *locks*, no? > Ultimately the problems should be solved in this way. But it > takes a lot of time so it's not easy to use the way right away. > And I need helps from experts of other sub-systems. > > While talking about this way, I made a trouble.. I still believe > that each sub-system expert knows how to solve dependency problems > most, since each has own dependency rule, but it was not about > responsibility. I've never wanted to charge someone else it but me. The problem is that it's not one subsystem, but *many*. And it's the interactions between the subsystems. Consider the example I gave of a network block device, on which a local disk file system is mounted, which is then exported over NFS. So we have the Networking (TCP) stack involved, the NBD device driver, the local disk file system, the NFS file system, and the networking stack a second time. That is *many* subsystem maintainers who have to get involved. In addition, the lock classification system is not documented at all, so now you also need someone who understands the lockdep code. And since some of these classifications involve transient objects, and lockdep doesn't have a way of dealing with transient locks, and has a hard compile time limit of the number of locks that it supports, to expect a subsystem maintainer to figure out all of the interactions, plus figure out lockdep, and work around lockdep's limitations seems not realistic. (By the way, I've tried reading the crosslock and crossrelease documentation --- and I'm lost. Sorry, I'm just not smart enough to understand how it works, at least not from reading the documentation that was in the patch series. And honestly, I don't care. All I do need is some practical instructions for how to "classify locks properly", and how this interacts with lockdep's limitations.) >(2) The 2nd way: To make cross-release off by default. > > At the beginning, I proposed cross-release being off by default. > Honestly, I was happy and did it when Ingo suggested it on by > default once lockdep on. But I shouldn't have done that but kept > it off by default. Cross-release can make some happy but some > unhappy until problems go away through (1) or (2). Ingo's argument is that (1) is not going to be happening any time soon, and in the meantime, code which is turned off will bitrot. Given that once Lockdep reports a locking violation, it doesn't report any more lockdep violations, if there are a large number of false positives, people will not want to turn on cross-release, since it will report the false positive and then turn itself off, so it won't report anything useful. So if no one turns it on because of the false positives, how does the bitrot problem get resolved? And if the answer is that some small number of lockdep experts will be trying to figure out how to do (1) in a tractable way, then Ingo has argued it can be handled via an out-of-tree patch. >(3) The 3rd way: To invalidate waiters making trouble. Hmm, can we make cross-release and cross-lock off by default on a per lock basis? With a well documented to enable it? I'm still not sure how this works given the cross-subsystem problem, though. So if networking enables it because there are no problems with their TCP-only test, and then it blows up when someone is doing NBD or NFS testing, what's the recourse? The file system developer submitting a patch against the networking subsystem to turn off the lockdep tracking on that particular lock because it's causing pain for the file system developer? I can see that potentially causing all sorts of inter-subsystem conflicts. > Talking about what Ingo said in the commit msg.. I want to ask him back, > if he did it with no false positives at the moment merging it in 2006, > without using (2) or (3) method. I bet he know what it means.. And > classifying locks/waiters correctly is not something uglifying code but > a way to document code better. I've felt ill at ease because of the > unnatural and forced explanation. So I think this is the big difference is that potential for cross-subsystem false positives is dramatically higher than with cross-release compared with the traditional lockdep. And I'm not sure there is a clean solution --- how do you "cleanly classify" locks when in some cases each object's locks needs to be considered individual locks, and when that must not be done lest there is an explosion of the number of locks which lockdep needs to track (which is strictly limited due to memory and CPU overhead, as I understand things)? I haven't seen an explanation for how to solve this in a clean, general way --- and I strongly suspect it doesn't exist. Regards, - Ted
Re: About the try to remove cross-release feature entirely by Ingo
On Fri, Dec 29, 2017 at 10:47:36AM +0900, Byungchul Park wrote: > On Wed, Dec 13, 2017 at 03:24:29PM +0900, Byungchul Park wrote: > > Lockdep works, based on the following: > > > >(1) Classifying locks properly > >(2) Checking relationship between the classes > > > > If (1) is not good or (2) is not good, then we > > might get false positives. > > > > For (1), we don't have to classify locks 100% > > properly but need as enough as lockdep works. > > > > For (2), we should have a mechanism w/o > > logical defects. > > > > Cross-release added an additional capacity to > > (2) and requires (1) to get more precisely classified. > > > > Since the current classification level is too low for > > cross-release to work, false positives are being > > reported frequently with enabling cross-release. > > Yes. It's a obvious problem. It needs to be off by > > default until the classification is done by the level > > that cross-release requires. > > > > But, the logic (2) is valid and logically true. Please > > keep the code, mechanism, and logic. > > I admit the cross-release feature had introduced several false positives > about 4 times(?), maybe. And I suggested roughly 3 ways to solve it. I > should have explained each in more detail. The lack might have led some > to misunderstand. > >(1) The best way: To classify all waiters correctly. > > Ultimately the problems should be solved in this way. But it > takes a lot of time so it's not easy to use the way right away. > And I need helps from experts of other sub-systems. > > While talking about this way, I made a trouble.. I still believe > that each sub-system expert knows how to solve dependency problems > most, since each has own dependency rule, but it was not about > responsibility. I've never wanted to charge someone else it but me. > >(2) The 2nd way: To make cross-release off by default. > > At the beginning, I proposed cross-release being off by default. > Honestly, I was happy and did it when Ingo suggested it on by > default once lockdep on. But I shouldn't have done that but kept > it off by default. Cross-release can make some happy but some > unhappy until problems go away through (1) or (2). > >(3) The 3rd way: To invalidate waiters making trouble. > > Of course, this is not the best. Now that you have already spent > a lot of time to fix original lockdep's problems since lockdep was > introduced in 2006, we don't need to use this way for typical > locks except a few special cases. Lockdep is fairly robust by now. > > And I understand you don't want to spend more time to fix > additional problems again. Now that the situation is different > from the time, 2006, it's not too bad to use this way to handle > the issues. > > IMO, the ways can be considered together at a time, which perhaps would > be even better. +cc dan...@ffwll.ch > Talking about what Ingo said in the commit msg.. I want to ask him back, I'm sorry for missing specifying the commit I'm talking about. e966eaeeb locking/lockdep: Remove the cross-release locking checks > if he did it with no false positives at the moment merging it in 2006, > without using (2) or (3) method. I bet he know what it means.. And > classifying locks/waiters correctly is not something uglifying code but > a way to document code better. I've felt ill at ease because of the > unnatural and forced explanation. > > -- > Thanks, > Byungchul
Re: About the try to remove cross-release feature entirely by Ingo
On Wed, Dec 13, 2017 at 03:24:29PM +0900, Byungchul Park wrote: > Lockdep works, based on the following: > >(1) Classifying locks properly >(2) Checking relationship between the classes > > If (1) is not good or (2) is not good, then we > might get false positives. > > For (1), we don't have to classify locks 100% > properly but need as enough as lockdep works. > > For (2), we should have a mechanism w/o > logical defects. > > Cross-release added an additional capacity to > (2) and requires (1) to get more precisely classified. > > Since the current classification level is too low for > cross-release to work, false positives are being > reported frequently with enabling cross-release. > Yes. It's a obvious problem. It needs to be off by > default until the classification is done by the level > that cross-release requires. > > But, the logic (2) is valid and logically true. Please > keep the code, mechanism, and logic. I admit the cross-release feature had introduced several false positives about 4 times(?), maybe. And I suggested roughly 3 ways to solve it. I should have explained each in more detail. The lack might have led some to misunderstand. (1) The best way: To classify all waiters correctly. Ultimately the problems should be solved in this way. But it takes a lot of time so it's not easy to use the way right away. And I need helps from experts of other sub-systems. While talking about this way, I made a trouble.. I still believe that each sub-system expert knows how to solve dependency problems most, since each has own dependency rule, but it was not about responsibility. I've never wanted to charge someone else it but me. (2) The 2nd way: To make cross-release off by default. At the beginning, I proposed cross-release being off by default. Honestly, I was happy and did it when Ingo suggested it on by default once lockdep on. But I shouldn't have done that but kept it off by default. Cross-release can make some happy but some unhappy until problems go away through (1) or (2). (3) The 3rd way: To invalidate waiters making trouble. Of course, this is not the best. Now that you have already spent a lot of time to fix original lockdep's problems since lockdep was introduced in 2006, we don't need to use this way for typical locks except a few special cases. Lockdep is fairly robust by now. And I understand you don't want to spend more time to fix additional problems again. Now that the situation is different from the time, 2006, it's not too bad to use this way to handle the issues. IMO, the ways can be considered together at a time, which perhaps would be even better. Talking about what Ingo said in the commit msg.. I want to ask him back, if he did it with no false positives at the moment merging it in 2006, without using (2) or (3) method. I bet he know what it means.. And classifying locks/waiters correctly is not something uglifying code but a way to document code better. I've felt ill at ease because of the unnatural and forced explanation. -- Thanks, Byungchul
Re: About the try to remove cross-release feature entirely by Ingo
On Thu, Dec 14, 2017 at 8:18 PM, Peter Zijlstra wrote: > On Wed, Dec 13, 2017 at 10:07:11PM -0500, Theodore Ts'o wrote: >> interpreted this as the lockdep maintainers saying, "hey, not my >> fault, it's the subsystem maintainer's fault for not properly >> classifying the locks" --- and thus dumping the responsibility in the >> subsystem maintainers' laps. > > Let me clarify that I (as lockdep maintainer) disagree with that > sentiment. I have spend a lot of time over the years staring at random > code trying to fix lockdep splats. Its awesome if corresponding > subsystem maintainers help out and many have, but I very much do not > agree its their problem and their problem alone. I apologize to all of you. That's really not what I intended to say. I said that other folks can annotate it for the sub-system better than lockdep developer, so suggested to invalidate locks making trouble and wanting to avoid annotating it at the moment, and validate those back when necessary with additional annotations. It's my fault. I'm not sure how I should express what I want to say, but, I didn't intend to charge the responsibility to other folks. Ideally, I think it's best to solve it with co-work. I should've been more careful to say that. Again, I apologize for that, to lockdep and fs maintainers. Of course, for cross-release, I have the will to annotate it or find a better way to avoid false positives. And I think I have to. -- Thanks, Byungchul
Re: About the try to remove cross-release feature entirely by Ingo
On Wed, Dec 13, 2017 at 10:07:11PM -0500, Theodore Ts'o wrote: > interpreted this as the lockdep maintainers saying, "hey, not my > fault, it's the subsystem maintainer's fault for not properly > classifying the locks" --- and thus dumping the responsibility in the > subsystem maintainers' laps. Let me clarify that I (as lockdep maintainer) disagree with that sentiment. I have spend a lot of time over the years staring at random code trying to fix lockdep splats. Its awesome if corresponding subsystem maintainers help out and many have, but I very much do not agree its their problem and their problem alone. This attitude is one of the biggest issues I have with the crossrelease stuff and why I don't disagree with Ingo taking it out (for now).
Re: About the try to remove cross-release feature entirely by Ingo
On Thu, Dec 14, 2017 at 12:07 PM, Theodore Ts'o wrote: > On Wed, Dec 13, 2017 at 04:13:07PM +0900, Byungchul Park wrote: >> >> Therefore, I want to say the fundamental problem >> comes from classification, not cross-release >> specific. > > You keep saying that it is "just" a matter of classificaion. But, it's a fact. > However, it is not obvious how to do the classification in a sane > manner. And this is why I keep pointing out that there is no > documentation on how to do this, and somehow you never respond to this > point I can write a document explaining what lock class is but.. I cannot explain how to assign it perfectly since there's no right answer. It's something we need to improve more and more. > In the case where you have multiple unrelated subsystems that can be > stacked in different ways, with potentially multiple instances stacked > on top of each other, it is not at all clear to me how this problem > should be solved. I cannot give you a perfect solution immediately. I know, and as you know, it's a very difficult problem to solve. > It was said on one of these threads (perhaps by you, perhaps by > someone else), that we can't expect the lockdep maintainers to > understand all of the subsystems in the kernels, and so therefore it > must be up to the subsystem maintainers to classify the locks. I > interpreted this as the lockdep maintainers saying, "hey, not my > fault, it's the subsystem maintainer's fault for not properly > classifying the locks" --- and thus dumping the responsibility in the > subsystem maintainers' laps. Sorry to say, making you feel like that. Precisely speaking, the responsibility for something caused by cross-release is on me, and the responsibility for something caused by lockdep itselt is on lockdep. I meant, in the current way to assign lock class automatically, it's inevitable for someone to annotate places manually, and it can be done best by each expert. But, anyway fundamentally I think the responsibility is on lockdep. > I don't know if the situation is just that lockdep is insufficiently > documented, and with the proper tutorial, it would be obvious how to > solve the classification problem. > > Or, if perhaps, there *is* no way to solve the classification problem, > at least not in a general form. Agree. It's a very difficult one to solve. > For example --- suppose we have a network block device on which there > is an btrfs file system, which is then exported via NFS. Now all of > the TCP locks will be used twice for two different instances, once for > the TCP connection for the network block device, and then for the NFS > export. > > How exactly are we supposed to classify the locks to make it all work? > > Or the loop device built on top of an ext4 file system which on a > LVM/device mapper device. And suppose the loop device is then layered > with a dm-error device for regression testing, and with another ext4 > file system on top of that? Ditto. > How exactly are we supposed to classify the locks in that situation? > Where's the documentation and tutorials which explain how to make this > work, if the responsibility is going to be dumped on the subsystem > maintainers' laps? Or if the lockdep maintainers are expected to fix > and classify all of these locks, are you volunteering to do this? I have the will. I will. > How hard is it exactly to do all of this classification work, no > matter whose responsibility it will ultimately be? > > And if the answer is that it is too hard, then let me gently suggest > to you that perhaps, if this is a case, that maybe this is a > fundamental and fatal flaw with the cross-release and completion > lockdep feature? I don't understand this. > Best regards, > > - Ted -- Thanks, Byungchul
Re: About the try to remove cross-release feature entirely by Ingo
On Wed, Dec 13, 2017 at 04:13:07PM +0900, Byungchul Park wrote: > > Therefore, I want to say the fundamental problem > comes from classification, not cross-release > specific. You keep saying that it is "just" a matter of classificaion. However, it is not obvious how to do the classification in a sane manner. And this is why I keep pointing out that there is no documentation on how to do this, and somehow you never respond to this point In the case where you have multiple unrelated subsystems that can be stacked in different ways, with potentially multiple instances stacked on top of each other, it is not at all clear to me how this problem should be solved. It was said on one of these threads (perhaps by you, perhaps by someone else), that we can't expect the lockdep maintainers to understand all of the subsystems in the kernels, and so therefore it must be up to the subsystem maintainers to classify the locks. I interpreted this as the lockdep maintainers saying, "hey, not my fault, it's the subsystem maintainer's fault for not properly classifying the locks" --- and thus dumping the responsibility in the subsystem maintainers' laps. I don't know if the situation is just that lockdep is insufficiently documented, and with the proper tutorial, it would be obvious how to solve the classification problem. Or, if perhaps, there *is* no way to solve the classification problem, at least not in a general form. For example --- suppose we have a network block device on which there is an btrfs file system, which is then exported via NFS. Now all of the TCP locks will be used twice for two different instances, once for the TCP connection for the network block device, and then for the NFS export. How exactly are we supposed to classify the locks to make it all work? Or the loop device built on top of an ext4 file system which on a LVM/device mapper device. And suppose the loop device is then layered with a dm-error device for regression testing, and with another ext4 file system on top of that? How exactly are we supposed to classify the locks in that situation? Where's the documentation and tutorials which explain how to make this work, if the responsibility is going to be dumped on the subsystem maintainers' laps? Or if the lockdep maintainers are expected to fix and classify all of these locks, are you volunteering to do this? How hard is it exactly to do all of this classification work, no matter whose responsibility it will ultimately be? And if the answer is that it is too hard, then let me gently suggest to you that perhaps, if this is a case, that maybe this is a fundamental and fatal flaw with the cross-release and completion lockdep feature? Best regards, - Ted
Re: About the try to remove cross-release feature entirely by Ingo
On Wed, 2017-12-13 at 16:13 +0900, Byungchul Park wrote: > In addition, I want to say that the current level of > classification is much less than 100% but, since we > have annotated well to suppress wrong reports by > rough classifications, finally it does not come into > view by original lockdep for now. The Linux kernel is not a vehicle for experiments. The majority of false positives should have been fixed before the crossrelease patches were sent to Linus. Bart.
Re: About the try to remove cross-release feature entirely by Ingo
On Wed, Dec 13, 2017 at 3:24 PM, Byungchul Park wrote: > Lockdep works, based on the following: > >(1) Classifying locks properly >(2) Checking relationship between the classes > > If (1) is not good or (2) is not good, then we > might get false positives. > > For (1), we don't have to classify locks 100% > properly but need as enough as lockdep works. > > For (2), we should have a mechanism w/o > logical defects. > > Cross-release added an additional capacity to > (2) and requires (1) to get more precisely classified. > > Since the current classification level is too low for > cross-release to work, false positives are being > reported frequently with enabling cross-release. > Yes. It's a obvious problem. It needs to be off by > default until the classification is done by the level > that cross-release requires. > > But, the logic (2) is valid and logically true. Please > keep the code, mechanism, and logic. In addition, I want to say that the current level of classification is much less than 100% but, since we have annotated well to suppress wrong reports by rough classifications, finally it does not come into view by original lockdep for now. But since cross-release makes the dependency graph more fine-grained, it easily comes into view. Even w/o cross-release, it can happen by adding additional dependencies connecting two roughly classified lock classes in the future. Furthermore, I can see many places in kernel to consider wait_for_completion() using manual lock_acquire()/lock_release() and the rate using it raises. In other words, even without cross-release, the more we add manual annotations for wait_for_completion() the more we definitely suffer same problems someday, we are facing now through cross-release. Therefore, I want to say the fundamental problem comes from classification, not cross-release specific. Of course, since cross-release accelerates the condition, I agree with it to be off for now. -- Thanks, Byungchul
About the try to remove cross-release feature entirely by Ingo
Lockdep works, based on the following: (1) Classifying locks properly (2) Checking relationship between the classes If (1) is not good or (2) is not good, then we might get false positives. For (1), we don't have to classify locks 100% properly but need as enough as lockdep works. For (2), we should have a mechanism w/o logical defects. Cross-release added an additional capacity to (2) and requires (1) to get more precisely classified. Since the current classification level is too low for cross-release to work, false positives are being reported frequently with enabling cross-release. Yes. It's a obvious problem. It needs to be off by default until the classification is done by the level that cross-release requires. But, the logic (2) is valid and logically true. Please keep the code, mechanism, and logic. -- Thanks, Byungchul
