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

[Hugh Dickins <hughd@google.com>]

On Fri, 24 Jan 2025, Peter Zijlstra wrote:
> On Thu, Jan 23, 2025 at 08:42:36PM -0800, Hugh Dickins wrote:
> > The changelog of commit b67fbebd4cf9 ("mmu_gather: Force tlb-flush
> > VM_PFNMAP vmas") has not helped me either.  Nor could I locate any
> > discussion (Jann, Linus, Peter, Will?) that led up to it.
> 
> Hmm, that was probably on security -- I should have those mails around
> somewhere, I'll see if I can dig them up.

That was very helpful, thank you: I've gone through a lot of confusion,
but feeling more confident about it all today.

> 
> > To me, Peter's patch looks much like yours, except wth different
> > names and comments, plus the "vma" error you point out below.
> 
> Yes, 3 differences:
> 
>  - naming;
>  - the extra check;
>  - the vma_pfn clearing condition.
> 
> Under the assumption that this is all about those PFNs, the argument
> (as also outlined in the email to Roman just now) is that you only need
> to flush if both: you have pending TLBI for PFN and are indeed about to
> unlink a PFN vma.
> 
> 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.

> 
> 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; 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).

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().

Hugh