Re: [PATCH v2 2/2] mm: speed up mremap by 500x on large regions

[Anton Ivanov <anton.ivanov@kot-begemot.co.uk>]

[-- Attachment #1: Type: text/plain, Size: 5032 bytes --]


On 10/12/18 3:48 PM, Anton Ivanov wrote:
> On 12/10/2018 15:37, Kirill A. Shutemov wrote:
>> On Fri, Oct 12, 2018 at 03:09:49PM +0100, Anton Ivanov wrote:
>>> On 10/12/18 2:37 AM, Joel Fernandes (Google) wrote:
>>>> Android needs to mremap large regions of memory during memory 
>>>> management
>>>> related operations. The mremap system call can be really slow if 
>>>> THP is
>>>> not enabled. The bottleneck is move_page_tables, which is copying each
>>>> pte at a time, and can be really slow across a large map. Turning 
>>>> on THP
>>>> may not be a viable option, and is not for us. This patch speeds up 
>>>> the
>>>> performance for non-THP system by copying at the PMD level when 
>>>> possible.
>>>>
>>>> The speed up is three orders of magnitude. On a 1GB mremap, the mremap
>>>> completion times drops from 160-250 millesconds to 380-400 
>>>> microseconds.
>>>>
>>>> Before:
>>>> Total mremap time for 1GB data: 242321014 nanoseconds.
>>>> Total mremap time for 1GB data: 196842467 nanoseconds.
>>>> Total mremap time for 1GB data: 167051162 nanoseconds.
>>>>
>>>> After:
>>>> Total mremap time for 1GB data: 385781 nanoseconds.
>>>> Total mremap time for 1GB data: 388959 nanoseconds.
>>>> Total mremap time for 1GB data: 402813 nanoseconds.
>>>>
>>>> Incase THP is enabled, the optimization is skipped. I also flush the
>>>> tlb every time we do this optimization since I couldn't find a way to
>>>> determine if the low-level PTEs are dirty. It is seen that the cost of
>>>> doing so is not much compared the improvement, on both x86-64 and 
>>>> arm64.
>>>>
>>>> Cc: minchan@kernel.org
>>>> Cc: pantin@google.com
>>>> Cc: hughd@google.com
>>>> Cc: lokeshgidra@google.com
>>>> Cc: dancol@google.com
>>>> Cc: mhocko@kernel.org
>>>> Cc: kirill@shutemov.name
>>>> Cc: akpm@linux-foundation.org
>>>> Signed-off-by: Joel Fernandes (Google) <joel@joelfernandes.org>
>>>> ---
>>>> A A  mm/mremap.c | 62 
>>>> +++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>> A A  1 file changed, 62 insertions(+)
>>>>
>>>> diff --git a/mm/mremap.c b/mm/mremap.c
>>>> index 9e68a02a52b1..d82c485822ef 100644
>>>> --- a/mm/mremap.c
>>>> +++ b/mm/mremap.c
>>>> @@ -191,6 +191,54 @@ static void move_ptes(struct vm_area_struct 
>>>> *vma, pmd_t *old_pmd,
>>>> A A A A A A A A A A  drop_rmap_locks(vma);
>>>> A A  }
>>>> +static bool move_normal_pmd(struct vm_area_struct *vma, unsigned 
>>>> long old_addr,
>>>> +A A A A A A A A A  unsigned long new_addr, unsigned long old_end,
>>>> +A A A A A A A A A  pmd_t *old_pmd, pmd_t *new_pmd, bool *need_flush)
>>>> +{
>>>> +A A A  spinlock_t *old_ptl, *new_ptl;
>>>> +A A A  struct mm_struct *mm = vma->vm_mm;
>>>> +
>>>> +A A A  if ((old_addr & ~PMD_MASK) || (new_addr & ~PMD_MASK)
>>>> +A A A A A A A  || old_end - old_addr < PMD_SIZE)
>>>> +A A A A A A A  return false;
>>>> +
>>>> +A A A  /*
>>>> +A A A A  * The destination pmd shouldn't be established, free_pgtables()
>>>> +A A A A  * should have release it.
>>>> +A A A A  */
>>>> +A A A  if (WARN_ON(!pmd_none(*new_pmd)))
>>>> +A A A A A A A  return false;
>>>> +
>>>> +A A A  /*
>>>> +A A A A  * We don't have to worry about the ordering of src and dst
>>>> +A A A A  * ptlocks because exclusive mmap_sem prevents deadlock.
>>>> +A A A A  */
>>>> +A A A  old_ptl = pmd_lock(vma->vm_mm, old_pmd);
>>>> +A A A  if (old_ptl) {
>>>> +A A A A A A A  pmd_t pmd;
>>>> +
>>>> +A A A A A A A  new_ptl = pmd_lockptr(mm, new_pmd);
>>>> +A A A A A A A  if (new_ptl != old_ptl)
>>>> +A A A A A A A A A A A  spin_lock_nested(new_ptl, SINGLE_DEPTH_NESTING);
>>>> +
>>>> +A A A A A A A  /* Clear the pmd */
>>>> +A A A A A A A  pmd = *old_pmd;
>>>> +A A A A A A A  pmd_clear(old_pmd);
>>>> +
>>>> +A A A A A A A  VM_BUG_ON(!pmd_none(*new_pmd));
>>>> +
>>>> +A A A A A A A  /* Set the new pmd */
>>>> +A A A A A A A  set_pmd_at(mm, new_addr, new_pmd, pmd);
>>> UML does not have set_pmd_at at all
>> Every architecture does. :)
>
> I tried to build it patching vs 4.19-rc before I made this statement 
> and ran into that.
>
> Presently it does not.
>
> https://elixir.bootlin.com/linux/v4.19-rc7/ident/set_pmd_at - UML is 
> not on the list.

Once this problem as well as the omissions in the include changes for 
UML in patch one have been fixed it appears to be working.

What it needs is attached.


>
>>
>> But it may come not from the arch code.
>
> There is no generic definition as far as I can see. All 12 defines in 
> 4.19 are in arch specific code. Unless i am missing something...
>
>>
>>> If I read the code right, MIPS completely ignores the address 
>>> argument so
>>> set_pmd_at there may not have the effect which this patch is trying to
>>> achieve.
>> Ignoring address is fine. Most architectures do that..
>> The ideas is to move page table to the new pmd slot. It's nothing to do
>> with the address passed to set_pmd_at().
>
> If that is it's only function, then I am going to appropriate the code 
> out of the MIPS tree for further uml testing. It does exactly that - 
> just move the pmd the new slot.
>
>>
> A.


A.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Incremental-fixes-to-the-mmremap-patch.patch --]
[-- Type: text/x-patch; name="0001-Incremental-fixes-to-the-mmremap-patch.patch", Size: 0 bytes --]