Re: [GIT PULL] Please pull NFS client changes for 4.18
On Wed, Jun 13, 2018 at 11:42:51AM +1000, NeilBrown wrote: > On Tue, Jun 12 2018, Paul E. McKenney wrote: > > > On Tue, Jun 12, 2018 at 06:11:50PM -0700, Linus Torvalds wrote: > >> On Tue, Jun 12, 2018 at 6:02 PM Paul E. McKenney > >> wrote: > >> > > >> > We did review this one, and Neil did make requested changes to the > >> > comment > >> > header and documentation. > >> > >> Oh, I checked the commit for "acked-by" from you and didn't see any, > >> so I was assuming this was just Neil and Trond on their own.. > > > > Hmmm... I gave them a Reviewed-by, but perhaps it got lost. > > > > http://lkml.kernel.org/r/20180501141404.gd26...@linux.vnet.ibm.com > > Sorry about that. I think I planned to add it after I heard back from > Trond what he thought of the patch, but I didn't hear anything until it > landed. I'll try to be more proactive next time. Not a problem from my viewpoint. There may come a time when I need to care about contribution metrics, but I don't need to at the moment. ;-) Thanx, Paul
Re: [GIT PULL] Please pull NFS client changes for 4.18
On Wed, Jun 13, 2018 at 11:42:51AM +1000, NeilBrown wrote: > On Tue, Jun 12 2018, Paul E. McKenney wrote: > > > On Tue, Jun 12, 2018 at 06:11:50PM -0700, Linus Torvalds wrote: > >> On Tue, Jun 12, 2018 at 6:02 PM Paul E. McKenney > >> wrote: > >> > > >> > We did review this one, and Neil did make requested changes to the > >> > comment > >> > header and documentation. > >> > >> Oh, I checked the commit for "acked-by" from you and didn't see any, > >> so I was assuming this was just Neil and Trond on their own.. > > > > Hmmm... I gave them a Reviewed-by, but perhaps it got lost. > > > > http://lkml.kernel.org/r/20180501141404.gd26...@linux.vnet.ibm.com > > Sorry about that. I think I planned to add it after I heard back from > Trond what he thought of the patch, but I didn't hear anything until it > landed. I'll try to be more proactive next time. Not a problem from my viewpoint. There may come a time when I need to care about contribution metrics, but I don't need to at the moment. ;-) Thanx, Paul
Re: [GIT PULL] Please pull NFS client changes for 4.18
On Tue, Jun 12 2018, Paul E. McKenney wrote: > On Tue, Jun 12, 2018 at 06:11:50PM -0700, Linus Torvalds wrote: >> On Tue, Jun 12, 2018 at 6:02 PM Paul E. McKenney >> wrote: >> > >> > We did review this one, and Neil did make requested changes to the comment >> > header and documentation. >> >> Oh, I checked the commit for "acked-by" from you and didn't see any, >> so I was assuming this was just Neil and Trond on their own.. > > Hmmm... I gave them a Reviewed-by, but perhaps it got lost. > > http://lkml.kernel.org/r/20180501141404.gd26...@linux.vnet.ibm.com > > Thanx, Paul Sorry about that. I think I planned to add it after I heard back from Trond what he thought of the patch, but I didn't hear anything until it landed. I'll try to be more proactive next time. Thanks, NeilBrown signature.asc Description: PGP signature
Re: [GIT PULL] Please pull NFS client changes for 4.18
On Tue, Jun 12 2018, Paul E. McKenney wrote: > On Tue, Jun 12, 2018 at 06:11:50PM -0700, Linus Torvalds wrote: >> On Tue, Jun 12, 2018 at 6:02 PM Paul E. McKenney >> wrote: >> > >> > We did review this one, and Neil did make requested changes to the comment >> > header and documentation. >> >> Oh, I checked the commit for "acked-by" from you and didn't see any, >> so I was assuming this was just Neil and Trond on their own.. > > Hmmm... I gave them a Reviewed-by, but perhaps it got lost. > > http://lkml.kernel.org/r/20180501141404.gd26...@linux.vnet.ibm.com > > Thanx, Paul Sorry about that. I think I planned to add it after I heard back from Trond what he thought of the patch, but I didn't hear anything until it landed. I'll try to be more proactive next time. Thanks, NeilBrown signature.asc Description: PGP signature
Re: [GIT PULL] Please pull NFS client changes for 4.18
On Tue, Jun 12, 2018 at 06:11:50PM -0700, Linus Torvalds wrote: > On Tue, Jun 12, 2018 at 6:02 PM Paul E. McKenney > wrote: > > > > We did review this one, and Neil did make requested changes to the comment > > header and documentation. > > Oh, I checked the commit for "acked-by" from you and didn't see any, > so I was assuming this was just Neil and Trond on their own.. Hmmm... I gave them a Reviewed-by, but perhaps it got lost. http://lkml.kernel.org/r/20180501141404.gd26...@linux.vnet.ibm.com Thanx, Paul
Re: [GIT PULL] Please pull NFS client changes for 4.18
On Tue, Jun 12, 2018 at 06:11:50PM -0700, Linus Torvalds wrote: > On Tue, Jun 12, 2018 at 6:02 PM Paul E. McKenney > wrote: > > > > We did review this one, and Neil did make requested changes to the comment > > header and documentation. > > Oh, I checked the commit for "acked-by" from you and didn't see any, > so I was assuming this was just Neil and Trond on their own.. Hmmm... I gave them a Reviewed-by, but perhaps it got lost. http://lkml.kernel.org/r/20180501141404.gd26...@linux.vnet.ibm.com Thanx, Paul
Re: [GIT PULL] Please pull NFS client changes for 4.18
On Tue, Jun 12, 2018 at 6:02 PM Paul E. McKenney wrote: > > We did review this one, and Neil did make requested changes to the comment > header and documentation. Oh, I checked the commit for "acked-by" from you and didn't see any, so I was assuming this was just Neil and Trond on their own.. Linus
Re: [GIT PULL] Please pull NFS client changes for 4.18
On Tue, Jun 12, 2018 at 6:02 PM Paul E. McKenney wrote: > > We did review this one, and Neil did make requested changes to the comment > header and documentation. Oh, I checked the commit for "acked-by" from you and didn't see any, so I was assuming this was just Neil and Trond on their own.. Linus
Re: [GIT PULL] Please pull NFS client changes for 4.18
On Tue, Jun 12, 2018 at 11:08:31AM -0700, Linus Torvalds wrote: > Final note (for now) on this: I've merged the nfs code, but I really > am obviously not happy with these crazy random ad-hoc > cursor-not-cursor list games. We did review this one, and Neil did make requested changes to the comment header and documentation. However, as you say, I will push back harder on future RCU list traversal macros. Thanx, Paul
Re: [GIT PULL] Please pull NFS client changes for 4.18
On Tue, Jun 12, 2018 at 11:08:31AM -0700, Linus Torvalds wrote: > Final note (for now) on this: I've merged the nfs code, but I really > am obviously not happy with these crazy random ad-hoc > cursor-not-cursor list games. We did review this one, and Neil did make requested changes to the comment header and documentation. However, as you say, I will push back harder on future RCU list traversal macros. Thanx, Paul
Re: [GIT PULL] Please pull NFS client changes for 4.18
On Tue, Jun 12 2018, Linus Torvalds wrote: > Final note (for now) on this: I've merged the nfs code, but I really > am obviously not happy with these crazy random ad-hoc > cursor-not-cursor list games. > > Linus Hi Linus, thanks for merging the code despite your reservations. Yes, we could create a generic rcu-list cursor. I have given it some thought but didn't like the added complexity. As there were existing objects in the list that could be used as a cursor, that seemed to me to be the better solution. As you say, and cursor would need to be allocated from a slab, not on the stack. We could use a SLAB_TYPESAFE_BY_RCU and not need to use rcu to delay the freeing. The lsb in the next pointer of the cursor would be 1 to indicate the cursor. Any iteration of the list would need to look out for this flag. When found it would need to skip over any cursors to the next non-cursor, then repeat the skip and make sure it found the same non-cursor. This guards against the cursor moving while it is being examined. Any walk that needed to place a cursor would need to get an exclusive lock on the list from the start. This is more locking overhead than just grabbing the lock to optimistically take a reference on the "current" item which I did in the NFS patch. If the lists were normally short that might not be a problem. In this case the list can get quite long so the extra locking might be noticeable. Deleting objects from the list would need to be careful to preserve the flag bit, but that is the least difficult part. FYI I have an open proposal to improve the cursor used by rhashtable for rhashtable_walk - it sometimes needs to drop out of RCU in the middle of a bucket chain. In that case the chain is normally short (16 is considered so long that the hash must have been compromised) and I propose an insertion sort to keep the addresses of objects in numerical order. This way the address of the last object found can work as a stable cursor - we just search through the list until an object has a larger address. So my perspective is that while an rcu_cursor_list could be developed, I'm not sure it would always (or ever?) be the best solution to a given problem. I can turn these thoughts into a patch if you like and see what people think. Thanks, NeilBrown signature.asc Description: PGP signature
Re: [GIT PULL] Please pull NFS client changes for 4.18
On Tue, Jun 12 2018, Linus Torvalds wrote: > Final note (for now) on this: I've merged the nfs code, but I really > am obviously not happy with these crazy random ad-hoc > cursor-not-cursor list games. > > Linus Hi Linus, thanks for merging the code despite your reservations. Yes, we could create a generic rcu-list cursor. I have given it some thought but didn't like the added complexity. As there were existing objects in the list that could be used as a cursor, that seemed to me to be the better solution. As you say, and cursor would need to be allocated from a slab, not on the stack. We could use a SLAB_TYPESAFE_BY_RCU and not need to use rcu to delay the freeing. The lsb in the next pointer of the cursor would be 1 to indicate the cursor. Any iteration of the list would need to look out for this flag. When found it would need to skip over any cursors to the next non-cursor, then repeat the skip and make sure it found the same non-cursor. This guards against the cursor moving while it is being examined. Any walk that needed to place a cursor would need to get an exclusive lock on the list from the start. This is more locking overhead than just grabbing the lock to optimistically take a reference on the "current" item which I did in the NFS patch. If the lists were normally short that might not be a problem. In this case the list can get quite long so the extra locking might be noticeable. Deleting objects from the list would need to be careful to preserve the flag bit, but that is the least difficult part. FYI I have an open proposal to improve the cursor used by rhashtable for rhashtable_walk - it sometimes needs to drop out of RCU in the middle of a bucket chain. In that case the chain is normally short (16 is considered so long that the hash must have been compromised) and I propose an insertion sort to keep the addresses of objects in numerical order. This way the address of the last object found can work as a stable cursor - we just search through the list until an object has a larger address. So my perspective is that while an rcu_cursor_list could be developed, I'm not sure it would always (or ever?) be the best solution to a given problem. I can turn these thoughts into a patch if you like and see what people think. Thanks, NeilBrown signature.asc Description: PGP signature
Re: [GIT PULL] Please pull NFS client changes for 4.18
Final note (for now) on this: I've merged the nfs code, but I really am obviously not happy with these crazy random ad-hoc cursor-not-cursor list games. Linus
Re: [GIT PULL] Please pull NFS client changes for 4.18
Final note (for now) on this: I've merged the nfs code, but I really am obviously not happy with these crazy random ad-hoc cursor-not-cursor list games. Linus
Re: [GIT PULL] Please pull NFS client changes for 4.18
On Tue, Jun 12, 2018 at 10:42 AM Linus Torvalds wrote: > > So could we please have a cursor entry for RCU walking, and actually > have a agreed-upon way to do this? Maybe even using the low bit in the > "next" field to mark a cursor entry generically - the same way we > already do for the HEAD field in the bit-locked lists, just on the > actual entries _on_ the list instead. The real problem with a RCU cursor is that the lifetime of the cursor is also RCU extended, so you can't do the traditional "just allocate the cursor entry on the stack" that you can with synchronous locked lists. That makes it slightly more inconvenient to do simple cursors. The dcache code has a fairly clean "dcache cursor", but it does use a whole "struct dentry" for it (and it depends on "next_positive()" to just skip them because dursors are never _positive_ dentries, so there's some subtlety there). But at least it's a very explicit cursor model, not that odd delegation inode that seems to have a life as a real inode too. So maybe a generic rcu-list cursor is too hard to do sanely, but can we at least encourage that very explicit cursor model that is *only* a cursor, and might not be touched/moved/reallocated by something else? Linus
Re: [GIT PULL] Please pull NFS client changes for 4.18
On Tue, Jun 12, 2018 at 10:42 AM Linus Torvalds wrote: > > So could we please have a cursor entry for RCU walking, and actually > have a agreed-upon way to do this? Maybe even using the low bit in the > "next" field to mark a cursor entry generically - the same way we > already do for the HEAD field in the bit-locked lists, just on the > actual entries _on_ the list instead. The real problem with a RCU cursor is that the lifetime of the cursor is also RCU extended, so you can't do the traditional "just allocate the cursor entry on the stack" that you can with synchronous locked lists. That makes it slightly more inconvenient to do simple cursors. The dcache code has a fairly clean "dcache cursor", but it does use a whole "struct dentry" for it (and it depends on "next_positive()" to just skip them because dursors are never _positive_ dentries, so there's some subtlety there). But at least it's a very explicit cursor model, not that odd delegation inode that seems to have a life as a real inode too. So maybe a generic rcu-list cursor is too hard to do sanely, but can we at least encourage that very explicit cursor model that is *only* a cursor, and might not be touched/moved/reallocated by something else? Linus
Re: [GIT PULL] Please pull NFS client changes for 4.18
On Tue, Jun 12, 2018 at 6:39 AM Trond Myklebust wrote:
>
> NeilBrown (4):
> rculist: add list_for_each_entry_from_rcu()
Oh christ, people. Not another one of these.
We need to start having a real rule in place where people DO NOT PLAY
GAMES WITH RCU LISTS.
Adding Paul McKenney to the list, since he clearly wasn't for the
actual development, and the commit has no sign that it was sanity
checked.
This whole "let's re-start RCU walking in the middle after we've
dropped the RCU read lock" needs a hell of a lot more thought than
people seem to actually be giving it.
Maybe the code is actually correct - it looks ok to me. But every time
I see it I just shudder.
What people actually want is to just have a cursor entry in an RCU
list. I'd much rather have *that*, than have people make up these
insane ad-hoc cursor entries that are insane and have horrible
semantics.
For example, looking at that commit e04bbf6b1bbe ("NFS: Avoid
quadratic search when freeing delegations"), it even talks about the
ad-hoc cursor entry not actually being reliable, and how that's ok
because the code doesn't care. No, random odd behavior that is
basically never ever going to be testable is *NOT* ok.
So could we please have a cursor entry for RCU walking, and actually
have a agreed-upon way to do this? Maybe even using the low bit in the
"next" field to mark a cursor entry generically - the same way we
already do for the HEAD field in the bit-locked lists, just on the
actual entries _on_ the list instead.
Because we now have *two* of these odd special "restart the loop in
the middle" come in during the same merge window. That really implies
to me that we should have a _proper_ solution for the problem, not
this horribly nasty case where people make up their own pseudo-cursors
that have odd and questionable semantics.
Paul, comments?
Linus
Re: [GIT PULL] Please pull NFS client changes for 4.18
On Tue, Jun 12, 2018 at 6:39 AM Trond Myklebust wrote:
>
> NeilBrown (4):
> rculist: add list_for_each_entry_from_rcu()
Oh christ, people. Not another one of these.
We need to start having a real rule in place where people DO NOT PLAY
GAMES WITH RCU LISTS.
Adding Paul McKenney to the list, since he clearly wasn't for the
actual development, and the commit has no sign that it was sanity
checked.
This whole "let's re-start RCU walking in the middle after we've
dropped the RCU read lock" needs a hell of a lot more thought than
people seem to actually be giving it.
Maybe the code is actually correct - it looks ok to me. But every time
I see it I just shudder.
What people actually want is to just have a cursor entry in an RCU
list. I'd much rather have *that*, than have people make up these
insane ad-hoc cursor entries that are insane and have horrible
semantics.
For example, looking at that commit e04bbf6b1bbe ("NFS: Avoid
quadratic search when freeing delegations"), it even talks about the
ad-hoc cursor entry not actually being reliable, and how that's ok
because the code doesn't care. No, random odd behavior that is
basically never ever going to be testable is *NOT* ok.
So could we please have a cursor entry for RCU walking, and actually
have a agreed-upon way to do this? Maybe even using the low bit in the
"next" field to mark a cursor entry generically - the same way we
already do for the HEAD field in the bit-locked lists, just on the
actual entries _on_ the list instead.
Because we now have *two* of these odd special "restart the loop in
the middle" come in during the same merge window. That really implies
to me that we should have a _proper_ solution for the problem, not
this horribly nasty case where people make up their own pseudo-cursors
that have odd and questionable semantics.
Paul, comments?
Linus
