[RESEND,v2,3/4] mm/mremap: calculate extent in one place
diff mbox series

Message ID 20200626135216.24314-4-richard.weiyang@linux.alibaba.com
State New
Headers show
Series
  • mm/mremap: cleanup move_page_tables() a little
Related show

Commit Message

Wei Yang June 26, 2020, 1:52 p.m. UTC
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(-)

Comments

Kirill A. Shutemov July 6, 2020, 10:07 a.m. UTC | #1
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)
>
Wei Yang July 7, 2020, 1:38 a.m. UTC | #2
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
Kirill A. Shutemov July 7, 2020, 10:47 a.m. UTC | #3
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)?
Wei Yang July 7, 2020, 12:53 p.m. UTC | #4
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

Patch
diff mbox series

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);
 	}