Re: [PATCH v3] mm/hugetlb: fix hugetlb vs. core-mm PT locking
From: Peter Xu
Date: Thu Aug 01 2024 - 12:09:08 EST
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