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.
