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

[Roman Gushchin]

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.

Do I misunderstand this?

Thanks