diff mbox series

[RESEND] mm/oom_kill: don't kill exiting tasks in oom_kill_memcg_member

Message ID 20230314091136.264878-1-haifeng.xu@shopee.com
State New
Headers
Series [RESEND] mm/oom_kill: don't kill exiting tasks in oom_kill_memcg_member |

Commit Message

Haifeng Xu March 14, 2023, 9:11 a.m. UTC 
  If oom_group is set, oom_kill_process() invokes oom_kill_memcg_member()
to kill all processes in the memcg. When scanning tasks in memcg, maybe
the provided task is marked as oom victim. Also, some tasks are likely
to release their address space. There is no need to kill the exiting tasks.

In order to handle these tasks which may free memory in the future, add
a function helper reap_task_will_free_mem() to mark it as oom victim and
queue it in oom reaper.

Signed-off-by: Haifeng Xu <haifeng.xu@shopee.com>
---
 mm/oom_kill.c | 35 +++++++++++++++++++++++++++++------
 1 file changed, 29 insertions(+), 6 deletions(-)

Comments

Michal Hocko March 14, 2023, 9:19 a.m. UTC | #1 
On Tue 14-03-23 09:11:36, Haifeng Xu wrote:
> If oom_group is set, oom_kill_process() invokes oom_kill_memcg_member()
> to kill all processes in the memcg. When scanning tasks in memcg, maybe
> the provided task is marked as oom victim. Also, some tasks are likely
> to release their address space. There is no need to kill the exiting tasks.

This doesn't state any actual problem. Could you be more specific? Is
this a bug fix, a behavior change or an optimization?
Haifeng Xu March 14, 2023, 10:07 a.m. UTC | #2 
On 2023/3/14 17:19, Michal Hocko wrote:
> On Tue 14-03-23 09:11:36, Haifeng Xu wrote:
>> If oom_group is set, oom_kill_process() invokes oom_kill_memcg_member()
>> to kill all processes in the memcg. When scanning tasks in memcg, maybe
>> the provided task is marked as oom victim. Also, some tasks are likely
>> to release their address space. There is no need to kill the exiting tasks.
> 
> This doesn't state any actual problem. Could you be more specific? Is
> this a bug fix, a behavior change or an optimization?


1) oom_kill_process() has inovked __oom_kill_process() to kill the selected victim, but it will be scanned
in mem_cgroup_scan_tasks(). It's pointless to kill the victim twice. 

2) for those exiting processes, reaping them directly is also a faster way to free memory compare with invoking
__oom_kill_process().
Michal Hocko March 14, 2023, 10:16 a.m. UTC | #3 
On Tue 14-03-23 18:07:42, Haifeng Xu wrote:
> 
> 
> On 2023/3/14 17:19, Michal Hocko wrote:
> > On Tue 14-03-23 09:11:36, Haifeng Xu wrote:
> >> If oom_group is set, oom_kill_process() invokes oom_kill_memcg_member()
> >> to kill all processes in the memcg. When scanning tasks in memcg, maybe
> >> the provided task is marked as oom victim. Also, some tasks are likely
> >> to release their address space. There is no need to kill the exiting tasks.
> > 
> > This doesn't state any actual problem. Could you be more specific? Is
> > this a bug fix, a behavior change or an optimization?
> 
> 
> 1) oom_kill_process() has inovked __oom_kill_process() to kill the selected victim, but it will be scanned
> in mem_cgroup_scan_tasks(). It's pointless to kill the victim twice. 

Why does that matter though? The purpose of task_will_free_mem in
oom_kill_process is different. It would bail out from a potentially
noisy OOM report when the selected oom victim is expected to terminate
soon. __oom_kill_process called for the whole memcg doesn't aim at
avoiding any oom victims. It merely sends a kill signal too all of them.

> 2) for those exiting processes, reaping them directly is also a faster way to free memory compare with invoking
> __oom_kill_process().

Is it? What if the terminating task is blocked on lock? Async oom
reaping might release those resources in that case.
Haifeng Xu March 14, 2023, 11:07 a.m. UTC | #4 
On 2023/3/14 18:16, Michal Hocko wrote:
> On Tue 14-03-23 18:07:42, Haifeng Xu wrote:
>>
>>
>> On 2023/3/14 17:19, Michal Hocko wrote:
>>> On Tue 14-03-23 09:11:36, Haifeng Xu wrote:
>>>> If oom_group is set, oom_kill_process() invokes oom_kill_memcg_member()
>>>> to kill all processes in the memcg. When scanning tasks in memcg, maybe
>>>> the provided task is marked as oom victim. Also, some tasks are likely
>>>> to release their address space. There is no need to kill the exiting tasks.
>>>
>>> This doesn't state any actual problem. Could you be more specific? Is
>>> this a bug fix, a behavior change or an optimization?
>>
>>
>> 1) oom_kill_process() has inovked __oom_kill_process() to kill the selected victim, but it will be scanned
>> in mem_cgroup_scan_tasks(). It's pointless to kill the victim twice. 
> 
> Why does that matter though? The purpose of task_will_free_mem in
> oom_kill_process is different. It would bail out from a potentially
> noisy OOM report when the selected oom victim is expected to terminate
> soon. __oom_kill_process called for the whole memcg doesn't aim at
> avoiding any oom victims. It merely sends a kill signal too all of them.
> 

except sending kill signals, __oom_kill_process() will do some other work, such as print messeages, traversal all 
all user processes sharing mm which holds RCU section and so on. So if skip the victim, we don't need those work again
and it won't affect the original mechanism. All oom victims are still get killed. 

>> 2) for those exiting processes, reaping them directly is also a faster way to free memory compare with invoking
>> __oom_kill_process().
> 
> Is it? What if the terminating task is blocked on lock? Async oom
> reaping might release those resources in that case.

Yes, the reaping process is asynchronous. I mean we don't need the work mentioned above any more.
"reaping them directly" here is that joining the task in oom reaper queue.
Michal Hocko March 14, 2023, noon UTC | #5 
On Tue 14-03-23 19:07:27, Haifeng Xu wrote:
> 
> 
> On 2023/3/14 18:16, Michal Hocko wrote:
> > On Tue 14-03-23 18:07:42, Haifeng Xu wrote:
> >>
> >>
> >> On 2023/3/14 17:19, Michal Hocko wrote:
> >>> On Tue 14-03-23 09:11:36, Haifeng Xu wrote:
> >>>> If oom_group is set, oom_kill_process() invokes oom_kill_memcg_member()
> >>>> to kill all processes in the memcg. When scanning tasks in memcg, maybe
> >>>> the provided task is marked as oom victim. Also, some tasks are likely
> >>>> to release their address space. There is no need to kill the exiting tasks.
> >>>
> >>> This doesn't state any actual problem. Could you be more specific? Is
> >>> this a bug fix, a behavior change or an optimization?
> >>
> >>
> >> 1) oom_kill_process() has inovked __oom_kill_process() to kill the selected victim, but it will be scanned
> >> in mem_cgroup_scan_tasks(). It's pointless to kill the victim twice. 
> > 
> > Why does that matter though? The purpose of task_will_free_mem in
> > oom_kill_process is different. It would bail out from a potentially
> > noisy OOM report when the selected oom victim is expected to terminate
> > soon. __oom_kill_process called for the whole memcg doesn't aim at
> > avoiding any oom victims. It merely sends a kill signal too all of them.
> > 
> 
> except sending kill signals, __oom_kill_process() will do some other work, such as print messeages, traversal all 
> all user processes sharing mm which holds RCU section and so on. So if skip the victim, we don't need those work again
> and it won't affect the original mechanism. All oom victims are still get killed. 

mm sharing among processes is a very rare thing but do not forget that
task_will_free_mem needs to do the same thing for the same reason.

> >> 2) for those exiting processes, reaping them directly is also a faster way to free memory compare with invoking
> >> __oom_kill_process().
> > 
> > Is it? What if the terminating task is blocked on lock? Async oom
> > reaping might release those resources in that case.
> 
> Yes, the reaping process is asynchronous. I mean we don't need the work mentioned above any more.
> "reaping them directly" here is that joining the task in oom reaper queue.

I do not follow.

In any case I still do not see any actual justification for the change
other than "we can do it and it might turn out less expensive". This
alone is not sufficient, just be explicit, because oom is hardly a fast
path to optimize every single cpu cycle for. So unless you see an actual
real life problem that would be behaving much better or even fixed then
I am not convinced this is a worthwhile change to have.
Haifeng Xu March 14, 2023, 1:27 p.m. UTC | #6 
On 2023/3/14 20:00, Michal Hocko wrote:
> On Tue 14-03-23 19:07:27, Haifeng Xu wrote:
>>
>>
>> On 2023/3/14 18:16, Michal Hocko wrote:
>>> On Tue 14-03-23 18:07:42, Haifeng Xu wrote:
>>>>
>>>>
>>>> On 2023/3/14 17:19, Michal Hocko wrote:
>>>>> On Tue 14-03-23 09:11:36, Haifeng Xu wrote:
>>>>>> If oom_group is set, oom_kill_process() invokes oom_kill_memcg_member()
>>>>>> to kill all processes in the memcg. When scanning tasks in memcg, maybe
>>>>>> the provided task is marked as oom victim. Also, some tasks are likely
>>>>>> to release their address space. There is no need to kill the exiting tasks.
>>>>>
>>>>> This doesn't state any actual problem. Could you be more specific? Is
>>>>> this a bug fix, a behavior change or an optimization?
>>>>
>>>>
>>>> 1) oom_kill_process() has inovked __oom_kill_process() to kill the selected victim, but it will be scanned
>>>> in mem_cgroup_scan_tasks(). It's pointless to kill the victim twice. 
>>>
>>> Why does that matter though? The purpose of task_will_free_mem in
>>> oom_kill_process is different. It would bail out from a potentially
>>> noisy OOM report when the selected oom victim is expected to terminate
>>> soon. __oom_kill_process called for the whole memcg doesn't aim at
>>> avoiding any oom victims. It merely sends a kill signal too all of them.
>>>
>>
>> except sending kill signals, __oom_kill_process() will do some other work, such as print messeages, traversal all 
>> all user processes sharing mm which holds RCU section and so on. So if skip the victim, we don't need those work again
>> and it won't affect the original mechanism. All oom victims are still get killed. 
> 
> mm sharing among processes is a very rare thing but do not forget that
> task_will_free_mem needs to do the same thing for the same reason.

For the victim, __oom_kill_process() traversals all processes in the system whether there some other tasks sharing mm or not.
If skip it, this work can be dropped.

> 
>>>> 2) for those exiting processes, reaping them directly is also a faster way to free memory compare with invoking
>>>> __oom_kill_process().
>>>
>>> Is it? What if the terminating task is blocked on lock? Async oom
>>> reaping might release those resources in that case.
>>
>> Yes, the reaping process is asynchronous. I mean we don't need the work mentioned above any more.
>> "reaping them directly" here is that joining the task in oom reaper queue.
> 
> I do not follow.
> 
> In any case I still do not see any actual justification for the change
> other than "we can do it and it might turn out less expensive". This
> alone is not sufficient, just be explicit, because oom is hardly a fast
> path to optimize every single cpu cycle for. So unless you see an actual
> real life problem that would be behaving much better or even fixed then
> I am not convinced this is a worthwhile change to have.
> 

we can also see two same messages("Memory cgroup out of memory: Killed process ***")about the victim.
This seems a little confusing. If skip the victim, only one message was printed.
diff mbox series

Patch

diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 044e1eed720e..f16bca506dc2 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -996,15 +996,43 @@  static void __oom_kill_process(struct task_struct *victim, const char *message)
 }
 #undef K
 
+static bool reap_task_will_free_mem(struct task_struct *victim)
+{
+	bool ret = false;
+
+	task_lock(victim);
+	if (task_will_free_mem(victim)) {
+		mark_oom_victim(victim);
+		queue_oom_reaper(victim);
+		ret = true;
+	}
+	task_unlock(victim);
+
+	return ret;
+}
+
 /*
  * Kill provided task unless it's secured by setting
  * oom_score_adj to OOM_SCORE_ADJ_MIN.
+ * If the task is marked as oom victim or will free
+ * memory, there is no need to kill it again.
  */
 static int oom_kill_memcg_member(struct task_struct *task, void *message)
 {
 	if (task->signal->oom_score_adj != OOM_SCORE_ADJ_MIN &&
 	    !is_global_init(task)) {
 		get_task_struct(task);
+
+		/*
+		 * If the task is already exiting, don't alarm the sysadmin or kill
+		 * its children or threads, just give it access to memory reserves
+		 * so it can die quickly
+		 */
+		if (tsk_is_oom_victim(task) || reap_task_will_free_mem(task)) {
+			put_task_struct(task);
+			return 0;
+		}
+
 		__oom_kill_process(task, message);
 	}
 	return 0;
@@ -1022,15 +1050,10 @@  static void oom_kill_process(struct oom_control *oc, const char *message)
 	 * its children or threads, just give it access to memory reserves
 	 * so it can die quickly
 	 */
-	task_lock(victim);
-	if (task_will_free_mem(victim)) {
-		mark_oom_victim(victim);
-		queue_oom_reaper(victim);
-		task_unlock(victim);
+	if (reap_task_will_free_mem(victim)) {
 		put_task_struct(victim);
 		return;
 	}
-	task_unlock(victim);
 
 	if (__ratelimit(&oom_rs))
 		dump_header(oc, victim);