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

[Peter Zijlstra <peterz@infradead.org>]

On Sun, Jan 26, 2025 at 06:34:48PM -0800, Hugh Dickins wrote:

> > If we've flushed the relevant PFNs earlier, for whatever reason,
> > batching, or the arch has !MERGE_VMAS or whatever, then we do not need
> > to flush again. So clearing vma_pfn in __tlb_reset_range() is the right
> > place.
> 
> Yes, Roman moved to clearing vma_pfn in __tlb_reset_range() in his v3:
> we are all in agreement on that.

Ah, I had not seen v3 yet.

> > Similarly, if we don't ever actually free/unlink the PFN vma, we also
> > don't care.
> 
> I cannot think of a case in which we arrive at free_pgtables(), but do not
> unlink the vma(s) which caused vma_pfn to be set.  If there is such a case,
> it's not worth optimizing for;

Yeah, I suppose it doesn't happen. But I figured why assume stuff.

> and wrong to check just the first vma in the
> list (don't look only at the stable commit 895428ee124a which Roman cited -
> it had to be fixed by 891f03f688de afterwards).

Duh, yeah, so tlb_free_vma() wants to be inside the vma loop of
free_pgtables().

> Personally, I prefer code inline in free_pgtables() which shows what's
> going on, as Roman did in v1, rather than struggling to devise a
> self-explanatory function name for something over there in tlb.h.
> 
> But I may be in a minority on that, and his tlb_flush_mmu_pfnmap()
> is much more to the point than tlb_free_vma().

I prefer a function over the in-line thing such that mmu-gather is more
or less self contained.

So my concern is/was maintainability of all this; tlb_flush_mmu_pfnmap()
tells me nothing about when this function should be called. Otoh
tlb_free_vma() tell me this should be called when we're freeing VMAs --
much harder to misplace etc. If we have hooks placed at, and named
after, natural events in the lifetime of things, placement is 'obvious'.

Another possible name might be tlb_free_pgtables(), indicating we're
about to start freeing pagetables -- but it would need to assert
!tlb->freed_tables, and I'm not sure this is a constraint worth
imposing. It would bring pain if someone wanted to mix freeing pages and
page-tables.

And we already have vma based hooks, so I much prefer adding one more of
those. This is about funky VMAs after all.