If PV lock should be by superpage, the shift should be 24, because super pages are 16MB. I wonder if that might reduce contention a bit. Since the lock would then cover the full 16MB instead of needing 8 separate locks, you may be able to simply lock once to iterate over the full super page.

On amd64 the lock must be per super-page. Demotions and promotions depend on this, they need to manipulate pv entries for all pages that constitute the superpage. This is not an optimization to reduce contention, but the fundamental feature.

I do not know if this holds for powerpc pmaps, but I highly suspect that it does.

You're very likely correct on this. When @luporl wrote the superpage code he probably missed this, and so did I in the review. That may explain some of the complexity in the superpage code.

errr...

hw.pagesizes: { 4096, 2097152 }

that's on a power9 vm, and my power8 powernv shows the same thing.

So we're doing 2mb superpages, so that shift looks right!

It's a lie. 2MB superpages are only on radix pmap. This needs to be fixed for HPT, which only supports 16MB superpages.

I don't see how you could see 2097152 for HPT, because moea64_early_bootstrap() sets vm_level_0_order to 12 immediately, before it sets the pagesizes array. Something is screwy.

ah no, it /is/ 16mb superpages on the hash table.

i'll ask markj about this in a bit more detail.

A read lock should suffice for this.

Yeah, good idea to assert on this. But assert on RA_LOCKED, not RA_RLOCKED, because you do have some spots that are locking for write and others that lock for read.

hm should i make this a different diff?

Yeah, make it separate. I like the rest of this diff, but this I one I think would just add noise. We don't need to know which pages are contended on, because they'll keep changing as the workload changes.

Hm, this should be one of the lock switch/iterate calls (CHANGE_PV_LIST_WR_LOCK_TO_PHYS()) but when i do this it'll hit a lock recursion assert.

With this call here i hit witness, but not a panic.

I'll need to figure that out.

figured it out, moea64_pvo_cleanup() was also grabbing/iterating locks. I've fixed that and now it doesn't panic or give recursion witness warnings.

This can just be rw_init().

How is it guaranteed that all of these pages are protected by the same PV list lock? All of the physical pages here belong to the same large page, right?

Justin's comment is marked done but there is no assertion.

I believe that's what @jhibbits is asserting with his previous change, yes.

Which vm_page_t to to use though? At the point that moea64_sp_query() has been called something's already converted a vm_page_t to /a/ page; moea64_sp_query_locked() looks for the first pvo for the superpage, then does

m = PHYS_TO_VM_PAGE(PVO_PADDR(sp));

where sp is again the first pvo in the superpage.

So, should I just put an assert on PHYS_TO_VM_PAGE(PVO_ADDR(pvo)) in entry to moea64_sp_query() ? or moea64_sp_query_locked() ? or assert that the vm_page it determined to be the "first" in the list?

(I also note that although I added a comment, there wasn't already a lock assertion there, and I kinda wanna argue that adding a lock assertion should be an separate, earlier diff in the stack so we can validate IT is currently asserting correctly.)