Re: [PATCH v5 2/4] mm/memory.c: Update local TLB if PTE entry exists

[maobibo]

On 05/22/2020 03:22 AM, Andrew Morton wrote:
> On Thu, 21 May 2020 11:30:35 +0800 Bibo Mao <maobibo@xxxxxxxxxxx> wrote:
> 
>> If two threads concurrently fault at the same address, the thread that
>> won the race updates the PTE and its local TLB. For now, the other
>> thread gives up, simply does nothing, and continues.
>>
>> It could happen that this second thread triggers another fault, whereby
>> it only updates its local TLB while handling the fault. Instead of
>> triggering another fault, let's directly update the local TLB of the
>> second thread.
>>
>> It is only useful to architectures where software can update TLB, it may
>> bring out some negative effect if update_mmu_cache is used for other
>> purpose also. It seldom happens where multiple threads access the same
>> page at the same time, so the negative effect is limited on other arches.
>>
>> With specjvm2008 workload, smp-race pgfault counts is about 3% to 4%
>> of the total pgfault counts by watching /proc/vmstats information
>>
> 
> I'm sorry to keep thrashing this for so long, but I'd really prefer not
> to add any overhead to architectures which don't need it.  However,
> we're getting somewhere!
> 
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -2436,10 +2436,9 @@ static inline bool cow_user_page(struct page *dst, struct page *src,
>>  		if (!likely(pte_same(*vmf->pte, vmf->orig_pte))) {
>>  			/*
>>  			 * Other thread has already handled the fault
>> -			 * and we don't need to do anything. If it's
>> -			 * not the case, the fault will be triggered
>> -			 * again on the same address.
>> +			 * and update local tlb only
>>  			 */
>> +			update_mmu_cache(vma, addr, vmf->pte);
> 
> Now, all the patch does is to add new calls to update_mmu_cache().
> 
> So can we replace all these with a call to a new
> update_mmu_cache_sw_tlb() (or whatever) which is a no-op on
> architectures which don't need the additional call?
> 
> Also, I wonder about the long-term maintainability.  People who
> regularly work on this code won't be thinking of this MIPS peculiarity
> and it's likely that any new calls to update_mmu_cache_sw_tlb() won't
> be added where they should have been.  Hopefully copy-and-paste from
> the existing code will serve us well.  Please do ensure that the
> update_mmu_cache_sw_tlb() implementation is carefully commented so
> that people can understand where they should (and shouldn't) include
> this call.
Well, I will do that. MIPS is actually somewhat different with generic
architectures, and old MIPS system does not support hardware page walk,
it requires software to update TLB entry.

regards
bibo, mao