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

[Peter Xu]

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.

Either way works for me, thanks!

-- 
Peter Xu