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 22:10 Jason Gunthorpe wrote:
>On Fri, Apr 24, 2020 at 10:07:19PM +0800, Li Xinhai wrote:
>> 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.
>> ...
>
>well if you are relying on the caller to not call this in wrong cases
>it would make sense to have a
>
>if (WARN_ON(!pmd_huge(*pmd)))
>    return NULL
>
>To document the assertion
> 
right, if this WARN_ON occurs, which means huge_pte_offset() been called
for a normal 4K mapping, that is a bug of caller. After inspected current code
of callers, no one tries to call it on 4K mapping.

>Jason