Re: [PATCH v1 4/4] mm/hugetlb: fix excessive IPI broadcasts when unsharing PMD tables using mmu_gather

["David Hildenbrand (Red Hat)" <david@kernel.org>]

>>>
>>> As Nadav points out, should also initialise fully_unshared_tables.
>>
>> Right, but on an earlier init path, not on the range reset path here.
> 
> Shouldn't we reset it also?
> 
> I mean __tlb_reset_range() is also called by __tlb_gather_mmu() (invoked by
> tlb_gather_mmu[_fullmm]()).
> 

__tlb_reset_range() is all about flushing the TLB.

In v2 I have:

diff --git a/mm/mmu_gather.c b/mm/mmu_gather.c
index 247e3f9db6c7a..030a162a263ba 100644
--- a/mm/mmu_gather.c
+++ b/mm/mmu_gather.c
@@ -426,6 +426,7 @@ static void __tlb_gather_mmu(struct mmu_gather *tlb, struct mm_struct *mm,
  #endif
         tlb->vma_pfn = 0;
  
+       tlb->fully_unshared_tables = 0;
         __tlb_reset_range(tlb);
         inc_tlb_flush_pending(tlb->mm);
  }


>>
>>>
>>>>    	/*
>>>>    	 * Do not reset mmu_gather::vma_* fields here, we do not
>>>>    	 * call into tlb_start_vma() again to set them if there is an
>>>> @@ -484,7 +496,7 @@ static inline void tlb_flush_mmu_tlbonly(struct mmu_gather *tlb)
>>>>    	 * these bits.
>>>>    	 */
>>>>    	if (!(tlb->freed_tables || tlb->cleared_ptes || tlb->cleared_pmds ||
>>>> -	      tlb->cleared_puds || tlb->cleared_p4ds))
>>>> +	      tlb->cleared_puds || tlb->cleared_p4ds || tlb->unshared_tables))
>>>
>>> What about fully_unshared_tables? I guess though unshared_tables implies
>>> fully_unshared_tables.
>>
>> fully_unshared_tables is only for triggering IPIs and consequently not about
>> flushing TLBs.
>>
>> The TLB part is taken care of by unshared_tables, and we will always set
>> unshared_tables when unsharing any page tables (incl. fully unshared ones).
> 
> OK, so is there ever a situation where fully_unshared_tables would be set
> without unshared_tables? Presumably not.

tlb_unshare_pmd_ptdesc() will always set "unshared_tables" but only conditionally
sets "fully_unshared_tables".

"unshared_tables" might get handled by a prior TLB flush,
leaving only fully_unshared_tables set to perform the IPI in tlb_flush_unshared_tables().

So the important part is that whenever we unshare, we set "unshared_tables".

[...]

>>>> +{
>>>> +	/*
>>>> +	 * As soon as the caller drops locks to allow for reuse of
>>>> +	 * previously-shared tables, these tables could get modified and
>>>> +	 * even reused outside of hugetlb context. So flush the TLB now.
>>>
>>> Hmm but you're doing this in both the case of unshare and fully unsharing, so is
>>> this the right place to make this comment?
>>
>> That's why I start the comment below with "Similarly", to make it clear that
>> the comments build up on each other.
>>
>> But I'm afraid I might not be getting your point fully here :/
> 
> what I mean is, if we are not at the point of the table being fully unshared,
> nobody else can come in and reuse it right? Because we're still using it, just
> dropped a ref + flushed tlb?

After we drop the lock, someone else could fully unshare it. And that other (MM) would
not be able to flush the TLB for us -- in contrast to the IPI that would affect all
CPUs.

> 
> Isn't really the correct comment here that ranges that previously mapped the
> shared pages might no longer, so we must clear the TLB? I may be missing
> something :)

There are cases where we defer flushing the TLB until we dropped all (exclusive) locks.
In particular, MADV_DONTNEED does that in some cases, essentially deferring the flush
to the tlb_finish_mmu().

free_pgtables() will also defer the flush, performing the TLB flush during tlb_finish_mmu(),
before

The point is (as I tried to make clear in the comment), for unsharing we have no control
whenn the page table gets freed after we drop the lock.

So we must flush the TLB now and cannot defer it like we do in the other cases.

> 
> Or maybe the right thing is 'we must always flush the TLB because <blahdyblah>,
> and if we are fully unsharing tables we must avoid reuse of previously-shared
> tables when the caller drops the locks' or something?

I hope the description above made it clearer why I spell out that the TLB must be flushed
now.

> 
>>
>>>
>>> Surely here this is about flushing TLBs for the unsharer only as it no longer
>>> uses it?
>>>
>>>> +	 *
>>>> +	 * Note that we cannot defer the flush to a later point even if we are
>>>> +	 * not the last sharer of the page table.
>>>> +	 */
>>>
>>> Not hugely clear, some double negative here. Maybe worth saying something like:
>>>
>>> 'Even if we are not fully unsharing a PMD table, we must flush the TLB for the
>>> unsharer who no longer has access to this memory'
>>>
>>> Or something? Assuming this is accurate :)
>>
>> I'll adjust it to "Not that even if we are not fully unsharing a PMD table,
>> we must flush the TLB for the unsharer now.".
> 
> I guess you mean Note or that's even more confusing :P

:) Yeah, I did that in v2.

-- 
Cheers

David