| Message ID | 20190722154210.42799-13-steven.price@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Generic page walk and ptdump | expand |
On 07/22/2019 09:12 PM, Steven Price wrote: > Since 48684a65b4e3: "mm: pagewalk: fix misbehavior of walk_page_range > for vma(VM_PFNMAP)", page_table_walk() will report any kernel area as > a hole, because it lacks a vma. > > This means each arch has re-implemented page table walking when needed, > for example in the per-arch ptdump walker. > > Remove the requirement to have a vma except when trying to split huge > pages. > > Signed-off-by: Steven Price <steven.price@arm.com> > --- > mm/pagewalk.c | 25 +++++++++++++++++-------- > 1 file changed, 17 insertions(+), 8 deletions(-) > > diff --git a/mm/pagewalk.c b/mm/pagewalk.c > index 98373a9f88b8..1cbef99e9258 100644 > --- a/mm/pagewalk.c > +++ b/mm/pagewalk.c > @@ -36,7 +36,7 @@ static int walk_pmd_range(pud_t *pud, unsigned long addr, unsigned long end, > do { > again: > next = pmd_addr_end(addr, end); > - if (pmd_none(*pmd) || !walk->vma) { > + if (pmd_none(*pmd)) { > if (walk->pte_hole) > err = walk->pte_hole(addr, next, walk); > if (err) > @@ -59,9 +59,14 @@ static int walk_pmd_range(pud_t *pud, unsigned long addr, unsigned long end, > if (!walk->pte_entry) > continue; > > - split_huge_pmd(walk->vma, pmd, addr); > - if (pmd_trans_unstable(pmd)) > - goto again; > + if (walk->vma) { > + split_huge_pmd(walk->vma, pmd, addr); Check for a PMD THP entry before attempting to split it ? > + if (pmd_trans_unstable(pmd)) > + goto again; > + } else if (pmd_leaf(*pmd)) { > + continue; > + } > + > err = walk_pte_range(pmd, addr, next, walk); > if (err) > break; > @@ -81,7 +86,7 @@ static int walk_pud_range(p4d_t *p4d, unsigned long addr, unsigned long end, > do { > again: > next = pud_addr_end(addr, end); > - if (pud_none(*pud) || !walk->vma) { > + if (pud_none(*pud)) { > if (walk->pte_hole) > err = walk->pte_hole(addr, next, walk); > if (err) > @@ -95,9 +100,13 @@ static int walk_pud_range(p4d_t *p4d, unsigned long addr, unsigned long end, > break; > } > > - split_huge_pud(walk->vma, pud, addr); > - if (pud_none(*pud)) > - goto again; > + if (walk->vma) { > + split_huge_pud(walk->vma, pud, addr); Check for a PUD THP entry before attempting to split it ? > + if (pud_none(*pud)) > + goto again; > + } else if (pud_leaf(*pud)) { > + continue; > + } This is bit cryptic. walk->vma check should be inside a helper is_user_page_table() or similar to make things clear. p4d_leaf() check missing in walk_p4d_range() for kernel page table walk ? Wondering if p?d_leaf() test should be moved earlier while calling p?d_entry() for kernel page table walk.
On 28/07/2019 15:20, Anshuman Khandual wrote: > > > On 07/22/2019 09:12 PM, Steven Price wrote: >> Since 48684a65b4e3: "mm: pagewalk: fix misbehavior of walk_page_range >> for vma(VM_PFNMAP)", page_table_walk() will report any kernel area as >> a hole, because it lacks a vma. >> >> This means each arch has re-implemented page table walking when needed, >> for example in the per-arch ptdump walker. >> >> Remove the requirement to have a vma except when trying to split huge >> pages. >> >> Signed-off-by: Steven Price <steven.price@arm.com> >> --- >> mm/pagewalk.c | 25 +++++++++++++++++-------- >> 1 file changed, 17 insertions(+), 8 deletions(-) >> >> diff --git a/mm/pagewalk.c b/mm/pagewalk.c >> index 98373a9f88b8..1cbef99e9258 100644 >> --- a/mm/pagewalk.c >> +++ b/mm/pagewalk.c >> @@ -36,7 +36,7 @@ static int walk_pmd_range(pud_t *pud, unsigned long addr, unsigned long end, >> do { >> again: >> next = pmd_addr_end(addr, end); >> - if (pmd_none(*pmd) || !walk->vma) { >> + if (pmd_none(*pmd)) { >> if (walk->pte_hole) >> err = walk->pte_hole(addr, next, walk); >> if (err) >> @@ -59,9 +59,14 @@ static int walk_pmd_range(pud_t *pud, unsigned long addr, unsigned long end, >> if (!walk->pte_entry) >> continue; >> >> - split_huge_pmd(walk->vma, pmd, addr); >> - if (pmd_trans_unstable(pmd)) >> - goto again; >> + if (walk->vma) { >> + split_huge_pmd(walk->vma, pmd, addr); > > Check for a PMD THP entry before attempting to split it ? split_huge_pmd does the check for us: > #define split_huge_pmd(__vma, __pmd, __address) \ > do { \ > pmd_t *____pmd = (__pmd); \ > if (is_swap_pmd(*____pmd) || pmd_trans_huge(*____pmd) \ > || pmd_devmap(*____pmd)) \ > __split_huge_pmd(__vma, __pmd, __address, \ > false, NULL); \ > } while (0) And this isn't a change from the previous code - only that the entry is no longer split when walk->vma==NULL. >> + if (pmd_trans_unstable(pmd)) >> + goto again; >> + } else if (pmd_leaf(*pmd)) { >> + continue; >> + } >> + >> err = walk_pte_range(pmd, addr, next, walk); >> if (err) >> break; >> @@ -81,7 +86,7 @@ static int walk_pud_range(p4d_t *p4d, unsigned long addr, unsigned long end, >> do { >> again: >> next = pud_addr_end(addr, end); >> - if (pud_none(*pud) || !walk->vma) { >> + if (pud_none(*pud)) { >> if (walk->pte_hole) >> err = walk->pte_hole(addr, next, walk); >> if (err) >> @@ -95,9 +100,13 @@ static int walk_pud_range(p4d_t *p4d, unsigned long addr, unsigned long end, >> break; >> } >> >> - split_huge_pud(walk->vma, pud, addr); >> - if (pud_none(*pud)) >> - goto again; >> + if (walk->vma) { >> + split_huge_pud(walk->vma, pud, addr); > > Check for a PUD THP entry before attempting to split it ? Same as above. >> + if (pud_none(*pud)) >> + goto again; >> + } else if (pud_leaf(*pud)) { >> + continue; >> + } > > This is bit cryptic. walk->vma check should be inside a helper is_user_page_table() > or similar to make things clear. p4d_leaf() check missing in walk_p4d_range() for > kernel page table walk ? Wondering if p?d_leaf() test should be moved earlier while > calling p?d_entry() for kernel page table walk. I wasn't sure if it was worth putting p4d_leaf() and pgd_leaf() checks in (yet). No architecture that I know of uses such large pages. I'm not sure what you mean by moving the p?d_leaf() test earlier? Can you explain with an example? Thanks, Steve
On 07/29/2019 05:59 PM, Steven Price wrote: > On 28/07/2019 15:20, Anshuman Khandual wrote: >> >> >> On 07/22/2019 09:12 PM, Steven Price wrote: >>> Since 48684a65b4e3: "mm: pagewalk: fix misbehavior of walk_page_range >>> for vma(VM_PFNMAP)", page_table_walk() will report any kernel area as >>> a hole, because it lacks a vma. >>> >>> This means each arch has re-implemented page table walking when needed, >>> for example in the per-arch ptdump walker. >>> >>> Remove the requirement to have a vma except when trying to split huge >>> pages. >>> >>> Signed-off-by: Steven Price <steven.price@arm.com> >>> --- >>> mm/pagewalk.c | 25 +++++++++++++++++-------- >>> 1 file changed, 17 insertions(+), 8 deletions(-) >>> >>> diff --git a/mm/pagewalk.c b/mm/pagewalk.c >>> index 98373a9f88b8..1cbef99e9258 100644 >>> --- a/mm/pagewalk.c >>> +++ b/mm/pagewalk.c >>> @@ -36,7 +36,7 @@ static int walk_pmd_range(pud_t *pud, unsigned long addr, unsigned long end, >>> do { >>> again: >>> next = pmd_addr_end(addr, end); >>> - if (pmd_none(*pmd) || !walk->vma) { >>> + if (pmd_none(*pmd)) { >>> if (walk->pte_hole) >>> err = walk->pte_hole(addr, next, walk); >>> if (err) >>> @@ -59,9 +59,14 @@ static int walk_pmd_range(pud_t *pud, unsigned long addr, unsigned long end, >>> if (!walk->pte_entry) >>> continue; >>> >>> - split_huge_pmd(walk->vma, pmd, addr); >>> - if (pmd_trans_unstable(pmd)) >>> - goto again; >>> + if (walk->vma) { >>> + split_huge_pmd(walk->vma, pmd, addr); >> >> Check for a PMD THP entry before attempting to split it ? > > split_huge_pmd does the check for us: >> #define split_huge_pmd(__vma, __pmd, __address) \ >> do { \ >> pmd_t *____pmd = (__pmd); \ >> if (is_swap_pmd(*____pmd) || pmd_trans_huge(*____pmd) \ >> || pmd_devmap(*____pmd)) \ >> __split_huge_pmd(__vma, __pmd, __address, \ >> false, NULL); \ >> } while (0) > > And this isn't a change from the previous code - only that the entry is > no longer split when walk->vma==NULL. Does it make sense to name walk->vma check to differentiate between user and kernel page tables. IMHO that will help make things clear and explicit during page table walk. > >>> + if (pmd_trans_unstable(pmd)) >>> + goto again; >>> + } else if (pmd_leaf(*pmd)) { >>> + continue; >>> + } >>> + >>> err = walk_pte_range(pmd, addr, next, walk); >>> if (err) >>> break; >>> @@ -81,7 +86,7 @@ static int walk_pud_range(p4d_t *p4d, unsigned long addr, unsigned long end, >>> do { >>> again: >>> next = pud_addr_end(addr, end); >>> - if (pud_none(*pud) || !walk->vma) { >>> + if (pud_none(*pud)) { >>> if (walk->pte_hole) >>> err = walk->pte_hole(addr, next, walk); >>> if (err) >>> @@ -95,9 +100,13 @@ static int walk_pud_range(p4d_t *p4d, unsigned long addr, unsigned long end, >>> break; >>> } >>> >>> - split_huge_pud(walk->vma, pud, addr); >>> - if (pud_none(*pud)) >>> - goto again; >>> + if (walk->vma) { >>> + split_huge_pud(walk->vma, pud, addr); >> >> Check for a PUD THP entry before attempting to split it ? > > Same as above. > >>> + if (pud_none(*pud)) >>> + goto again; >>> + } else if (pud_leaf(*pud)) { >>> + continue; >>> + } >> >> This is bit cryptic. walk->vma check should be inside a helper is_user_page_table() >> or similar to make things clear. p4d_leaf() check missing in walk_p4d_range() for >> kernel page table walk ? Wondering if p?d_leaf() test should be moved earlier while >> calling p?d_entry() for kernel page table walk. > > I wasn't sure if it was worth putting p4d_leaf() and pgd_leaf() checks > in (yet). No architecture that I know of uses such large pages. Just to be complete it does make sense to add the remaining possible leaf entry checks but will leave it upto you. > > I'm not sure what you mean by moving the p?d_leaf() test earlier? Can > you explain with an example? In case its a kernel p?d_leaf() entry, then there is nothing to be done after calling respective walk->p?d_entry() functions. Hence this check should not complement user page table check (walk->vma) later in the function but instead be checked right after walk->p?d_entry(). But its not a big deal I guess.
diff --git a/mm/pagewalk.c b/mm/pagewalk.c index 98373a9f88b8..1cbef99e9258 100644 --- a/mm/pagewalk.c +++ b/mm/pagewalk.c @@ -36,7 +36,7 @@ static int walk_pmd_range(pud_t *pud, unsigned long addr, unsigned long end, do { again: next = pmd_addr_end(addr, end); - if (pmd_none(*pmd) || !walk->vma) { + if (pmd_none(*pmd)) { if (walk->pte_hole) err = walk->pte_hole(addr, next, walk); if (err) @@ -59,9 +59,14 @@ static int walk_pmd_range(pud_t *pud, unsigned long addr, unsigned long end, if (!walk->pte_entry) continue; - split_huge_pmd(walk->vma, pmd, addr); - if (pmd_trans_unstable(pmd)) - goto again; + if (walk->vma) { + split_huge_pmd(walk->vma, pmd, addr); + if (pmd_trans_unstable(pmd)) + goto again; + } else if (pmd_leaf(*pmd)) { + continue; + } + err = walk_pte_range(pmd, addr, next, walk); if (err) break; @@ -81,7 +86,7 @@ static int walk_pud_range(p4d_t *p4d, unsigned long addr, unsigned long end, do { again: next = pud_addr_end(addr, end); - if (pud_none(*pud) || !walk->vma) { + if (pud_none(*pud)) { if (walk->pte_hole) err = walk->pte_hole(addr, next, walk); if (err) @@ -95,9 +100,13 @@ static int walk_pud_range(p4d_t *p4d, unsigned long addr, unsigned long end, break; } - split_huge_pud(walk->vma, pud, addr); - if (pud_none(*pud)) - goto again; + if (walk->vma) { + split_huge_pud(walk->vma, pud, addr); + if (pud_none(*pud)) + goto again; + } else if (pud_leaf(*pud)) { + continue; + } if (walk->pmd_entry || walk->pte_entry) err = walk_pmd_range(pud, addr, next, walk);
Since 48684a65b4e3: "mm: pagewalk: fix misbehavior of walk_page_range for vma(VM_PFNMAP)", page_table_walk() will report any kernel area as a hole, because it lacks a vma. This means each arch has re-implemented page table walking when needed, for example in the per-arch ptdump walker. Remove the requirement to have a vma except when trying to split huge pages. Signed-off-by: Steven Price <steven.price@arm.com> --- mm/pagewalk.c | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-)