On Sat, Jan 25, 2025 at 01:23:54AM +0000, Roman Gushchin wrote:
> On Fri, Jan 24, 2025 at 09:22:50AM +0100, Peter Zijlstra wrote:
> > On Thu, Jan 23, 2025 at 11:12:33PM +0000, Roman Gushchin wrote:
> >
> > > > +static inline void tlb_free_vma(struct mmu_gather *tlb, struct vm_area_struct *vma)
> > > > {
> > > > if (tlb->fullmm)
> > > > return;
> > > >
> > > > /*
> > > > * VM_PFNMAP is more fragile because the core mm will not track the
> > > > + * page mapcount -- there might not be page-frames for these PFNs
> > > > + * after all.
> > > > + *
> > > > + * Specifically() there is a race between munmap() and
> > > > + * unmap_mapping_range(), where munmap() will unlink the VMA, such
> > > > + * that unmap_mapping_range() will no longer observe the VMA and
> > > > + * no-op, without observing the TLBI, returning prematurely.
> > > > + *
> > > > + * So if we're about to unlink such a VMA, and we have pending
> > > > + * TLBI for such a vma, flush things now.
> > > > */
> > > > + if ((vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP)) && tlb->vma_pfn)
> > > > tlb_flush_mmu_tlbonly(tlb);
> > >
> > > Why do we need to re-check vma->vm_flags & (VM_PFNMAP|VM_MIXEDMAP) here?
> >
> > No need, but an opportunity.
> >
> > > In free_pgtables() we're iterating over multiple vma's. What if the first has
> > > no VM_PFNMAP set, but some other do? Idk if it's even possible, but it's not
> > > obvious that it's not possible either.
> >
> > If we only need to flush PFN entries before unlinking PFN VMAs, then:
> >
> > - if there are no PFNs pending (vma_pfn), we don't need to flush;
> > - if no PFN vma is being freed (vm_flags), we don't need to flush.
>
> Right, but if I understand the code correctly, more than one vma can be
> freed by a single free_pgtables() invocation. Should we then check
> each vma's flags in the while loop in free_pgtables()? But then
> we're back to where we're now with multiple flushes.
Right, I misplaced it -- it should be in the vma loop.
> Do I misunderstand this?
I'm not sure how this would cause more flushes; notably it will not
cause flushes where no page-tables are dropped, eg. MADV, which was why
you started all this IIUC.
