Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1650361 > unrolled thread

[PATCH v2] mm/oom_kill: count global and memory cgroup oom kills

Back to article view | Back to linux.kernel

Contents

  [PATCH v2] mm/oom_kill: count global and memory cgroup oom kills Konstantin Khlebnikov <khlebnikov@yandex-team.ru> - 2017-05-25 12:30 +0200
    Re: [PATCH v2] mm/oom_kill: count global and memory cgroup oom  kills David Rientjes <rientjes@google.com> - 2017-05-30 06:40 +0200
      Re: [PATCH v2] mm/oom_kill: count global and memory cgroup oom kills Konstantin Khlebnikov <koct9i@gmail.com> - 2017-05-30 07:00 +0200
    Re: [PATCH v2] mm/oom_kill: count global and memory cgroup oom kills Michal Hocko <mhocko@kernel.org> - 2017-06-05 11:00 +0200
      Re: [PATCH v2] mm/oom_kill: count global and memory cgroup oom kills Konstantin Khlebnikov <khlebnikov@yandex-team.ru> - 2017-06-05 16:30 +0200

#1650361 — [PATCH v2] mm/oom_kill: count global and memory cgroup oom kills

Show count of oom killer invocations in /proc/vmstat and count of
processes killed in memory cgroup in knob "memory.events"
(in memory.oom_control for v1 cgroup).

Also describe difference between "oom" and "oom_kill" in memory
cgroup documentation. Currently oom in memory cgroup kills tasks
iff shortage has happened inside page fault.

These counters helps in monitoring oom kills - for now
the only way is grepping for magic words in kernel log.

Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>

---

v1: https://lkml.kernel.org/r/149520375057.74196.2843113275800730971.stgit@buzz

v2:
* count all oom kills in /proc/vmstat
* update counter for cgroup which tasks belongs to
---
 Documentation/cgroup-v2.txt   |   20 ++++++++++++++++----
 include/linux/memcontrol.h    |    5 ++++-
 include/linux/vm_event_item.h |    1 +
 mm/memcontrol.c               |    2 ++
 mm/oom_kill.c                 |    5 +++++
 mm/vmstat.c                   |    1 +
 6 files changed, 29 insertions(+), 5 deletions(-)

diff --git a/Documentation/cgroup-v2.txt b/Documentation/cgroup-v2.txt
index dc5e2dcdbef4..738b1c7023ad 100644
--- a/Documentation/cgroup-v2.txt
+++ b/Documentation/cgroup-v2.txt
@@ -826,13 +826,25 @@ PAGE_SIZE multiple when read back.
 
 		The number of times the cgroup's memory usage was
 		about to go over the max boundary.  If direct reclaim
-		fails to bring it down, the OOM killer is invoked.
+		fails to bring it down, the cgroup goes to OOM state.
 
 	  oom
 
-		The number of times the OOM killer has been invoked in
-		the cgroup.  This may not exactly match the number of
-		processes killed but should generally be close.
+		The number of time the cgroup's memory usage was
+		reached the limit and allocation was about to fail.
+
+		Depending on context result could be invocation of OOM
+		killer and retrying allocation or failing alloction.
+
+		Failed allocation in its turn could be returned into
+		userspace as -ENOMEM or siletly ignored in cases like
+		disk readahead.	 For now OOM in memory cgroup kills
+		tasks iff shortage has happened inside page fault.
+
+	  oom_kill
+
+		The number of processes belonging to this cgroup
+		killed by any kind of OOM killer.
 
   memory.stat
 
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 899949bbb2f9..42296f7001da 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -556,8 +556,11 @@ static inline void mem_cgroup_count_vm_event(struct mm_struct *mm,
 
 	rcu_read_lock();
 	memcg = mem_cgroup_from_task(rcu_dereference(mm->owner));
-	if (likely(memcg))
+	if (likely(memcg)) {
 		this_cpu_inc(memcg->stat->events[idx]);
+		if (idx == OOM_KILL)
+			cgroup_file_notify(&memcg->events_file);
+	}
 	rcu_read_unlock();
 }
 #ifdef CONFIG_TRANSPARENT_HUGEPAGE
diff --git a/include/linux/vm_event_item.h b/include/linux/vm_event_item.h
index d84ae90ccd5c..1707e0a7d943 100644
--- a/include/linux/vm_event_item.h
+++ b/include/linux/vm_event_item.h
@@ -41,6 +41,7 @@ enum vm_event_item { PGPGIN, PGPGOUT, PSWPIN, PSWPOUT,
 		KSWAPD_LOW_WMARK_HIT_QUICKLY, KSWAPD_HIGH_WMARK_HIT_QUICKLY,
 		PAGEOUTRUN, PGROTATED,
 		DROP_PAGECACHE, DROP_SLAB,
+		OOM_KILL,
 #ifdef CONFIG_NUMA_BALANCING
 		NUMA_PTE_UPDATES,
 		NUMA_HUGE_PTE_UPDATES,
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 94172089f52f..7011ebf2b90e 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3574,6 +3574,7 @@ static int mem_cgroup_oom_control_read(struct seq_file *sf, void *v)
 
 	seq_printf(sf, "oom_kill_disable %d\n", memcg->oom_kill_disable);
 	seq_printf(sf, "under_oom %d\n", (bool)memcg->under_oom);
+	seq_printf(sf, "oom_kill %lu\n", memcg_sum_events(memcg, OOM_KILL));
 	return 0;
 }
 
@@ -5165,6 +5166,7 @@ static int memory_events_show(struct seq_file *m, void *v)
 	seq_printf(m, "high %lu\n", memcg_sum_events(memcg, MEMCG_HIGH));
 	seq_printf(m, "max %lu\n", memcg_sum_events(memcg, MEMCG_MAX));
 	seq_printf(m, "oom %lu\n", memcg_sum_events(memcg, MEMCG_OOM));
+	seq_printf(m, "oom_kill %lu\n", memcg_sum_events(memcg, OOM_KILL));
 
 	return 0;
 }
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 04c9143a8625..dd30a045ef5b 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -876,6 +876,11 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
 	/* Get a reference to safely compare mm after task_unlock(victim) */
 	mm = victim->mm;
 	mmgrab(mm);
+
+	/* Raise event before sending signal: reaper must see this */
+	count_vm_event(OOM_KILL);
+	mem_cgroup_count_vm_event(mm, OOM_KILL);
+
 	/*
 	 * We should send SIGKILL before setting TIF_MEMDIE in order to prevent
 	 * the OOM victim from depleting the memory reserves from the user
diff --git a/mm/vmstat.c b/mm/vmstat.c
index 76f73670200a..fe80b81a86e0 100644
--- a/mm/vmstat.c
+++ b/mm/vmstat.c
@@ -1018,6 +1018,7 @@ const char * const vmstat_text[] = {
 
 	"drop_pagecache",
 	"drop_slab",
+	"oom_kill",
 
 #ifdef CONFIG_NUMA_BALANCING
 	"numa_pte_updates",

[toc] | [next] | [standalone]

#1652803 — Re: [PATCH v2] mm/oom_kill: count global and memory cgroup oom kills

On Thu, 25 May 2017, Konstantin Khlebnikov wrote:

> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 04c9143a8625..dd30a045ef5b 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -876,6 +876,11 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
>  	/* Get a reference to safely compare mm after task_unlock(victim) */
>  	mm = victim->mm;
>  	mmgrab(mm);
> +
> +	/* Raise event before sending signal: reaper must see this */

How is the oom reaper involved here?

> +	count_vm_event(OOM_KILL);
> +	mem_cgroup_count_vm_event(mm, OOM_KILL);
> +
>  	/*
>  	 * We should send SIGKILL before setting TIF_MEMDIE in order to prevent
>  	 * the OOM victim from depleting the memory reserves from the user

[toc] | [prev] | [next] | [standalone]

#1652810

On Tue, May 30, 2017 at 7:29 AM, David Rientjes <rientjes@google.com> wrote:
> On Thu, 25 May 2017, Konstantin Khlebnikov wrote:
>
>> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
>> index 04c9143a8625..dd30a045ef5b 100644
>> --- a/mm/oom_kill.c
>> +++ b/mm/oom_kill.c
>> @@ -876,6 +876,11 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
>>       /* Get a reference to safely compare mm after task_unlock(victim) */
>>       mm = victim->mm;
>>       mmgrab(mm);
>> +
>> +     /* Raise event before sending signal: reaper must see this */
>
> How is the oom reaper involved here?

Task reaper - OOM event should happens before SIGCHLD.

>
>> +     count_vm_event(OOM_KILL);
>> +     mem_cgroup_count_vm_event(mm, OOM_KILL);
>> +
>>       /*
>>        * We should send SIGKILL before setting TIF_MEMDIE in order to prevent
>>        * the OOM victim from depleting the memory reserves from the user
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org.  For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[toc] | [prev] | [next] | [standalone]

#1657403

On Thu 25-05-17 13:28:30, Konstantin Khlebnikov wrote:
> Show count of oom killer invocations in /proc/vmstat and count of
> processes killed in memory cgroup in knob "memory.events"
> (in memory.oom_control for v1 cgroup).
> 
> Also describe difference between "oom" and "oom_kill" in memory
> cgroup documentation. Currently oom in memory cgroup kills tasks
> iff shortage has happened inside page fault.
> 
> These counters helps in monitoring oom kills - for now
> the only way is grepping for magic words in kernel log.

Yes this is less than optimal and the counter sounds like a good step
forward. I have 2 comments to the patch though.

[...]

> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 899949bbb2f9..42296f7001da 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -556,8 +556,11 @@ static inline void mem_cgroup_count_vm_event(struct mm_struct *mm,
>  
>  	rcu_read_lock();
>  	memcg = mem_cgroup_from_task(rcu_dereference(mm->owner));
> -	if (likely(memcg))
> +	if (likely(memcg)) {
>  		this_cpu_inc(memcg->stat->events[idx]);
> +		if (idx == OOM_KILL)
> +			cgroup_file_notify(&memcg->events_file);
> +	}
>  	rcu_read_unlock();

Well, this is ugly. I see how you want to share the global counter and
the memcg event which needs the notification. But I cannot say this
would be really easy to follow. Can we have at least a comment in
memcg_event_item enum definition?

> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 04c9143a8625..dd30a045ef5b 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -876,6 +876,11 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
>  	/* Get a reference to safely compare mm after task_unlock(victim) */
>  	mm = victim->mm;
>  	mmgrab(mm);
> +
> +	/* Raise event before sending signal: reaper must see this */
> +	count_vm_event(OOM_KILL);
> +	mem_cgroup_count_vm_event(mm, OOM_KILL);
> +
>  	/*
>  	 * We should send SIGKILL before setting TIF_MEMDIE in order to prevent
>  	 * the OOM victim from depleting the memory reserves from the user

Why don't you count tasks which share mm with the oom victim? 
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 0e2c925e7826..9a95947a60ba 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -924,6 +924,8 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
 		 */
 		if (unlikely(p->flags & PF_KTHREAD))
 			continue;
+		count_vm_event(OOM_KILL);
+		count_memcg_event_mm(mm, OOM_KILL);
 		do_send_sig_info(SIGKILL, SEND_SIG_FORCED, p, true);
 	}
 	rcu_read_unlock();

Other than that looks good to me.
Acked-by: Michal Hocko <mhocko@suse.com>
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1657628

On 05.06.2017 11:50, Michal Hocko wrote:
> On Thu 25-05-17 13:28:30, Konstantin Khlebnikov wrote:
>> Show count of oom killer invocations in /proc/vmstat and count of
>> processes killed in memory cgroup in knob "memory.events"
>> (in memory.oom_control for v1 cgroup).
>>
>> Also describe difference between "oom" and "oom_kill" in memory
>> cgroup documentation. Currently oom in memory cgroup kills tasks
>> iff shortage has happened inside page fault.
>>
>> These counters helps in monitoring oom kills - for now
>> the only way is grepping for magic words in kernel log.
> 
> Yes this is less than optimal and the counter sounds like a good step
> forward. I have 2 comments to the patch though.
> 
> [...]
> 
>> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
>> index 899949bbb2f9..42296f7001da 100644
>> --- a/include/linux/memcontrol.h
>> +++ b/include/linux/memcontrol.h
>> @@ -556,8 +556,11 @@ static inline void mem_cgroup_count_vm_event(struct mm_struct *mm,
>>   
>>   	rcu_read_lock();
>>   	memcg = mem_cgroup_from_task(rcu_dereference(mm->owner));
>> -	if (likely(memcg))
>> +	if (likely(memcg)) {
>>   		this_cpu_inc(memcg->stat->events[idx]);
>> +		if (idx == OOM_KILL)
>> +			cgroup_file_notify(&memcg->events_file);
>> +	}
>>   	rcu_read_unlock();
> 
> Well, this is ugly. I see how you want to share the global counter and
> the memcg event which needs the notification. But I cannot say this
> would be really easy to follow. Can we have at least a comment in
> memcg_event_item enum definition?

Yep, this is a little bit ugly.
But this funciton is static-inline and idx always constant so resulting code is fine.

> 
>> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
>> index 04c9143a8625..dd30a045ef5b 100644
>> --- a/mm/oom_kill.c
>> +++ b/mm/oom_kill.c
>> @@ -876,6 +876,11 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
>>   	/* Get a reference to safely compare mm after task_unlock(victim) */
>>   	mm = victim->mm;
>>   	mmgrab(mm);
>> +
>> +	/* Raise event before sending signal: reaper must see this */
>> +	count_vm_event(OOM_KILL);
>> +	mem_cgroup_count_vm_event(mm, OOM_KILL);
>> +
>>   	/*
>>   	 * We should send SIGKILL before setting TIF_MEMDIE in order to prevent
>>   	 * the OOM victim from depleting the memory reserves from the user
> 
> Why don't you count tasks which share mm with the oom victim?

Yes, this makes sense. But these kills are not logged thus counter will differs from logged events.
Also these tasks might live in different cgroups, so counting to mm owner isn't correct.

> diff --git a/mm/oom_kill.c b/mm/oom_kill.c
> index 0e2c925e7826..9a95947a60ba 100644
> --- a/mm/oom_kill.c
> +++ b/mm/oom_kill.c
> @@ -924,6 +924,8 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
>   		 */
>   		if (unlikely(p->flags & PF_KTHREAD))
>   			continue;
> +		count_vm_event(OOM_KILL);
> +		count_memcg_event_mm(mm, OOM_KILL);
>   		do_send_sig_info(SIGKILL, SEND_SIG_FORCED, p, true);
>   	}
>   	rcu_read_unlock();
> 
> Other than that looks good to me.
> Acked-by: Michal Hocko <mhocko@suse.com>
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web