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
