Re: [PATCH] mm/hugetlb: avoid unnecessary check on pud and pmd entry in huge_pte_offset

["Li Xinhai" <lixinhai.lxh@gmail.com>]

On 2020-04-24 at 21:42 Jason Gunthorpe wrote:
>On Fri, Apr 24, 2020 at 09:33:40PM +0800, Li Xinhai wrote:
>> >
>> >> , so when sz == PMD_SIZE, pmd_offset() only called with a valid PUD
>> >> entry which point to PMD page table.
>> >
>> >But what prevents pud_huge?
>> >
>> if sz == PUD_SIZE, the 'return (pte_t*)pud' alrady end the function, which cover
>> pud_huge() and pud_none(), because we the mapping is for PUD_SIZE huge page.
>>
>> So, there is no possibility for pmd_offset() been called with invalid pud entry.
>> Below is the code I used for test which has BUG_ON, that should give more
>> clear idea about the semantics of code path:
>>
>> ...
>> pud = pud_offset(p4d, addr);
>> if (sz == PUD_SIZE) {
>> /* must be pud_huge or pud_none */
>> BUG_ON(!pud_huge(*pud) && !pud_none(*pud));
>> return (pte_t *)pud; // note that return valid pointer for pud_none() case,
>> // instead of NULL, that is same semantics as existing code.
>> }
>> if (!pud_present(*pud))
>> return NULL; // note that only return NULL in case pud not present,
>> // same sematics as existing code.
>> /* must have a valid entry and size to go further */
>> BUG_ON(sz != PMD_SIZE);
>>
>> pmd = pmd_offset(pud, addr);
>> /* must be pmd_huge or pmd_none */
>> BUG_ON(!pmd_huge(*pmd) && !pmd_none(*pmd));
>
>But why is !pmd_huge() ? The prior code returned null here, is that
>dead code? Your commit message should explain all of this..
> 
let's see exising code for pmd part, the reason are in comments:
...
        pmd = pmd_offset(pud, addr);
        if (sz != PMD_SIZE && pmd_none(*pmd))
                return NULL; // dead code, must sz == PMD_SIZE
        /* hugepage or swap? */
        if (pmd_huge(*pmd) || !pmd_present(*pmd)) // !pmd_present() also cover pmd_none(),
                return (pte_t *)pmd; // so, all possible and valid value in pmd entry will reach here.

	return NULL; // dead code; can we have (!pmd_huge() && pmd_present()) and reach here? 
		// no, because this is a hugetlb mapping. otherwise, there is invalid value in pmd entry.
...

>Jason