On Thu, Oct 03, 2019 at 05:36:31PM -0300, Leonardo Bras wrote: > > Also, I'm not sure I understand things properly. > > > > So serialize_against_pte_lookup() wants to wait for all currently > > out-standing __find_linux_pte() instances (which are very similar to > > gup_fast). > > > > It seems to want to do this before flushing the THP TLB for some reason; > > why? Should not THP observe the normal page table freeing rules which > > includes a RCU-like grace period like this already. > > > > Why is THP special here? This doesn't seem adequately explained. > > "It's necessary to monitor lockless pagetable walks, in order to avoid > doing THP splitting/collapsing during them." > > If a there is a THP split/collapse during the lockless pagetable walk, > the returned ptep can be a pointing to an invalid pte. So the whole premise of lockless page-table walks (gup_fast) is that it can work on in-flux page-tables. Specifically gup_fast() never returns PTEs, only struct page *, and only if it can increment the page refcount. In order to enable this, page-table pages are RCU(-like) freed, such that even if we access page-tables that have (concurrently) been unlinked, we'll not UaF (see asm-generic/tlb.h, the comment at HAVE_RCU_TABLE_FREE). IOW, the worst case if not getting a struct page *. I really don't see how THP splitting/collapsing is special here, either we see the PMD and find a struct page * or we see a PTE and find the same struct page * (compound page head). The only thing that needs to be guaranteed is that both PTEs and PMD page-tables are valid. Is this not so? > To avoid that, the pmd is updated, then serialize_against_pte_lookup is > ran. Serialize runs a do_nothing in all cpu in cpu_mask. > > So, after all cpus finish running do_nothing(), there is a guarantee > that if there is any 'lockless pagetable walk' it is running on top of > a updated version of this pmd, and so, collapsing/splitting THP is > safe. But why would it matter?! It would find the same struct page * through either version of the page-tables. *confused* > > Also, specifically to munmap(), this seems entirely superfluous, > > munmap() uses the normal page-table freeing code and should be entirely > > fine without additional waiting. > > To be honest, I remember it being needed in munmap case, but I really > don't remember the details. I will take a deeper look and come back > with this answer. munmap does normal mmu_gather page-table teardown, the THP PMD should be RCU-like freed just like any other PMD. Which should be perfectly safe vs lockless page-table walks. If you can find anything there that isn't right, please explain that in detail and we'll need to look hard at fixing _that_. > > Furthermore, Power never accurately tracks mm_cpumask(), so using that > > makes the whole thing more expensive than it needs to be. Also, I > > suppose that is buggered vs file backed THP. > > That accuracy of mm_cpumask is above my knowledge right now. =) Basically PowerPC only ever sets bits in there, unlike x86 that also clears bits (at expense, but it's worth it for us).
