Re: Changes to reduce pressure on uvm_pageqlock

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

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