mm: protect free_pgtables with mmap_lock write lock in exit_mmap

Add comments for vm_operations_struct::close documenting locking requirements for this callback and its callers.

Signed-off-by Suren Baghdasaryan <[email protected]>

---
 include/linux/mm.h | 4 ++++
 1 file changed, 4 insertions(+)

include/linux/mm.h +4 -0

index a7e4a9e7d807..b9b88ba7564b 100644
@@ -577,6 +577,10 @@ enum page_entry_size {
  */
 struct vm_operations_struct {
 	void (*open)(struct vm_area_struct * area);
+	/**
+	 * @close: Called when the VMA is being removed from the MM.
+	 * Context: User context.  May sleep.  Caller holds mmap_lock.
+	 */
 	void (*close)(struct vm_area_struct * area);
 	/* Called any time before splitting to check if it's allowed */
 	int (*may_split)(struct vm_area_struct *area, unsigned long addr);

Signature · 1 line

2.34.1.400.ga245620fadb-goog

With exit_mmap holding mmap_write_lock during free_pgtables call, process_mrelease does not need to elevate mm->mm_users in order to prevent exit_mmap from destrying pagetables while __oom_reap_task_mm is walking the VMA tree. The change prevents process_mrelease from calling the last mmput, which can lead to waiting for IO completion in exit_aio.

Fixes 337546e83fc7 ("mm/oom_kill.c: prevent a race between process_mrelease and exit_mmap")

Signed-off-by Suren Baghdasaryan <[email protected]>

---
 mm/oom_kill.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

mm/oom_kill.c +15 -12

index 1ddabefcfb5a..67780386f478 100644
@@ -1169,15 +1169,15 @@ SYSCALL_DEFINE2(process_mrelease, int, pidfd, unsigned int, flags)
 		goto put_task;
 	}
 
-	if (mmget_not_zero(p->mm)) {
-		mm = p->mm;
-		if (task_will_free_mem(p))
-			reap = true;
-		else {
-			/* Error only if the work has not been done already */
-			if (!test_bit(MMF_OOM_SKIP, &mm->flags))
-				ret = -EINVAL;
-		}
+	mm = p->mm;
+	mmgrab(mm);
+
+	if (task_will_free_mem(p))
+		reap = true;
+	else {
+		/* Error only if the work has not been done already */
+		if (!test_bit(MMF_OOM_SKIP, &mm->flags))
+			ret = -EINVAL;
 	}
 	task_unlock(p);
 

@@ -1188,13 +1188,16 @@ SYSCALL_DEFINE2(process_mrelease, int, pidfd, unsigned int, flags)
 		ret = -EINTR;
 		goto drop_mm;
 	}
-	if (!__oom_reap_task_mm(mm))
+	/*
+	 * Check MMF_OOM_SKIP again under mmap_read_lock protection to ensure
+	 * possible change in exit_mmap is seen
+	 */
+	if (!test_bit(MMF_OOM_SKIP, &mm->flags) && !__oom_reap_task_mm(mm))
 		ret = -EAGAIN;
 	mmap_read_unlock(mm);
 
 drop_mm:
-	if (mm)
-		mmput(mm);
+	mmdrop(mm);
 put_task:
 	put_task_struct(task);
 	return ret;

Signature · 1 line

2.34.1.400.ga245620fadb-goog

Quoted 7 lines · Suren Baghdasaryan

With exit_mmap holding mmap_write_lock during free_pgtables call, process_mrelease does not need to elevate mm->mm_users in order to prevent exit_mmap from destrying pagetables while __oom_reap_task_mm is walking the VMA tree. The change prevents process_mrelease from calling the last mmput, which can lead to waiting for IO completion in exit_aio.

Quoted 1 line

Fixes 337546e83fc7 ("mm/oom_kill.c: prevent a race between process_mrelease and exit_mmap")

I am not sure I have brought this up already but I do not think Fixes tag is a good fit. 337546e83fc7 is a correct way to handle the race. It is just slightly less optimal than this fix.

Quoted 2 lines

Signed-off-by Suren Baghdasaryan <[email protected]>

Acked-by Michal Hocko <[email protected]>

Thanks!

Quoted 54 lines

---
 mm/oom_kill.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

mm/oom_kill.c +15 -13

index 1ddabefcfb5a..67780386f478 100644
@@ -1169,15 +1169,15 @@ SYSCALL_DEFINE2(process_mrelease, int, pidfd, unsigned int, flags)
              goto put_task;
      }
 
-     if (mmget_not_zero(p->mm)) {
-             mm = p->mm;
-             if (task_will_free_mem(p))
-                     reap = true;
-             else {
-                     /* Error only if the work has not been done already */
-                     if (!test_bit(MMF_OOM_SKIP, &mm->flags))
-                             ret = -EINVAL;
-             }
+     mm = p->mm;
+     mmgrab(mm);
+
+     if (task_will_free_mem(p))
+             reap = true;
+     else {
+             /* Error only if the work has not been done already */
+             if (!test_bit(MMF_OOM_SKIP, &mm->flags))
+                     ret = -EINVAL;
      }
      task_unlock(p);
 

@@ -1188,13 +1188,16 @@ SYSCALL_DEFINE2(process_mrelease, int, pidfd, unsigned int, flags)
              ret = -EINTR;
              goto drop_mm;
      }
-     if (!__oom_reap_task_mm(mm))
+     /*
+      * Check MMF_OOM_SKIP again under mmap_read_lock protection to ensure
+      * possible change in exit_mmap is seen
+      */
+     if (!test_bit(MMF_OOM_SKIP, &mm->flags) && !__oom_reap_task_mm(mm))
              ret = -EAGAIN;
      mmap_read_unlock(mm);
 
 drop_mm:
-     if (mm)
-             mmput(mm);
+     mmdrop(mm);
 put_task:
      put_task_struct(task);
      return ret;
--

Quoted 1 line

2.34.1.400.ga245620fadb-goog

--
Michal Hocko
SUSE Labs

oom-reaper and process_mrelease system call should protect against races with exit_mmap which can destroy page tables while they walk the VMA tree. oom-reaper protects from that race by setting MMF_OOM_VICTIM and by relying on exit_mmap to set MMF_OOM_SKIP before taking and releasing mmap_write_lock. process_mrelease has to elevate mm->mm_users to prevent such race. Both oom-reaper and process_mrelease hold mmap_read_lock when walking the VMA tree.

The locking rules and mechanisms could be simpler if exit_mmap takes mmap_write_lock while executing destructive operations such as free_pgtables.
Change exit_mmap to hold the mmap_write_lock when calling free_pgtables and remove_vma. Operations like unmap_vmas and unlock_range are not destructive and could run under mmap_read_lock but for simplicity we take one mmap_write_lock during almost the entire operation.

Note also that because oom-reaper checks VM_LOCKED flag, unlock_range() should not be allowed to race with it. Before this patch, remove_vma used to be called with no locks held, however with fput being executed asynchronously and vm_ops->close not being allowed to hold mmap_lock (it is called from __split_vma with mmap_sem held for write), changing that should be fine. In most cases this lock should be uncontended. Previously, Kirill reported ~4% regression caused by a similar change [1]. We reran the same test and although the individual results are quite noisy, the percentiles show lower regression with 1.6% being the worst case [2]. The change allows oom-reaper and process_mrelease to execute safely under mmap_read_lock without worries that exit_mmap might destroy page tables from under them.

[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/CAJuCfpGC9-c9P40x7oy=jy5SphMcd0o0G_6U1-+JAziGKG6dGA@mail.gmail.com/

Signed-off-by Suren Baghdasaryan <[email protected]>

---
changes in v4
- Separated comments describing vm_operations_struct::close locking requirements into a separate patch, per Matthew Wilcox

 mm/mmap.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

mm/mmap.c +8 -8

index bfb0ea164a90..f4e09d390a07 100644
@@ -3142,25 +3142,27 @@ void exit_mmap(struct mm_struct *mm)
 		 * to mmu_notifier_release(mm) ensures mmu notifier callbacks in
 		 * __oom_reap_task_mm() will not block.
 		 *
-		 * This needs to be done before calling munlock_vma_pages_all(),
+		 * This needs to be done before calling unlock_range(),
 		 * which clears VM_LOCKED, otherwise the oom reaper cannot
 		 * reliably test it.
 		 */
 		(void)__oom_reap_task_mm(mm);
 
 		set_bit(MMF_OOM_SKIP, &mm->flags);
-		mmap_write_lock(mm);
-		mmap_write_unlock(mm);
 	}
 
+	mmap_write_lock(mm);
 	if (mm->locked_vm)
 		unlock_range(mm->mmap, ULONG_MAX);
 
 	arch_exit_mmap(mm);
 
 	vma = mm->mmap;
-	if (!vma)	/* Can happen if dup_mmap() received an OOM */
+	if (!vma) {
+		/* Can happen if dup_mmap() received an OOM */
+		mmap_write_unlock(mm);
 		return;
+	}
 
 	lru_add_drain();
 	flush_cache_mm(mm);

@@ -3171,16 +3173,14 @@ void exit_mmap(struct mm_struct *mm)
 	free_pgtables(&tlb, vma, FIRST_USER_ADDRESS, USER_PGTABLES_CEILING);
 	tlb_finish_mmu(&tlb);
 
-	/*
-	 * Walk the list again, actually closing and freeing it,
-	 * with preemption enabled, without holding any MM locks.
-	 */
+	/* Walk the list again, actually closing and freeing it. */
 	while (vma) {
 		if (vma->vm_flags & VM_ACCOUNT)
 			nr_accounted += vma_pages(vma);
 		vma = remove_vma(vma);
 		cond_resched();
 	}
+	mmap_write_unlock(mm);
 	vm_unacct_memory(nr_accounted);
 }
 

Signature · 1 line

2.34.1.400.ga245620fadb-goog

Quoted 7 lines · Suren Baghdasaryan

oom-reaper and process_mrelease system call should protect against races with exit_mmap which can destroy page tables while they walk the VMA tree. oom-reaper protects from that race by setting MMF_OOM_VICTIM and by relying on exit_mmap to set MMF_OOM_SKIP before taking and releasing mmap_write_lock. process_mrelease has to elevate mm->mm_users to prevent such race. Both oom-reaper and process_mrelease hold mmap_read_lock when walking the VMA tree.

Quoted 8 lines

The locking rules and mechanisms could be simpler if exit_mmap takes mmap_write_lock while executing destructive operations such as free_pgtables.
Change exit_mmap to hold the mmap_write_lock when calling free_pgtables and remove_vma. Operations like unmap_vmas and unlock_range are not destructive and could run under mmap_read_lock but for simplicity we take one mmap_write_lock during almost the entire operation.

unlock_range is not safe to be called under read lock. See 27ae357fa82b ("mm, oom: fix concurrent munlock and oom reaper unmap, v3").

Quoted 18 lines

Note also that because oom-reaper checks VM_LOCKED flag, unlock_range() should not be allowed to race with it. Before this patch, remove_vma used to be called with no locks held, however with fput being executed asynchronously and vm_ops->close not being allowed to hold mmap_lock (it is called from __split_vma with mmap_sem held for write), changing that should be fine. In most cases this lock should be uncontended. Previously, Kirill reported ~4% regression caused by a similar change [1]. We reran the same test and although the individual results are quite noisy, the percentiles show lower regression with 1.6% being the worst case [2]. The change allows oom-reaper and process_mrelease to execute safely under mmap_read_lock without worries that exit_mmap might destroy page tables from under them.

[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/CAJuCfpGC9-c9P40x7oy=jy5SphMcd0o0G_6U1-+JAziGKG6dGA@mail.gmail.com/

Quoted 1 line

Signed-off-by Suren Baghdasaryan <[email protected]>

The patch looks good otherwise. Btw. when I was trying to do something similar in the past Hugh has noted that we can get rid of the same lock&&unlock trick in ksm. Maybe you want to have a look at that as well ;)

Acked-by Michal Hocko <[email protected]>

Thanks!

Quoted 64 lines

---
changes in v4
- Separated comments describing vm_operations_struct::close locking requirements into a separate patch, per Matthew Wilcox

 mm/mmap.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

mm/mmap.c +8 -9

index bfb0ea164a90..f4e09d390a07 100644
@@ -3142,25 +3142,27 @@ void exit_mmap(struct mm_struct *mm)
               * to mmu_notifier_release(mm) ensures mmu notifier callbacks in
               * __oom_reap_task_mm() will not block.
               *
-              * This needs to be done before calling munlock_vma_pages_all(),
+              * This needs to be done before calling unlock_range(),
               * which clears VM_LOCKED, otherwise the oom reaper cannot
               * reliably test it.
               */
              (void)__oom_reap_task_mm(mm);
 
              set_bit(MMF_OOM_SKIP, &mm->flags);
-             mmap_write_lock(mm);
-             mmap_write_unlock(mm);
      }
 
+     mmap_write_lock(mm);
      if (mm->locked_vm)
              unlock_range(mm->mmap, ULONG_MAX);
 
      arch_exit_mmap(mm);
 
      vma = mm->mmap;
-     if (!vma)       /* Can happen if dup_mmap() received an OOM */
+     if (!vma) {
+             /* Can happen if dup_mmap() received an OOM */
+             mmap_write_unlock(mm);
              return;
+     }
 
      lru_add_drain();
      flush_cache_mm(mm);

@@ -3171,16 +3173,14 @@ void exit_mmap(struct mm_struct *mm)
      free_pgtables(&tlb, vma, FIRST_USER_ADDRESS, USER_PGTABLES_CEILING);
      tlb_finish_mmu(&tlb);
 
-     /*
-      * Walk the list again, actually closing and freeing it,
-      * with preemption enabled, without holding any MM locks.
-      */
+     /* Walk the list again, actually closing and freeing it. */
      while (vma) {
              if (vma->vm_flags & VM_ACCOUNT)
                      nr_accounted += vma_pages(vma);
              vma = remove_vma(vma);
              cond_resched();
      }
+     mmap_write_unlock(mm);
      vm_unacct_memory(nr_accounted);
 }
 
--

Quoted 1 line

2.34.1.400.ga245620fadb-goog

--
Michal Hocko
SUSE Labs

Quoted 18 lines · Michal Hocko

Quoted 15 lines · Suren Baghdasaryan

oom-reaper and process_mrelease system call should protect against races with exit_mmap which can destroy page tables while they walk the VMA tree. oom-reaper protects from that race by setting MMF_OOM_VICTIM and by relying on exit_mmap to set MMF_OOM_SKIP before taking and releasing mmap_write_lock. process_mrelease has to elevate mm->mm_users to prevent such race. Both oom-reaper and process_mrelease hold mmap_read_lock when walking the VMA tree. The locking rules and mechanisms could be simpler if exit_mmap takes mmap_write_lock while executing destructive operations such as free_pgtables.
Change exit_mmap to hold the mmap_write_lock when calling free_pgtables and remove_vma. Operations like unmap_vmas and unlock_range are not destructive and could run under mmap_read_lock but for simplicity we take one mmap_write_lock during almost the entire operation.

Quoted 2 lines

unlock_range is not safe to be called under read lock. See 27ae357fa82b ("mm, oom: fix concurrent munlock and oom reaper unmap, v3").

Ok, I'll remove the sentence above.
Is my understanding correct that it is unsafe only because oom-reaper can't deal with VM_LOCKED, otherwise it would be fine?

Do we want this on top?

----
From 58b04ae6dc97b0105ea2651daca55cf2386f69b4 Mon Sep 17 00:00:00 2001
From: Michal Hocko <[email protected]>
Date: Thu, 9 Dec 2021 10:07:51 +0100
Subject: [PATCH] mm: drop MMF_OOM_SKIP from exit_mmap

MMF_OOM_SKIP used to play a synchronization role between exit_mmap and oom repear in the past. Since the exclusive mmap_sem is held in exit_mmap to cover all destructive operations the flag synchronization is not needed anymore and we can safely drop it. Just make sure that mm->mmap is set to NULL so that nobody will access the freed vma list.

Signed-off-by Michal Hocko <[email protected]>

---
 mm/mmap.c | 23 +----------------------
 1 file changed, 1 insertion(+), 22 deletions(-)

mm/mmap.c +1 -23

index f4e09d390a07..0d6af9d89aa8 100644
@@ -3129,28 +3129,6 @@ void exit_mmap(struct mm_struct *mm)
        /* mm's last user has gone, and its about to be pulled down */
        mmu_notifier_release(mm);
 
-       if (unlikely(mm_is_oom_victim(mm))) {
-               /*
-                * Manually reap the mm to free as much memory as possible.
-                * Then, as the oom reaper does, set MMF_OOM_SKIP to disregard
-                * this mm from further consideration.  Taking mm->mmap_lock for
-                * write after setting MMF_OOM_SKIP will guarantee that the oom
-                * reaper will not run on this mm again after mmap_lock is
-                * dropped.
-                *
-                * Nothing can be holding mm->mmap_lock here and the above call
-                * to mmu_notifier_release(mm) ensures mmu notifier callbacks in
-                * __oom_reap_task_mm() will not block.
-                *
-                * This needs to be done before calling unlock_range(),
-                * which clears VM_LOCKED, otherwise the oom reaper cannot
-                * reliably test it.
-                */
-               (void)__oom_reap_task_mm(mm);
-
-               set_bit(MMF_OOM_SKIP, &mm->flags);
-       }
-
        mmap_write_lock(mm);
        if (mm->locked_vm)
                unlock_range(mm->mmap, ULONG_MAX);

@@ -3180,6 +3158,7 @@ void exit_mmap(struct mm_struct *mm)
                vma = remove_vma(vma);
                cond_resched();
        }
+       mm->mmap = NULL;
        mmap_write_unlock(mm);
        vm_unacct_memory(nr_accounted);
 }
--

2.30.2

--
Michal Hocko
SUSE Labs

Quoted 2 lines · Michal Hocko

Do we want this on top?

As we discussed in this thread
https://lore.kernel.org/all/[email protected],
__oom_reap_task_mm in exit_mmap allows oom-reaper/process_mrelease to unmap pages in parallel with exit_mmap without blocking each other. Removal of __oom_reap_task_mm from exit_mmap prevents this parallelism and has a negative impact on performance. So the conclusion of that thread I thought was to keep that part. My understanding is that we also wanted to remove MMF_OOM_SKIP as a follow-up patch but __oom_reap_task_mm would stay.

Quoted 4 lines · Suren Baghdasaryan

Quoted 2 lines · Michal Hocko

Do we want this on top?

Quoted 9 lines

As we discussed in this thread
https://lore.kernel.org/all/[email protected],
__oom_reap_task_mm in exit_mmap allows oom-reaper/process_mrelease to unmap pages in parallel with exit_mmap without blocking each other. Removal of __oom_reap_task_mm from exit_mmap prevents this parallelism and has a negative impact on performance. So the conclusion of that thread I thought was to keep that part. My understanding is that we also wanted to remove MMF_OOM_SKIP as a follow-up patch but __oom_reap_task_mm would stay.

OK, then we were talking past each other, I am afraid. I really wanted to get rid of this oom specific stuff from exit_mmap. It was there out of necessity. With a proper locking we can finally get rid of the crud. As I've said previously oom reaping has never been a hot path.

If we really want to optimize this path then I would much rather see a generic solution which would allow to move the write lock down after unmap_vmas. That would require oom reaper to be able to handle mlocked memory.

--
Michal Hocko
SUSE Labs

Quoted 21 lines · Michal Hocko

Quoted 4 lines · Suren Baghdasaryan

Quoted 2 lines · Michal Hocko

Do we want this on top?

Quoted 9 lines

As we discussed in this thread
https://lore.kernel.org/all/[email protected],
__oom_reap_task_mm in exit_mmap allows oom-reaper/process_mrelease to unmap pages in parallel with exit_mmap without blocking each other. Removal of __oom_reap_task_mm from exit_mmap prevents this parallelism and has a negative impact on performance. So the conclusion of that thread I thought was to keep that part. My understanding is that we also wanted to remove MMF_OOM_SKIP as a follow-up patch but __oom_reap_task_mm would stay.

OK, then we were talking past each other, I am afraid. I really wanted to get rid of this oom specific stuff from exit_mmap. It was there out of necessity. With a proper locking we can finally get rid of the crud. As I've said previously oom reaping has never been a hot path.

Quoted 4 lines

If we really want to optimize this path then I would much rather see a generic solution which would allow to move the write lock down after unmap_vmas. That would require oom reaper to be able to handle mlocked memory.

Ok, let's work on that and when that's done we can get rid of the oom stuff in exit_mmap. I'll look into this over the weekend and will likely be back with questions.

Thanks!

Quoted 3 lines

--
Michal Hocko
SUSE Labs

Quoted 23 lines · Suren Baghdasaryan

Quoted 21 lines · Michal Hocko

Quoted 4 lines · Suren Baghdasaryan

Quoted 2 lines · Michal Hocko

Do we want this on top?

Quoted 9 lines

As we discussed in this thread
https://lore.kernel.org/all/[email protected],
__oom_reap_task_mm in exit_mmap allows oom-reaper/process_mrelease to unmap pages in parallel with exit_mmap without blocking each other. Removal of __oom_reap_task_mm from exit_mmap prevents this parallelism and has a negative impact on performance. So the conclusion of that thread I thought was to keep that part. My understanding is that we also wanted to remove MMF_OOM_SKIP as a follow-up patch but __oom_reap_task_mm would stay.

OK, then we were talking past each other, I am afraid. I really wanted to get rid of this oom specific stuff from exit_mmap. It was there out of necessity. With a proper locking we can finally get rid of the crud. As I've said previously oom reaping has never been a hot path.

Quoted 8 lines

Quoted 4 lines

If we really want to optimize this path then I would much rather see a generic solution which would allow to move the write lock down after unmap_vmas. That would require oom reaper to be able to handle mlocked memory.

Ok, let's work on that and when that's done we can get rid of the oom stuff in exit_mmap. I'll look into this over the weekend and will likely be back with questions.

As promised, I have a question:
Any particular reason why munlock_vma_pages_range clears VM_LOCKED before unlocking pages and not after (see:
https://elixir.bootlin.com/linux/latest/source/mm/mlock.c#L424)? Seems to me if VM_LOCKED was reset at the end (with proper ordering) then __oom_reap_task_mm would correctly skip VM_LOCKED vmas.
https://lore.kernel.org/lkml/[email protected]/
has this explanation:

"Since munlock_vma_pages_range() depends on clearing VM_LOCKED from vm_flags before actually doing the munlock to determine if any other vmas are locking the same memory, the check for VM_LOCKED in the oom reaper is racy."

but "to determine if any other vmas are locking the same memory" explanation eludes me... Any insights?

Quoted 42 lines · Suren Baghdasaryan

Quoted 23 lines · Suren Baghdasaryan

Quoted 21 lines · Michal Hocko

Quoted 4 lines · Suren Baghdasaryan

Quoted 2 lines · Michal Hocko

Do we want this on top?

Quoted 9 lines

As we discussed in this thread
https://lore.kernel.org/all/[email protected],
__oom_reap_task_mm in exit_mmap allows oom-reaper/process_mrelease to unmap pages in parallel with exit_mmap without blocking each other. Removal of __oom_reap_task_mm from exit_mmap prevents this parallelism and has a negative impact on performance. So the conclusion of that thread I thought was to keep that part. My understanding is that we also wanted to remove MMF_OOM_SKIP as a follow-up patch but __oom_reap_task_mm would stay.

OK, then we were talking past each other, I am afraid. I really wanted to get rid of this oom specific stuff from exit_mmap. It was there out of necessity. With a proper locking we can finally get rid of the crud. As I've said previously oom reaping has never been a hot path.

Quoted 8 lines

Quoted 4 lines

If we really want to optimize this path then I would much rather see a generic solution which would allow to move the write lock down after unmap_vmas. That would require oom reaper to be able to handle mlocked memory.

Ok, let's work on that and when that's done we can get rid of the oom stuff in exit_mmap. I'll look into this over the weekend and will likely be back with questions.

As promised, I have a question:
Any particular reason why munlock_vma_pages_range clears VM_LOCKED before unlocking pages and not after (see:
https://elixir.bootlin.com/linux/latest/source/mm/mlock.c#L424)? Seems to me if VM_LOCKED was reset at the end (with proper ordering) then __oom_reap_task_mm would correctly skip VM_LOCKED vmas.
https://lore.kernel.org/lkml/[email protected]/
has this explanation:

Quoted 7 lines

"Since munlock_vma_pages_range() depends on clearing VM_LOCKED from vm_flags before actually doing the munlock to determine if any other vmas are locking the same memory, the check for VM_LOCKED in the oom reaper is racy."

but "to determine if any other vmas are locking the same memory" explanation eludes me... Any insights?

A page's mlock state is determined by whether any of the vmas that map it are mlocked. The munlock code does:

vma->vm_flags &= VM_LOCKED_CLEAR_MASK
TestClearPageMlocked()
isolate_lru_page()
__munlock_isolated_page()
page_mlock()

    rmap_walk() # for_each_vma()
      page_mlock_one()
        (vma->vm_flags & VM_LOCKED) && TestSetPageMlocked()

If we didn't clear the VM_LOCKED flag first, racing threads could re-lock pages under us because they see that flag and think our vma wants those pages mlocked when we're in the process of munlocking.

Quoted 64 lines · Johannes Weiner

Quoted 42 lines · Suren Baghdasaryan

Quoted 23 lines · Suren Baghdasaryan

Quoted 21 lines · Michal Hocko

Quoted 4 lines · Suren Baghdasaryan

Quoted 2 lines · Michal Hocko

Do we want this on top?

Quoted 9 lines

As we discussed in this thread
https://lore.kernel.org/all/[email protected],
__oom_reap_task_mm in exit_mmap allows oom-reaper/process_mrelease to unmap pages in parallel with exit_mmap without blocking each other. Removal of __oom_reap_task_mm from exit_mmap prevents this parallelism and has a negative impact on performance. So the conclusion of that thread I thought was to keep that part. My understanding is that we also wanted to remove MMF_OOM_SKIP as a follow-up patch but __oom_reap_task_mm would stay.

OK, then we were talking past each other, I am afraid. I really wanted to get rid of this oom specific stuff from exit_mmap. It was there out of necessity. With a proper locking we can finally get rid of the crud. As I've said previously oom reaping has never been a hot path.

Quoted 8 lines

Quoted 4 lines

If we really want to optimize this path then I would much rather see a generic solution which would allow to move the write lock down after unmap_vmas. That would require oom reaper to be able to handle mlocked memory.

Ok, let's work on that and when that's done we can get rid of the oom stuff in exit_mmap. I'll look into this over the weekend and will likely be back with questions.

As promised, I have a question:
Any particular reason why munlock_vma_pages_range clears VM_LOCKED before unlocking pages and not after (see:
https://elixir.bootlin.com/linux/latest/source/mm/mlock.c#L424)? Seems to me if VM_LOCKED was reset at the end (with proper ordering) then __oom_reap_task_mm would correctly skip VM_LOCKED vmas.
https://lore.kernel.org/lkml/[email protected]/
has this explanation:

Quoted 7 lines

"Since munlock_vma_pages_range() depends on clearing VM_LOCKED from vm_flags before actually doing the munlock to determine if any other vmas are locking the same memory, the check for VM_LOCKED in the oom reaper is racy."

but "to determine if any other vmas are locking the same memory" explanation eludes me... Any insights?

A page's mlock state is determined by whether any of the vmas that map it are mlocked. The munlock code does:

vma->vm_flags &= VM_LOCKED_CLEAR_MASK
TestClearPageMlocked()
isolate_lru_page()
__munlock_isolated_page()
page_mlock()

    rmap_walk() # for_each_vma()
      page_mlock_one()
        (vma->vm_flags & VM_LOCKED) && TestSetPageMlocked()

Quoted 3 lines

If we didn't clear the VM_LOCKED flag first, racing threads could re-lock pages under us because they see that flag and think our vma wants those pages mlocked when we're in the process of munlocking.

Thanks for the explanation Johannes!
So far I didn't find an easy way to let __oom_reap_task_mm() run concurrently with unlock_range(). Will keep exploring.

Quoted 1 line

After some more digging I think there are two acceptable options:

1. Call unlock_range() under mmap_write_lock and then downgrade it to read lock so that both exit_mmap() and __oom_reap_task_mm() can unmap vmas in parallel like this:

    if (mm->locked_vm) {
        mmap_write_lock(mm);
        unlock_range(mm->mmap, ULONG_MAX);
        mmap_write_downgrade(mm);
    } else
        mmap_read_lock(mm);
...
    unmap_vmas(&tlb, vma, 0, -1);
    mmap_read_unlock(mm);
    mmap_write_lock(mm);
    free_pgtables(&tlb, vma, FIRST_USER_ADDRESS, USER_PGTABLES_CEILING);
...
    mm->mmap = NULL;
    mmap_write_unlock(mm);

This way exit_mmap() might block __oom_reap_task_mm() but for a much shorter time during unlock_range() call.

2. Introduce another vm_flag mask similar to VM_LOCKED which is set before munlock_vma_pages_range() clears VM_LOCKED so that __oom_reap_task_mm() can identify vmas being unlocked and skip them.

Option 1 seems cleaner to me because it keeps the locking pattern around unlock_range() in exit_mmap() consistent with all other places it is used (in mremap() and munmap()) with mmap_write_lock taken. WDYT?

On Wed 29-12-21 21:59:55, Suren Baghdasaryan wrote:
[...]

Quoted 21 lines

After some more digging I think there are two acceptable options:

1. Call unlock_range() under mmap_write_lock and then downgrade it to read lock so that both exit_mmap() and __oom_reap_task_mm() can unmap vmas in parallel like this:

    if (mm->locked_vm) {
        mmap_write_lock(mm);
        unlock_range(mm->mmap, ULONG_MAX);
        mmap_write_downgrade(mm);
    } else
        mmap_read_lock(mm);
...
    unmap_vmas(&tlb, vma, 0, -1);
    mmap_read_unlock(mm);
    mmap_write_lock(mm);
    free_pgtables(&tlb, vma, FIRST_USER_ADDRESS, USER_PGTABLES_CEILING);
...
    mm->mmap = NULL;
    mmap_write_unlock(mm);

Quoted 2 lines

This way exit_mmap() might block __oom_reap_task_mm() but for a much shorter time during unlock_range() call.

IIRC unlock_range depends on page lock at some stage and that can mean this will block for a long time or for ever when the holder of the lock depends on a memory allocation. This was the primary problem why the oom reaper skips over mlocked vmas.

Quoted 9 lines

2. Introduce another vm_flag mask similar to VM_LOCKED which is set before munlock_vma_pages_range() clears VM_LOCKED so that __oom_reap_task_mm() can identify vmas being unlocked and skip them.

Option 1 seems cleaner to me because it keeps the locking pattern around unlock_range() in exit_mmap() consistent with all other places it is used (in mremap() and munmap()) with mmap_write_lock taken. WDYT?

It would be really great to make unlock_range oom reaper aware IMHO.

You do not quote your change in the full length so it is not really clear whether you are planning to drop __oom_reap_task_mm from exit_mmap as well.

If yes then 1) could push oom reaper to timeout while the unlock_range could be dropped on something so that wouldn't be an improvement. 2) sounds like a workaround to me as it doesn't really address the underlying problem.

I have to say that I am not really a great fan of __oom_reap_task_mm in exit_mmap but I would rather see it in place than making the surrounding code more complex/tricky.

--
Michal Hocko
SUSE Labs

Quoted 24 lines · Michal Hocko

On Wed 29-12-21 21:59:55, Suren Baghdasaryan wrote:
[...]

Quoted 21 lines

After some more digging I think there are two acceptable options:

1. Call unlock_range() under mmap_write_lock and then downgrade it to read lock so that both exit_mmap() and __oom_reap_task_mm() can unmap vmas in parallel like this:

    if (mm->locked_vm) {
        mmap_write_lock(mm);
        unlock_range(mm->mmap, ULONG_MAX);
        mmap_write_downgrade(mm);
    } else
        mmap_read_lock(mm);
...
    unmap_vmas(&tlb, vma, 0, -1);
    mmap_read_unlock(mm);
    mmap_write_lock(mm);
    free_pgtables(&tlb, vma, FIRST_USER_ADDRESS, USER_PGTABLES_CEILING);
...
    mm->mmap = NULL;
    mmap_write_unlock(mm);

Quoted 7 lines

Quoted 2 lines

This way exit_mmap() might block __oom_reap_task_mm() but for a much shorter time during unlock_range() call.

IIRC unlock_range depends on page lock at some stage and that can mean this will block for a long time or for ever when the holder of the lock depends on a memory allocation. This was the primary problem why the oom reaper skips over mlocked vmas.

Oh, I missed that detail. I thought __oom_reap_task_mm() skips locked vmas only to avoid destroying pgds from under follow_page().

Quoted 10 lines

Quoted 9 lines

2. Introduce another vm_flag mask similar to VM_LOCKED which is set before munlock_vma_pages_range() clears VM_LOCKED so that __oom_reap_task_mm() can identify vmas being unlocked and skip them.

Option 1 seems cleaner to me because it keeps the locking pattern around unlock_range() in exit_mmap() consistent with all other places it is used (in mremap() and munmap()) with mmap_write_lock taken. WDYT?

Quoted 1 line

It would be really great to make unlock_range oom reaper aware IMHO.

What exactly do you envision? Say unlock_range() knows that it's racing with __oom_reap_task_mm() and that calling follow_page() is unsafe without locking, what should it do?

Quoted 7 lines · Suren Baghdasaryan

Quoted 2 lines · Michal Hocko

It would be really great to make unlock_range oom reaper aware IMHO.

What exactly do you envision? Say unlock_range() knows that it's racing with __oom_reap_task_mm() and that calling follow_page() is unsafe without locking, what should it do?

My original plan was to make the page lock conditional and use trylocking from the oom reaper (aka lockless context). It is OK to simply bail out and leave some mlocked memory behind if there is a contention on a specific page. The overall objective is to free as much memory as possible, not all of it.

IIRC Hugh was not a fan of this approach and he has mentioned that the lock might not be even really needed and that the area would benefit from a clean up rather than oom reaper specific hacks. I do tend to agree with that. I just never managed to find any spare time for that though and heavily mlocked oom victims tend to be really rare.

Quoted 15 lines · Michal Hocko

Quoted 7 lines · Suren Baghdasaryan

Quoted 2 lines · Michal Hocko

It would be really great to make unlock_range oom reaper aware IMHO.

What exactly do you envision? Say unlock_range() knows that it's racing with __oom_reap_task_mm() and that calling follow_page() is unsafe without locking, what should it do?

My original plan was to make the page lock conditional and use trylocking from the oom reaper (aka lockless context). It is OK to simply bail out and leave some mlocked memory behind if there is a contention on a specific page. The overall objective is to free as much memory as possible, not all of it.

Quoted 5 lines

IIRC Hugh was not a fan of this approach and he has mentioned that the lock might not be even really needed and that the area would benefit from a clean up rather than oom reaper specific hacks. I do tend to agree with that. I just never managed to find any spare time for that though and heavily mlocked oom victims tend to be really rare.

I forget when that was, and what I had in mind at that time. But yes, by now I am very sure that munlocking needs a cleanup.

And I do have that cleanup (against a much older tree), but never the time to rebase or post or shepherd it through N revisions.

It was 22 files changed, 464 insertions, 706 deletions:
which is too much to help with this immediate oom reaper question.

I'd better not drive this discussion further off-course; but it pains me to see munlock_vma_pages obstructing, knowing there's a better way.

I wonder: what if I were to steal time and promise to post a rebased series against 5.17-rc1 or rc2: not support it thereafter, but might there be someone to pick it up and shepherd it through? But there's no answer to that, without you seeing what it's like.

Hugh