Re: Changes to reduce pressure on uvm_pageqlock

[Andrew Doran <ad%netbsd.org@localhost>]

Complete & compilable diff as promised.  Having taken feedback into
consideration and considering that with the last revision uvm_pageqlock
wasn't doing a whole lot any more, this takes it further than before, about
as far as I want to go right now.  Summary:

- uvm_pageqlock is gone.
- instead we have vm_page::interlock protecting identity, and...
- the pdpolicy has its own lock.  could do multiple queues/locks later.
- the ground is paved for lazy dequeueing of pages.
- vm_page has grown (yamt's radixtree can eliminate rbtree node later).
- i think it fixes this recent PR: http://gnats.netbsd.org/54727
- vm_page::pqflags becomes totally private to pdpolicy.  it now needs
  to be 32-bits to stand on its own for architectures that can't do
  atomic sub-word writes, because it's locked by a different lock to
  the adjacent fields.  i am using the spare bits for lazy activation.
- i have bumped loan & wire counts to 32-bits.  i don't think 16 is safe.
- clockpro was already broken but I have tried not to make it worse.

http://www.netbsd.org/~ad/2019/uvm_pageqlock.diff

Cheers,
Andrew

On Sat, Dec 07, 2019 at 10:40:39PM +0000, Andrew Doran wrote:
> On Sat, Dec 07, 2019 at 01:32:35PM -0800, Chuck Silvers wrote:
> 
> > I like this new version much more than the previous version.
> > The new diff looks incomplete though, it uses the "pdpol" field in
> > vm_page that was new in the previous diff.  It looks like there
> > should be changes in uvm_pdaemon.c to go with the new diff too?
> > Could you send the complete new diff again please?
> 
> Yes it's incomplete.  I'll split these pieces out of my tree, merge into
> -current tomorrow and get a clean diff.
> 
> I think the locking change is enough on its own, so I will exclude the
> "pdpol" piece to avoid clouding things, and go again with another review for
> that and other parts in the near future.
> 
> > > With that done, we'd then only need one version of uvm_pageactivate() etc,
> > > because those functions will no longer care about uvm_pageqlock.
> > 
> > The pageq lock is named that because it currently protects the pageq's.  :-)
> > If it's not going to protect the pageq's anymore then it ought to be renamed.
> > Probably better to just work on eliminating it though.
> 
> Agreed.
>  
> > In your new diff, the pageq lock is still used by the pagedaemon
> > (for uvmpd_trylockowner() as you noted in the comment).
> > It would be good not to have a global lock involved in changing page identity,
> > and with the pageq's now protected by a separate lock, we should be able to
> > eliminate the need for the pageq lock when freeing a page with an RCUy thing
> > to prevent freeing uvm_object and uvm_anon structures while the pagedaemon might
> > still be accessing them via the vm_page fields.  We should be able to improve
> > the locking for pg->loan_count with a similar RCUy thing... my recollection is
> > that the only reason loan_count uses the pageq lock currently is to stabilize
> > the page identity while locking the owning object.  It might make sense to have
> 
> On stablisation yes I agree.  My reading has always been that it's safe to
> test those properties with either the pageqlock or the object locked - but
> on what happens next, your intent matters.
> 
> > pg->wire_count protected by the pdpol lock too (though I have some thoughts
> > about changing the nature of wire_count that would probaby make that not
> > make sense, I haven't had time to think that through yet).
> > 
> > But anyway, I'll stop rambling at this point so we can focus on the diff at hand.
> > :-)
> 
> That's an interesting point re stabilisation and a RCUy type thing I see
> exactly what you're getting at and it's related to the few points of
> connection between datasets I mentioned.  I'll have a think about that.
> 
> Thanks for taking the time to look this over Chuck.
> 
> Cheers,
> Andrew
> 
> > 
> > -Chuck
> > 
> > 
> > > I tried it out: the code looks more elegant and the scheme works in practice
> > > with contention on uvm_pageqlock now quite minor, and the sum of contention
> > > on the two locks lower than it was on uvm_pageqlock prior.
> > > 
> > > With the locking for the pdpolicy code being private to the file I think it
> > > would then be easier to experiment with different strategies to reduce
> > > contention further.
> > > 
> > > http://www.netbsd.org/~ad/2019/pdpolicy.diff
> > > 
> > > Andrew