| Message ID | 20211126022834.1622106-1-npiggin@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | powerpc/64s/radix: Fix unmapping huge vmaps when CONFIG_HUGETLB_PAGE=n | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/github-powerpc_ppctests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_selftests | success | Successfully ran 8 jobs. |
| snowpatch_ozlabs/github-powerpc_sparse | success | Successfully ran 4 jobs. |
| snowpatch_ozlabs/github-powerpc_kernel_qemu | success | Successfully ran 24 jobs. |
| snowpatch_ozlabs/github-powerpc_clang | success | Successfully ran 7 jobs. |
Hi, > pmd_huge is defined out to false when HUGETLB_PAGE is not configured, > but the vmap code still installs huge PMDs. This leads to errors > encountering bad PMDs when vunmapping because it is not seen as a > huge PTE, and the bad PMD check catches it. The end result may not > be much more serious than some bad pmd warning messages, because the > pmd_none_or_clear_bad() does what we wanted and clears the huge PTE > anyway. Huh. So vmap seems to key off arch_vmap_p?d_supported which checks for radix and HAVE_ARCH_HUGE_VMAP. > Fix this by checking pmd_is_leaf(), which checks for a PTE regardless > of config options. The whole huge/large/leaf stuff is a tangled mess > but that's kernel-wide and not something we can improve much in > arch/powerpc code. I guess I'm a bit late to the party here because p?d_is_leaf was added in 2019 in commit d6eacedd1f0e ("powerpc/book3s: Use config independent helpers for page table walk") but why wouldn't we just make pmd_huge() not config dependent? Also, looking at that commit, there are a few places that might still throw warnings, e.g. find_linux_pte, find_current_mm_pte, pud_page which seem like they might still throw warnings if they were to encounter a huge vmap page: struct page *pud_page(pud_t pud) { if (pud_is_leaf(pud)) { VM_WARN_ON(!pud_huge(pud)); Do these functions need special treatment for huge vmappings()? Apart from those questions, the patch itself makes sense to me and I can follow how it would fix a problem. Reviewed-by: Daniel Axtens <dja@axtens.net> Kind regards, Daniel
Excerpts from Daniel Axtens's message of November 26, 2021 4:09 pm: > Hi, > >> pmd_huge is defined out to false when HUGETLB_PAGE is not configured, >> but the vmap code still installs huge PMDs. This leads to errors >> encountering bad PMDs when vunmapping because it is not seen as a >> huge PTE, and the bad PMD check catches it. The end result may not >> be much more serious than some bad pmd warning messages, because the >> pmd_none_or_clear_bad() does what we wanted and clears the huge PTE >> anyway. > > Huh. So vmap seems to key off arch_vmap_p?d_supported which checks for > radix and HAVE_ARCH_HUGE_VMAP. > >> Fix this by checking pmd_is_leaf(), which checks for a PTE regardless >> of config options. The whole huge/large/leaf stuff is a tangled mess >> but that's kernel-wide and not something we can improve much in >> arch/powerpc code. > > I guess I'm a bit late to the party here because p?d_is_leaf was added > in 2019 in commit d6eacedd1f0e ("powerpc/book3s: Use config independent > helpers for page table walk") but why wouldn't we just make pmd_huge() > not config dependent? I guess so it constant folds code if hugetlbfs is not configured (and maybe so !huge kernels would correctly print a bad PMD warning if they got huge PMD in user mappings). > > Also, looking at that commit, there are a few places that might still > throw warnings, e.g. find_linux_pte, find_current_mm_pte, pud_page which > seem like they might still throw warnings if they were to encounter a > huge vmap page: > > struct page *pud_page(pud_t pud) > { > if (pud_is_leaf(pud)) { > VM_WARN_ON(!pud_huge(pud)); Oh, hmm. That is used in vmalloc.c so maybe that warning should be removed as a false positive. Good catch. > Do these functions need special treatment for huge vmappings()? find_linux_pte etc could be called for vmaps. I'm not sure I see a problem in that function. Thanks, Nick > > Apart from those questions, the patch itself makes sense to me and I can > follow how it would fix a problem. > > Reviewed-by: Daniel Axtens <dja@axtens.net> > > Kind regards, > Daniel >
diff --git a/arch/powerpc/mm/book3s64/radix_pgtable.c b/arch/powerpc/mm/book3s64/radix_pgtable.c index 99dbee114539..7559638068ef 100644 --- a/arch/powerpc/mm/book3s64/radix_pgtable.c +++ b/arch/powerpc/mm/book3s64/radix_pgtable.c @@ -1089,7 +1089,7 @@ int pud_set_huge(pud_t *pud, phys_addr_t addr, pgprot_t prot) int pud_clear_huge(pud_t *pud) { - if (pud_huge(*pud)) { + if (pud_is_leaf(*pud)) { pud_clear(pud); return 1; } @@ -1136,7 +1136,7 @@ int pmd_set_huge(pmd_t *pmd, phys_addr_t addr, pgprot_t prot) int pmd_clear_huge(pmd_t *pmd) { - if (pmd_huge(*pmd)) { + if (pmd_is_leaf(*pmd)) { pmd_clear(pmd); return 1; }
pmd_huge is defined out to false when HUGETLB_PAGE is not configured, but the vmap code still installs huge PMDs. This leads to errors encountering bad PMDs when vunmapping because it is not seen as a huge PTE, and the bad PMD check catches it. The end result may not be much more serious than some bad pmd warning messages, because the pmd_none_or_clear_bad() does what we wanted and clears the huge PTE anyway. Fix this by checking pmd_is_leaf(), which checks for a PTE regardless of config options. The whole huge/large/leaf stuff is a tangled mess but that's kernel-wide and not something we can improve much in arch/powerpc code. Fixes: d909f9109c30 ("powerpc/64s/radix: Enable HAVE_ARCH_HUGE_VMAP") Signed-off-by: Nicholas Piggin <npiggin@gmail.com> --- arch/powerpc/mm/book3s64/radix_pgtable.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)