Re: [RFC PATCH 1/2] mm: add spurious fault fixing support for huge pmd

[Huang, Ying]

Hi, David,

Thanks for review!

David Hildenbrand <david@xxxxxxxxxx> writes:

> On 15.09.25 05:29, Huang Ying wrote:
>> In the current kernel, there is spurious fault fixing support for pte,
>> but not for huge pmd because no architectures need it. But in the
>> next patch in the series, we will change the write protection fault
>> handling logic on arm64, so that some stale huge pmd entries may
>> remain in the TLB. These entries need to be flushed via the huge pmd
>> spurious fault fixing mechanism.
>> Signed-off-by: Huang Ying <ying.huang@xxxxxxxxxxxxxxxxx>
>> Cc: Catalin Marinas <catalin.marinas@xxxxxxx>
>> Cc: Will Deacon <will@xxxxxxxxxx>
>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>> Cc: David Hildenbrand <david@xxxxxxxxxx>
>> Cc: Lorenzo Stoakes <lorenzo.stoakes@xxxxxxxxxx>
>> Cc: Vlastimil Babka <vbabka@xxxxxxx>
>> Cc: Zi Yan <ziy@xxxxxxxxxx>
>> Cc: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>
>> Cc: Ryan Roberts <ryan.roberts@xxxxxxx>
>> Cc: Yang Shi <yang@xxxxxxxxxxxxxxxxxxxxxx>
>> Cc: "Christoph Lameter (Ampere)" <cl@xxxxxxxxxx>
>> Cc: Dev Jain <dev.jain@xxxxxxx>
>> Cc: Barry Song <baohua@xxxxxxxxxx>
>> Cc: Anshuman Khandual <anshuman.khandual@xxxxxxx>
>> Cc: Yicong Yang <yangyicong@xxxxxxxxxxxxx>
>> Cc: Kefeng Wang <wangkefeng.wang@xxxxxxxxxx>
>> Cc: Kevin Brodsky <kevin.brodsky@xxxxxxx>
>> Cc: Yin Fengwei <fengwei_yin@xxxxxxxxxxxxxxxxx>
>> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
>> Cc: linux-kernel@xxxxxxxxxxxxxxx
>> Cc: linux-mm@xxxxxxxxx
>> ---
>
> [...]
>
>>     int copy_huge_pmd(struct mm_struct *dst_mm, struct mm_struct
>> *src_mm,
>> @@ -1857,7 +1861,20 @@ void huge_pmd_set_accessed(struct vm_fault *vmf)
>>   	if (unlikely(!pmd_same(*vmf->pmd, vmf->orig_pmd)))
>>   		goto unlock;
>>   -	touch_pmd(vmf->vma, vmf->address, vmf->pmd, write);
>> +	if (!touch_pmd(vmf->vma, vmf->address, vmf->pmd, write)) {
>> +		/* Skip spurious TLB flush for retried page fault */
>> +		if (vmf->flags & FAULT_FLAG_TRIED)
>> +			goto unlock;
>> +		/*
>> +		 * This is needed only for protection faults but the arch code
>> +		 * is not yet telling us if this is a protection fault or not.
>> +		 * This still avoids useless tlb flushes for .text page faults
>> +		 * with threads.
>> +		 */
>
> Can we instead just remove these comments and simplly say "see
> handle_pte_fault()"

Sure.

>> +		if (vmf->flags & FAULT_FLAG_WRITE)
>> +			flush_tlb_fix_spurious_fault_pmd(vmf->vma, vmf->address,
>> +							 vmf->pmd);
>> +	}
>
> Okay, In the PTE case, we call flush_tlb_fix_spurious_fault() during
> write faults if ptep_set_access_flags() returned "0".
>
> You are calling flush_tlb_fix_spurious_fault_pmd() during a write
> fault when pmdp_set_access_flags() returned "0" as well.
>
> In general, LGTM, but I would just let touch_pmd() return the value of
> pmdp_set_access_flags() instead and add a quick comment for
> touch_pmd() what the return value means.

Sure.

---
Best Regards,
Huang, Ying