Re: [RESEND Patch v2 3/4] mm/mremap: calculate extent in one place

[Wei Yang]

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@xxxxxxxxxxxxxxxxx>
>> >> Tested-by: Dmitry Osipenko <digetx@xxxxxxxxx>
>> >> ---
>> >>  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@xxxxxxxxxxxxxxxxx>
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

-- 
Wei Yang
Help you, Help me