Re: [PATCH v2] mmu_gather: move tlb flush for VM_PFNMAP/VM_MIXEDMAP vmas into free_pgtables()

[Peter Zijlstra <peterz@infradead.org>]

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.