On Fri, Jun 05, 2026 at 09:18:27AM +0200, David Hildenbrand (Arm) wrote: > On 6/4/26 19:04, Nico Pache wrote: > > On Mon, Jun 1, 2026 at 9:00 AM Nico Pache <[email protected]> wrote: > >> > >> On Mon, Jun 1, 2026 at 5:14 AM David Hildenbrand (Arm) <[email protected]> > >> wrote: > >>> > >>> > >>> Yeah. BTW, I think we'd need a spin_lock_nested(), so @Nico, treat my > >>> code as a > >>> draft. > >> > >> Okay, I read the above and did some investigating. > >> > >> I will try to implement and verify the changes you suggested :) > > > > I've implemented something slightly different actually and I *think* its > > better! > > > > } else { > > /* this is map_anon_folio_pte_nopf with no mmu update */ > > __map_anon_folio_pte_nopf(folio, pte, vma, start_addr, > > /*uffd_wp=*/ false); > > smp_wmb(); > > pmd_populate(mm, pmd, pmd_pgtable(_pmd)); > > /* > > * Some architectures (e.g. MIPS) walk the live page table in > > * their implementation. update_mmu_cache_range() must be called > > * with a valid page table hierarchy and the PTE lock held. > > * Acquire it nested inside pmd_ptl when they are distinct locks. > > */ > > if (pte_ptl != pmd_ptl) > > spin_lock_nested(pte_ptl, SINGLE_DEPTH_NESTING); > > update_mmu_cache_range(NULL, vma, start_addr, pte, nr_pages); > > if (pte_ptl != pmd_ptl) > > spin_unlock(pte_ptl); > > } > > spin_unlock(pmd_ptl); > > > > The logic here is that when the PMD becomes visible, PTEs are already > > populated (no possibility of spurious faults on local CPU) > > > > the SMP_WMB makes sure of the above
THe locks prevent those 'spurious' (really: incorrect) faults anyway so I don't think this is necessary. > > > > And the pmd is installed with the pte and pmd lock both held through > > the mmu_cache update. > > > > This follows the conventions used in pmd_install() and clears the > > potential for local CPU faults hitting cleared PTE entries. > > After the pmdp_collapse_flush() we'd be getting CPU faults due to the cleared > PMD already? So the case here is rather different. Yeah conceptually the code above is problematic because you immediately make the PTE available right at the point you populate, so taking a PTE lock after that is rather shutting the stable door after the horse has bolted. Doing it this way is not a good idea in any case because we're adding complexity, an extra function and an open-coded cache maintenance call for really no benefit. I asked Nico to abstract the anon folio mapping stuff explicitly so we could avoid this sort of duplication so let's not roll that back :) So again, I think going with the original suggestion (with an updated comment) is the right thing to do. Anyway, an aside But in practice we can't have page faults here right? The VMA is: - Ensured to span at least the PMD range (this isn't immediately obvious in the code) - VMA write locked (mmap write lock held) And we hold the anon_vma lock so no rmap walkers can walk the page tables here either. So I actually wonder, given that, whether we need the PTE PTL at all. But. At this stage it'll almost certainly be an owned exclusive cache line so it's very low cost to do it, and it means we honour the update_mmu_cache_range() contract. And it also makes it clear that we're gating changes on the PTE being untouchable so any future stuff that maybe changes some of these rules doesn't get caught out. So probably worth keeping. > > -- > Cheers, > > David Thanks, Lorenzo
