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

[Jason Gunthorpe <jgg@ziepe.ca>]

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

Jason