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!