Skip to contentSkip to conversation

mm, oom: remove sleep from under oom_lock

PATCH In review Michal Hocko 2 participants 6 messages Jul 9, 2018 – Jul 11, 2018

Subsystems (2)
Memory management Memory management - OOM killer
lore
Share & tools
lore

Michal Hocko

author
#0 lore raw

PATCH 1/1 mm, oom: remove sleep from under oom_lock

From: Michal Hocko <[email protected]>

Tetsuo has pointed out that since 27ae357fa82b ("mm, oom: fix concurrent munlock and oom reaper unmap, v3") we have a strong synchronization between the oom_killer and victim's exiting because both have to take the oom_lock. Therefore the original heuristic to sleep for a short time in out_of_memory doesn't serve the original purpose.

Moreover Tetsuo has noticed that the short sleep can be more harmful than actually useful. Hammering the system with many processes can lead to a starvation when the task holding the oom_lock can block for a long time (minutes) and block any further progress because the oom_reaper depends on the oom_lock as well.

Drop the short sleep from out_of_memory when we hold the lock. Keep the sleep when the trylock fails to throttle the concurrent OOM paths a bit. This should be solved in a more reasonable way (e.g. sleep proportional to the time spent in the active reclaiming etc.) but this is much more complex thing to achieve. This is a quick fixup to remove a stale code.

Reported-by Tetsuo Handa <[email protected]>
Signed-off-by Michal Hocko <[email protected]>
---
 mm/oom_kill.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)
mm/oom_kill.c +1 -7
index 8ba6cb88cf58..ed9d473c571e 100644
@@ -1077,15 +1077,9 @@ bool out_of_memory(struct oom_control *oc)
 		dump_header(oc, NULL);
 		panic("Out of memory and no killable processes...\n");
 	}
-	if (oc->chosen && oc->chosen != (void *)-1UL) {
+	if (oc->chosen && oc->chosen != (void *)-1UL)
 		oom_kill_process(oc, !is_memcg_oom(oc) ? "Out of memory" :
 				 "Memory cgroup out of memory");
-		/*
-		 * Give the killed process a good chance to exit before trying
-		 * to allocate memory again.
-		 */
-		schedule_timeout_killable(1);
-	}
 	return !!oc->chosen;
 }
Signature · 1 line
2.18.0
reviewer
#1 lore raw
Quoted 14 lines · Michal Hocko

From: Michal Hocko <[email protected]>

Tetsuo has pointed out that since 27ae357fa82b ("mm, oom: fix concurrent munlock and oom reaper unmap, v3") we have a strong synchronization between the oom_killer and victim's exiting because both have to take the oom_lock. Therefore the original heuristic to sleep for a short time in out_of_memory doesn't serve the original purpose.

Moreover Tetsuo has noticed that the short sleep can be more harmful than actually useful. Hammering the system with many processes can lead to a starvation when the task holding the oom_lock can block for a long time (minutes) and block any further progress because the oom_reaper depends on the oom_lock as well.

Quoted 8 lines

Drop the short sleep from out_of_memory when we hold the lock. Keep the sleep when the trylock fails to throttle the concurrent OOM paths a bit. This should be solved in a more reasonable way (e.g. sleep proportional to the time spent in the active reclaiming etc.) but this is much more complex thing to achieve. This is a quick fixup to remove a stale code.

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

This reminds me:

mm/oom_kill.c

54) int sysctl_oom_dump_tasks = 1;
55)
56) DEFINE_MUTEX(oom_lock);
57)
58) #ifdef CONFIG_NUMA

Would you mind documenting oom_lock to specify what it's protecting?

author
#2 lore raw
Quoted 29 lines · David Rientjes
Quoted 14 lines · Michal Hocko

From: Michal Hocko <[email protected]>

Tetsuo has pointed out that since 27ae357fa82b ("mm, oom: fix concurrent munlock and oom reaper unmap, v3") we have a strong synchronization between the oom_killer and victim's exiting because both have to take the oom_lock. Therefore the original heuristic to sleep for a short time in out_of_memory doesn't serve the original purpose.

Moreover Tetsuo has noticed that the short sleep can be more harmful than actually useful. Hammering the system with many processes can lead to a starvation when the task holding the oom_lock can block for a long time (minutes) and block any further progress because the oom_reaper depends on the oom_lock as well.

Quoted 8 lines

Drop the short sleep from out_of_memory when we hold the lock. Keep the sleep when the trylock fails to throttle the concurrent OOM paths a bit. This should be solved in a more reasonable way (e.g. sleep proportional to the time spent in the active reclaiming etc.) but this is much more complex thing to achieve. This is a quick fixup to remove a stale code.

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

This reminds me:

mm/oom_kill.c

Quoted 7 lines

54) int sysctl_oom_dump_tasks = 1;
55)
56) DEFINE_MUTEX(oom_lock);
57)
58) #ifdef CONFIG_NUMA

Would you mind documenting oom_lock to specify what it's protecting?

What do you think about the following?

mm/oom_kill.c +8 -0
index ed9d473c571e..32e6f7becb40 100644
@@ -53,6 +53,14 @@ int sysctl_panic_on_oom;
 int sysctl_oom_kill_allocating_task;
 int sysctl_oom_dump_tasks = 1;
 
+/*
+ * Serializes oom killer invocations (out_of_memory()) from all contexts to
+ * prevent from over eager oom killing (e.g. when the oom killer is invoked
+ * from different domains).
+ *
+ * oom_killer_disable() relies on this lock to stabilize oom_killer_disabled
+ * and mark_oom_victim
+ */
 DEFINE_MUTEX(oom_lock);
 
 #ifdef CONFIG_NUMA
Signature · 2 lines
Michal Hocko
SUSE Labs
reviewer
#3 lore raw
Quoted 11 lines · Michal Hocko

What do you think about the following?

mm/oom_kill.c +1 -0
index ed9d473c571e..32e6f7becb40 100644
@@ -53,6 +53,14 @@ int sysctl_panic_on_oom;
 int sysctl_oom_kill_allocating_task;
 int sysctl_oom_dump_tasks = 1;
 
+/*

mm/oom_kill.c int sysctl_panic_on_oom; · line 66

Quoted 10 lines
mm/oom_kill.c +7 -0
+ * Serializes oom killer invocations (out_of_memory()) from all contexts to
+ * prevent from over eager oom killing (e.g. when the oom killer is invoked
+ * from different domains).
+ *
+ * oom_killer_disable() relies on this lock to stabilize oom_killer_disabled
+ * and mark_oom_victim
+ */
 DEFINE_MUTEX(oom_lock);
 
 #ifdef CONFIG_NUMA

I think it's better, thanks. However, does it address the question about why __oom_reap_task_mm() needs oom_lock protection? Perhaps it would be helpful to mention synchronization between reaping triggered from oom_reaper and by exit_mmap().

reviewer
#4 lore raw
Quoted 5 lines · David Rientjes

I think it's better, thanks. However, does it address the question about why __oom_reap_task_mm() needs oom_lock protection? Perhaps it would be helpful to mention synchronization between reaping triggered from oom_reaper and by exit_mmap().

Actually, can't we remove the need to take oom_lock in exit_mmap() if __oom_reap_task_mm() can do a test and set on MMF_UNSTABLE and, if already set, bail out immediately?

author
#5 lore raw
Quoted 11 lines · David Rientjes
Quoted 5 lines · David Rientjes

I think it's better, thanks. However, does it address the question about why __oom_reap_task_mm() needs oom_lock protection? Perhaps it would be helpful to mention synchronization between reaping triggered from oom_reaper and by exit_mmap().

Actually, can't we remove the need to take oom_lock in exit_mmap() if __oom_reap_task_mm() can do a test and set on MMF_UNSTABLE and, if already set, bail out immediately?

I think we do not really depend on oom_lock anymore in __oom_reap_task_mm. The race it was original added for (mmget_not_zero vs. exit path) is no longer a problem. I didn't really get to evaluate it deeper though. There are just too many things going on in parallel.

Tetsuo was proposing some patches to remove the lock but those patches had some other problems. If we have a simple patch to remove the oom_lock from the oom reaper then I will review it. I am not sure I can come up with a patch myself in few days.

Signature · 2 lines
Michal Hocko
SUSE Labs