| Message ID | 20200626135216.24314-4-richard.weiyang@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/mremap: cleanup move_page_tables() a little | expand |
On Fri, Jun 26, 2020 at 09:52:15PM +0800, Wei Yang wrote: > Page tables is moved on the base of PMD. This requires both source > and destination range should meet the requirement. > > Current code works well since move_huge_pmd() and move_normal_pmd() > would check old_addr and new_addr again. And then return to move_ptes() > if the either of them is not aligned. > > In stead of calculating the extent separately, it is better to calculate > in one place, so we know it is not necessary to try move pmd. By doing > so, the logic seems a little clear. > > Signed-off-by: Wei Yang <richard.weiyang@linux.alibaba.com> > Tested-by: Dmitry Osipenko <digetx@gmail.com> > --- > mm/mremap.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/mm/mremap.c b/mm/mremap.c > index de27b12c8a5a..a30b3e86cc99 100644 > --- a/mm/mremap.c > +++ b/mm/mremap.c > @@ -258,6 +258,9 @@ unsigned long move_page_tables(struct vm_area_struct *vma, > extent = next - old_addr; > if (extent > old_end - old_addr) > extent = old_end - old_addr; > + next = (new_addr + PMD_SIZE) & PMD_MASK; Please use round_up() for both 'next' calculations. > + if (extent > next - new_addr) > + extent = next - new_addr; > old_pmd = get_old_pmd(vma->vm_mm, old_addr); > if (!old_pmd) > continue; > @@ -301,9 +304,6 @@ unsigned long move_page_tables(struct vm_area_struct *vma, > > if (pte_alloc(new_vma->vm_mm, new_pmd)) > break; > - next = (new_addr + PMD_SIZE) & PMD_MASK; > - if (extent > next - new_addr) > - extent = next - new_addr; > move_ptes(vma, old_pmd, old_addr, old_addr + extent, new_vma, > new_pmd, new_addr, need_rmap_locks); > } > -- > 2.20.1 (Apple Git-117) >
On Mon, Jul 06, 2020 at 01:07:29PM +0300, Kirill A. Shutemov wrote: >On Fri, Jun 26, 2020 at 09:52:15PM +0800, Wei Yang wrote: >> Page tables is moved on the base of PMD. This requires both source >> and destination range should meet the requirement. >> >> Current code works well since move_huge_pmd() and move_normal_pmd() >> would check old_addr and new_addr again. And then return to move_ptes() >> if the either of them is not aligned. >> >> In stead of calculating the extent separately, it is better to calculate >> in one place, so we know it is not necessary to try move pmd. By doing >> so, the logic seems a little clear. >> >> Signed-off-by: Wei Yang <richard.weiyang@linux.alibaba.com> >> Tested-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> mm/mremap.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/mm/mremap.c b/mm/mremap.c >> index de27b12c8a5a..a30b3e86cc99 100644 >> --- a/mm/mremap.c >> +++ b/mm/mremap.c >> @@ -258,6 +258,9 @@ unsigned long move_page_tables(struct vm_area_struct *vma, >> extent = next - old_addr; >> if (extent > old_end - old_addr) >> extent = old_end - old_addr; >> + next = (new_addr + PMD_SIZE) & PMD_MASK; > >Please use round_up() for both 'next' calculations. > I took another close look into this, seems this is not a good suggestion. round_up(new_addr, PMD_SIZE) would be new_addr when new_addr is PMD_SIZE aligned, which is not what we expect. >> + if (extent > next - new_addr) >> + extent = next - new_addr; >> old_pmd = get_old_pmd(vma->vm_mm, old_addr); >> if (!old_pmd) >> continue; >> @@ -301,9 +304,6 @@ unsigned long move_page_tables(struct vm_area_struct *vma, >> >> if (pte_alloc(new_vma->vm_mm, new_pmd)) >> break; >> - next = (new_addr + PMD_SIZE) & PMD_MASK; >> - if (extent > next - new_addr) >> - extent = next - new_addr; >> move_ptes(vma, old_pmd, old_addr, old_addr + extent, new_vma, >> new_pmd, new_addr, need_rmap_locks); >> } >> -- >> 2.20.1 (Apple Git-117) >> > >-- > Kirill A. Shutemov
On Tue, Jul 07, 2020 at 09:38:56AM +0800, Wei Yang wrote: > On Mon, Jul 06, 2020 at 01:07:29PM +0300, Kirill A. Shutemov wrote: > >On Fri, Jun 26, 2020 at 09:52:15PM +0800, Wei Yang wrote: > >> Page tables is moved on the base of PMD. This requires both source > >> and destination range should meet the requirement. > >> > >> Current code works well since move_huge_pmd() and move_normal_pmd() > >> would check old_addr and new_addr again. And then return to move_ptes() > >> if the either of them is not aligned. > >> > >> In stead of calculating the extent separately, it is better to calculate > >> in one place, so we know it is not necessary to try move pmd. By doing > >> so, the logic seems a little clear. > >> > >> Signed-off-by: Wei Yang <richard.weiyang@linux.alibaba.com> > >> Tested-by: Dmitry Osipenko <digetx@gmail.com> > >> --- > >> mm/mremap.c | 6 +++--- > >> 1 file changed, 3 insertions(+), 3 deletions(-) > >> > >> diff --git a/mm/mremap.c b/mm/mremap.c > >> index de27b12c8a5a..a30b3e86cc99 100644 > >> --- a/mm/mremap.c > >> +++ b/mm/mremap.c > >> @@ -258,6 +258,9 @@ unsigned long move_page_tables(struct vm_area_struct *vma, > >> extent = next - old_addr; > >> if (extent > old_end - old_addr) > >> extent = old_end - old_addr; > >> + next = (new_addr + PMD_SIZE) & PMD_MASK; > > > >Please use round_up() for both 'next' calculations. > > > > I took another close look into this, seems this is not a good suggestion. > > round_up(new_addr, PMD_SIZE) > > would be new_addr when new_addr is PMD_SIZE aligned, which is not what we > expect. Maybe round_down(new_addr + PMD_SIZE, PMD_SIZE)?
On Tue, Jul 07, 2020 at 01:47:22PM +0300, Kirill A. Shutemov wrote: >On Tue, Jul 07, 2020 at 09:38:56AM +0800, Wei Yang wrote: >> On Mon, Jul 06, 2020 at 01:07:29PM +0300, Kirill A. Shutemov wrote: >> >On Fri, Jun 26, 2020 at 09:52:15PM +0800, Wei Yang wrote: >> >> Page tables is moved on the base of PMD. This requires both source >> >> and destination range should meet the requirement. >> >> >> >> Current code works well since move_huge_pmd() and move_normal_pmd() >> >> would check old_addr and new_addr again. And then return to move_ptes() >> >> if the either of them is not aligned. >> >> >> >> In stead of calculating the extent separately, it is better to calculate >> >> in one place, so we know it is not necessary to try move pmd. By doing >> >> so, the logic seems a little clear. >> >> >> >> Signed-off-by: Wei Yang <richard.weiyang@linux.alibaba.com> >> >> Tested-by: Dmitry Osipenko <digetx@gmail.com> >> >> --- >> >> mm/mremap.c | 6 +++--- >> >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> >> >> diff --git a/mm/mremap.c b/mm/mremap.c >> >> index de27b12c8a5a..a30b3e86cc99 100644 >> >> --- a/mm/mremap.c >> >> +++ b/mm/mremap.c >> >> @@ -258,6 +258,9 @@ unsigned long move_page_tables(struct vm_area_struct *vma, >> >> extent = next - old_addr; >> >> if (extent > old_end - old_addr) >> >> extent = old_end - old_addr; >> >> + next = (new_addr + PMD_SIZE) & PMD_MASK; >> > >> >Please use round_up() for both 'next' calculations. >> > >> >> I took another close look into this, seems this is not a good suggestion. >> >> round_up(new_addr, PMD_SIZE) >> >> would be new_addr when new_addr is PMD_SIZE aligned, which is not what we >> expect. > >Maybe round_down(new_addr + PMD_SIZE, PMD_SIZE)? > To be honest, I don't like this which makes the code not that straight forward. And when you look into the definition of pxd_addr_end(), they use the format of ((addr + PXD_SIZE) & PXD_MASK) too. I have another alternative to clean up this part with the help of pmd_addr_end(). If you agree, I would like to append the following change in next version to cleanup the next/extent staff especially. Author: Wei Yang <richard.weiyang@linux.alibaba.com> Date: Tue Jul 7 17:42:49 2020 +0800 mm/mremap: use pmd_addr_end to calculate extent diff --git a/mm/mremap.c b/mm/mremap.c index f5f17d050617..76e7fdf567c3 100644 --- a/mm/mremap.c +++ b/mm/mremap.c @@ -237,11 +237,12 @@ unsigned long move_page_tables(struct vm_area_struct *vma, unsigned long new_addr, unsigned long len, bool need_rmap_locks) { - unsigned long extent, next, old_end; + unsigned long extent, old_next, new_next, old_end, new_end; struct mmu_notifier_range range; pmd_t *old_pmd, *new_pmd; old_end = old_addr + len; + new_end = new_addr + len; flush_cache_range(vma, old_addr, old_end); mmu_notifier_range_init(&range, MMU_NOTIFY_UNMAP, 0, vma, vma->vm_mm, @@ -250,14 +251,11 @@ unsigned long move_page_tables(struct vm_area_struct *vma, for (; old_addr < old_end; old_addr += extent, new_addr += extent) { cond_resched(); - next = (old_addr + PMD_SIZE) & PMD_MASK; - /* even if next overflowed, extent below will be ok */ - extent = next - old_addr; - if (extent > old_end - old_addr) - extent = old_end - old_addr; - next = (new_addr + PMD_SIZE) & PMD_MASK; - if (extent > next - new_addr) - extent = next - new_addr; + + old_next = pmd_addr_end(old_addr, old_end); + new_next = pmd_addr_end(new_addr, new_end); + extent = min((old_next - old_addr), (new_next - new_addr)); + old_pmd = get_old_pmd(vma->vm_mm, old_addr); if (!old_pmd) continue; >-- > Kirill A. Shutemov
diff --git a/mm/mremap.c b/mm/mremap.c index de27b12c8a5a..a30b3e86cc99 100644 --- a/mm/mremap.c +++ b/mm/mremap.c @@ -258,6 +258,9 @@ unsigned long move_page_tables(struct vm_area_struct *vma, extent = next - old_addr; if (extent > old_end - old_addr) extent = old_end - old_addr; + next = (new_addr + PMD_SIZE) & PMD_MASK; + if (extent > next - new_addr) + extent = next - new_addr; old_pmd = get_old_pmd(vma->vm_mm, old_addr); if (!old_pmd) continue; @@ -301,9 +304,6 @@ unsigned long move_page_tables(struct vm_area_struct *vma, if (pte_alloc(new_vma->vm_mm, new_pmd)) break; - next = (new_addr + PMD_SIZE) & PMD_MASK; - if (extent > next - new_addr) - extent = next - new_addr; move_ptes(vma, old_pmd, old_addr, old_addr + extent, new_vma, new_pmd, new_addr, need_rmap_locks); }