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!