Re: [PATCH v3] mm/hugetlb: fix hugetlb vs. core-mm PT locking

[David Hildenbrand]

On 01.08.24 18:07, Peter Xu wrote:
> On Thu, Aug 01, 2024 at 05:35:20PM +0200, David Hildenbrand wrote:
> > Hi Peter,
>
> [...]
>
> > > > +	else if (size >= PUD_SIZE)
> > > > +		return pud_lockptr(mm, (pud_t *) pte);
> > > > +	else if (size >= PMD_SIZE || IS_ENABLED(CONFIG_HIGHPTE))
> > >
> > > I thought this HIGHPTE can also be dropped? Because in HIGHPTE it should
> > > never have lower-than-PMD huge pages or we're in trouble.  That's why I
> > > kept one WARN_ON() in my pesudo code but only before trying to take the pte
> > > lockptr.
> >
> > Then the compiler won't optimize out the ptep_lockptr() call and we'll run
> > into a build error. And I think the HIGHPTE builderror serves good purpose.
> >
> > In file included from <command-line>:
> > In function 'ptep_lockptr',
> >      inlined from 'huge_pte_lockptr' at ./include/linux/hugetlb.h:974:9,
> >      inlined from 'huge_pte_lock' at ./include/linux/hugetlb.h:1248:8,
> >      inlined from 'pagemap_scan_hugetlb_entry' at fs/proc/task_mmu.c:2581:8:
> > ././include/linux/compiler_types.h:510:45: error: call to '__compiletime_assert_256' declared with attribute error: BUILD_BUG_ON failed: IS_ENABLED(CONFIG_HIGHPTE)
> >    510 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> >        |                                             ^
> > ././include/linux/compiler_types.h:491:25: note: in definition of macro '__compiletime_assert'
> >    491 |                         prefix ## suffix();                             \
> >        |                         ^~~~~~
> > ././include/linux/compiler_types.h:510:9: note: in expansion of macro '_compiletime_assert'
> >    510 |         _compiletime_assert(condition, msg, __compiletime_assert_, __COUNTER__)
> >        |         ^~~~~~~~~~~~~~~~~~~
> > ./include/linux/build_bug.h:39:37: note: in expansion of macro 'compiletime_assert'
> >     39 | #define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
> >        |                                     ^~~~~~~~~~~~~~~~~~
> > ./include/linux/build_bug.h:50:9: note: in expansion of macro 'BUILD_BUG_ON_MSG'
> >     50 |         BUILD_BUG_ON_MSG(condition, "BUILD_BUG_ON failed: " #condition)
> >        |         ^~~~~~~~~~~~~~~~
> > ./include/linux/mm.h:2874:9: note: in expansion of macro 'BUILD_BUG_ON'
> >   2874 |         BUILD_BUG_ON(IS_ENABLED(CONFIG_HIGHPTE));
>
> Ahh.. this is in "ifdef USE_SPLIT_PTE_PTLOCKS" section.  I'm thinking maybe
> we should drop this BUILD_BUG_ON - it says "HIGHPTE shouldn't co-exist with
> USE_SPLIT_PTE_PTLOCKS", but I think it can?
>
> Said that, I think I can also understand your point, where you see
> ptep_lockptr() a hugetlb-only function, in that case the BUILD_BUG_ON would
> make sense in hugetlb world.
>
> So.. per my previous nitpick suggestion, IIUC we'll need to drop this
> BUILD_BUG_ON, just to say "USE_SPLIT_PTE_PTLOCKS can work with HIGHPTE" and
> perhaps slightly more readable; we'll rely on the WARN_ON to guard HIGHPTE
> won't use pte lock.

I really don't want to  drop the BUILD_BUG_ON. The function cannot 
possibly work with HIGHPTE, especially once used in other context by 
accident.

So I'll leave it like that. Feel free to optimize the hugetlb code 
further once the fix has landed (e.g., really optimize it out if we 
cannot possibly have such hugetlb sizes).

Thanks!

--
Cheers,

David / dhildenb