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
