| Message ID | 1529056341-16182-1-git-send-email-ufo19890607@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
PING
On Fri 15-06-18 17:52:21, ufo19890607@gmail.com wrote: > From: yuzhoujian <yuzhoujian@didichuxing.com> > > Some users complains that system-wide oom report does not print memcg's > name which contains the task killed by the oom-killer. The current system > wide oom report prints the task's command, gfp_mask, order ,oom_score_adj > and shows the memory info, but misses some important information, etc. the > memcg that has reached its limit and the memcg to which the killed process > is attached. We do not print the memcg which reached the limit in the global context because that is irrelevant completely. I do agree that memcg of the oom victim might be interesting and the changelog should explain why. So what about the following wording instead: " The current system wide oom report prints information about the victim and the allocation context and restrictions. It, however, doesn't provide any information about memory cgroup the victim belongs to. This information can be interesting for container users because they can find the victim's container much more easily. " > I follow the advices of David Rientjes and Michal Hocko, and refactor > part of the oom report. After this patch, users can get the memcg's > path from the oom report and check the certain container more quickly. > > The oom print info after this patch: > oom-kill:constraint=<constraint>,nodemask=<nodemask>,origin_memcg=<memcg>,kill_memcg=<memcg>,task=<commm>,pid=<pid>,uid=<uid> [...] > diff --git a/include/linux/oom.h b/include/linux/oom.h > index 6adac113e96d..5bed78d4bfb8 100644 > --- a/include/linux/oom.h > +++ b/include/linux/oom.h > @@ -15,6 +15,20 @@ struct notifier_block; > struct mem_cgroup; > struct task_struct; > > +enum oom_constraint { > + CONSTRAINT_NONE, > + CONSTRAINT_CPUSET, > + CONSTRAINT_MEMORY_POLICY, > + CONSTRAINT_MEMCG, > +}; > + > +static const char * const oom_constraint_text[] = { > + [CONSTRAINT_NONE] = "CONSTRAINT_NONE", > + [CONSTRAINT_CPUSET] = "CONSTRAINT_CPUSET", > + [CONSTRAINT_MEMORY_POLICY] = "CONSTRAINT_MEMORY_POLICY", > + [CONSTRAINT_MEMCG] = "CONSTRAINT_MEMCG", > +}; I've suggested that this should be a separate patch. [...] > -void mem_cgroup_print_oom_info(struct mem_cgroup *memcg, struct task_struct *p) > +void mem_cgroup_print_oom_context(struct mem_cgroup *memcg, struct task_struct *p, > + enum oom_constraint constraint, nodemask_t *nodemask) > { > - struct mem_cgroup *iter; > - unsigned int i; > + struct cgroup *origin_cgrp, *kill_cgrp; > > rcu_read_lock(); > > + pr_info("oom-kill:constraint=%s,nodemask=%*pbl,origin_memcg=", > + oom_constraint_text[constraint], nodemask_pr_args(nodemask)); > + > + if (memcg) > + pr_cont_cgroup_path(memcg->css.cgroup); > + else > + pr_cont("(null)"); I do not like this. What does origin_memcg=(null) tell you? You really have to know the code to see this is a global oom killer actually. Furthermore I would expect that origin_memcg is tasks' origin memcg rather than oom's origin. So I think you want the following instead pr_info("oom-kill:constraint=%s,nodemask=%*pbl", oom_constraint_text[constraint], nodemask_pr_args(nodemask)); if (memcg) { pr_cont(", oom_memcg="); pr_cont_cgroup_path(memcg->css.cgroup); } if (p) { pr_cont(", task_memcg="); pr_cont_cgroup_path(task_cgroup(p, memory_cgrp_id)); pr_cont(", task=%s, pid=%5d, uid=%5d", p->comm, p->pid, from_kuid(&init_user_ns, task_uid(p))); } pr_cont("\n");
Hi Michal > diff --git a/include/linux/oom.h b/include/linux/oom.h > index 6adac113e96d..5bed78d4bfb8 100644 > --- a/include/linux/oom.h > +++ b/include/linux/oom.h > @@ -15,6 +15,20 @@ struct notifier_block; > struct mem_cgroup; > struct task_struct; > > +enum oom_constraint { > + CONSTRAINT_NONE, > + CONSTRAINT_CPUSET, > + CONSTRAINT_MEMORY_POLICY, > + CONSTRAINT_MEMCG, > +}; > + > +static const char * const oom_constraint_text[] = { > + [CONSTRAINT_NONE] = "CONSTRAINT_NONE", > + [CONSTRAINT_CPUSET] = "CONSTRAINT_CPUSET", > + [CONSTRAINT_MEMORY_POLICY] = "CONSTRAINT_MEMORY_POLICY", > + [CONSTRAINT_MEMCG] = "CONSTRAINT_MEMCG", > +}; > I've suggested that this should be a separate patch. I've separate this part in patch v7. [PATCH v7 1/2] Add an array of const char and enum oom_constraint in memcontrol.h On Sat 02-06-18 19:58:51, ufo19890607@gmail.com wrote: >> From: yuzhoujian <yuzhoujian@didichuxing.com> >> >> This patch will make some preparation for the follow-up patch: Refactor >> part of the oom report in dump_header. It puts enum oom_constraint in >> memcontrol.h and adds an array of const char for each constraint. > I do not get why you separate this specific part out. > oom_constraint_text is not used in the patch. It is almost always > preferable to have a user of newly added functionality. So do I need to separate this part ? Thanks
On Fri 22-06-18 17:33:12, 禹舟键 wrote: > Hi Michal > > diff --git a/include/linux/oom.h b/include/linux/oom.h > > index 6adac113e96d..5bed78d4bfb8 100644 > > --- a/include/linux/oom.h > > +++ b/include/linux/oom.h > > @@ -15,6 +15,20 @@ struct notifier_block; > > struct mem_cgroup; > > struct task_struct; > > > > +enum oom_constraint { > > + CONSTRAINT_NONE, > > + CONSTRAINT_CPUSET, > > + CONSTRAINT_MEMORY_POLICY, > > + CONSTRAINT_MEMCG, > > +}; > > + > > +static const char * const oom_constraint_text[] = { > > + [CONSTRAINT_NONE] = "CONSTRAINT_NONE", > > + [CONSTRAINT_CPUSET] = "CONSTRAINT_CPUSET", > > + [CONSTRAINT_MEMORY_POLICY] = "CONSTRAINT_MEMORY_POLICY", > > + [CONSTRAINT_MEMCG] = "CONSTRAINT_MEMCG", > > +}; > > > I've suggested that this should be a separate patch. > I've separate this part in patch v7. > > [PATCH v7 1/2] Add an array of const char and enum oom_constraint in > memcontrol.h > On Sat 02-06-18 19:58:51, ufo19890607@gmail.com wrote: > >> From: yuzhoujian <yuzhoujian@didichuxing.com> > >> > >> This patch will make some preparation for the follow-up patch: Refactor > >> part of the oom report in dump_header. It puts enum oom_constraint in > >> memcontrol.h and adds an array of const char for each constraint. > > > I do not get why you separate this specific part out. > > oom_constraint_text is not used in the patch. It is almost always > > preferable to have a user of newly added functionality. > > So do I need to separate this part ? You misunderstood my suggestion. Let me be more specific. Please separate the whole new oom_constraint including its _usage_.
Hi Michal > You misunderstood my suggestion. Let me be more specific. Please > separate the whole new oom_constraint including its _usage_. Sorry for misunderstanding your words. I think you want me to separate enum oom_constraint and static const char * const oom_constraint_text[] to two parts, am I right ? Michal Hocko <mhocko@kernel.org> 于2018年6月22日周五 下午6:42写道： > > On Fri 22-06-18 17:33:12, 禹舟键 wrote: > > Hi Michal > > > diff --git a/include/linux/oom.h b/include/linux/oom.h > > > index 6adac113e96d..5bed78d4bfb8 100644 > > > --- a/include/linux/oom.h > > > +++ b/include/linux/oom.h > > > @@ -15,6 +15,20 @@ struct notifier_block; > > > struct mem_cgroup; > > > struct task_struct; > > > > > > +enum oom_constraint { > > > + CONSTRAINT_NONE, > > > + CONSTRAINT_CPUSET, > > > + CONSTRAINT_MEMORY_POLICY, > > > + CONSTRAINT_MEMCG, > > > +}; > > > + > > > +static const char * const oom_constraint_text[] = { > > > + [CONSTRAINT_NONE] = "CONSTRAINT_NONE", > > > + [CONSTRAINT_CPUSET] = "CONSTRAINT_CPUSET", > > > + [CONSTRAINT_MEMORY_POLICY] = "CONSTRAINT_MEMORY_POLICY", > > > + [CONSTRAINT_MEMCG] = "CONSTRAINT_MEMCG", > > > +}; > > > > > I've suggested that this should be a separate patch. > > I've separate this part in patch v7. > > > > [PATCH v7 1/2] Add an array of const char and enum oom_constraint in > > memcontrol.h > > On Sat 02-06-18 19:58:51, ufo19890607@gmail.com wrote: > > >> From: yuzhoujian <yuzhoujian@didichuxing.com> > > >> > > >> This patch will make some preparation for the follow-up patch: Refactor > > >> part of the oom report in dump_header. It puts enum oom_constraint in > > >> memcontrol.h and adds an array of const char for each constraint. > > > > > I do not get why you separate this specific part out. > > > oom_constraint_text is not used in the patch. It is almost always > > > preferable to have a user of newly added functionality. > > > > So do I need to separate this part ? > > You misunderstood my suggestion. Let me be more specific. Please > separate the whole new oom_constraint including its _usage_. > -- > Michal Hocko > SUSE Labs
On Fri 22-06-18 19:40:54, 禹舟键 wrote: > Hi Michal > > You misunderstood my suggestion. Let me be more specific. Please > > separate the whole new oom_constraint including its _usage_. > > Sorry for misunderstanding your words. I think you want me to separate > enum oom_constraint and static const char * const > oom_constraint_text[] to two parts, am I right ? Just split the patch into two parts. The first to add oom_constraint* and use it. And the second which adds the missing memcg information to the oom report.
diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 6c6fb116e925..d62390fca414 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -30,6 +30,7 @@ #include <linux/vmstat.h> #include <linux/writeback.h> #include <linux/page-flags.h> +#include <linux/oom.h> struct mem_cgroup; struct page; @@ -491,8 +492,11 @@ void mem_cgroup_handle_over_high(void); unsigned long mem_cgroup_get_max(struct mem_cgroup *memcg); -void mem_cgroup_print_oom_info(struct mem_cgroup *memcg, - struct task_struct *p); +void mem_cgroup_print_oom_context(struct mem_cgroup *memcg, + struct task_struct *p, enum oom_constraint constraint, + nodemask_t *nodemask); + +void mem_cgroup_print_oom_meminfo(struct mem_cgroup *memcg); static inline void mem_cgroup_oom_enable(void) { @@ -903,7 +907,13 @@ static inline unsigned long mem_cgroup_get_max(struct mem_cgroup *memcg) } static inline void -mem_cgroup_print_oom_info(struct mem_cgroup *memcg, struct task_struct *p) +mem_cgroup_print_oom_context(struct mem_cgroup *memcg, struct task_struct *p, + enum oom_constraint constraint, nodemask_t *nodemask) +{ +} + +static inline void +mem_cgroup_print_oom_meminfo(struct mem_cgroup *memcg) { } diff --git a/include/linux/oom.h b/include/linux/oom.h index 6adac113e96d..5bed78d4bfb8 100644 --- a/include/linux/oom.h +++ b/include/linux/oom.h @@ -15,6 +15,20 @@ struct notifier_block; struct mem_cgroup; struct task_struct; +enum oom_constraint { + CONSTRAINT_NONE, + CONSTRAINT_CPUSET, + CONSTRAINT_MEMORY_POLICY, + CONSTRAINT_MEMCG, +}; + +static const char * const oom_constraint_text[] = { + [CONSTRAINT_NONE] = "CONSTRAINT_NONE", + [CONSTRAINT_CPUSET] = "CONSTRAINT_CPUSET", + [CONSTRAINT_MEMORY_POLICY] = "CONSTRAINT_MEMORY_POLICY", + [CONSTRAINT_MEMCG] = "CONSTRAINT_MEMCG", +}; + /* * Details of the page allocation that triggered the oom killer that are used to * determine what should be killed. @@ -42,6 +56,9 @@ struct oom_control { unsigned long totalpages; struct task_struct *chosen; unsigned long chosen_points; + + /* Used to print the constraint info. */ + enum oom_constraint constraint; }; extern struct mutex oom_lock; diff --git a/mm/memcontrol.c b/mm/memcontrol.c index e6f0d5ef320a..7e0704bdff01 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -1119,32 +1119,51 @@ static const char *const memcg1_stat_names[] = { #define K(x) ((x) << (PAGE_SHIFT-10)) /** - * mem_cgroup_print_oom_info: Print OOM information relevant to memory controller. - * @memcg: The memory cgroup that went over limit + * mem_cgroup_print_oom_context: Print OOM context information relevant to + * memory controller. + * @memcg: The origin memory cgroup that went over limit * @p: Task that is going to be killed + * @constraint: The allocation constraint + * @nodemask: The allocation nodemask * * NOTE: @memcg and @p's mem_cgroup can be different when hierarchy is * enabled */ -void mem_cgroup_print_oom_info(struct mem_cgroup *memcg, struct task_struct *p) +void mem_cgroup_print_oom_context(struct mem_cgroup *memcg, struct task_struct *p, + enum oom_constraint constraint, nodemask_t *nodemask) { - struct mem_cgroup *iter; - unsigned int i; + struct cgroup *origin_cgrp, *kill_cgrp; rcu_read_lock(); + pr_info("oom-kill:constraint=%s,nodemask=%*pbl,origin_memcg=", + oom_constraint_text[constraint], nodemask_pr_args(nodemask)); + + if (memcg) + pr_cont_cgroup_path(memcg->css.cgroup); + else + pr_cont("(null)"); + if (p) { - pr_info("Task in "); + pr_cont(",kill_memcg="); pr_cont_cgroup_path(task_cgroup(p, memory_cgrp_id)); - pr_cont(" killed as a result of limit of "); - } else { - pr_info("Memory limit reached of cgroup "); + pr_cont(",task=%s,pid=%5d,uid=%5d", + p->comm, p->pid, from_kuid(&init_user_ns, task_uid(p))); } - - pr_cont_cgroup_path(memcg->css.cgroup); pr_cont("\n"); rcu_read_unlock(); +} + +/** + * mem_cgroup_print_oom_info: Print OOM memory information relevant to + * memory controller. + * @memcg: The memory cgroup that went over limit + */ +void mem_cgroup_print_oom_meminfo(struct mem_cgroup *memcg) +{ + struct mem_cgroup *iter; + unsigned int i; pr_info("memory: usage %llukB, limit %llukB, failcnt %lu\n", K((u64)page_counter_read(&memcg->memory)), diff --git a/mm/oom_kill.c b/mm/oom_kill.c index 84081e77bc51..1db70cec0c16 100644 --- a/mm/oom_kill.c +++ b/mm/oom_kill.c @@ -237,13 +237,6 @@ unsigned long oom_badness(struct task_struct *p, struct mem_cgroup *memcg, return points > 0 ? points : 1; } -enum oom_constraint { - CONSTRAINT_NONE, - CONSTRAINT_CPUSET, - CONSTRAINT_MEMORY_POLICY, - CONSTRAINT_MEMCG, -}; - /* * Determine the type of allocation constraint. */ @@ -430,8 +423,10 @@ static void dump_header(struct oom_control *oc, struct task_struct *p) cpuset_print_current_mems_allowed(); dump_stack(); + mem_cgroup_print_oom_context(oc->memcg, p, + oc->constraint, oc->nodemask); if (is_memcg_oom(oc)) - mem_cgroup_print_oom_info(oc->memcg, p); + mem_cgroup_print_oom_meminfo(oc->memcg); else { show_mem(SHOW_MEM_FILTER_NODES, oc->nodemask); if (is_dump_unreclaim_slabs()) @@ -973,8 +968,7 @@ static void oom_kill_process(struct oom_control *oc, const char *message) /* * Determines whether the kernel must panic because of the panic_on_oom sysctl. */ -static void check_panic_on_oom(struct oom_control *oc, - enum oom_constraint constraint) +static void check_panic_on_oom(struct oom_control *oc) { if (likely(!sysctl_panic_on_oom)) return; @@ -984,7 +978,7 @@ static void check_panic_on_oom(struct oom_control *oc, * does not panic for cpuset, mempolicy, or memcg allocation * failures. */ - if (constraint != CONSTRAINT_NONE) + if (oc->constraint != CONSTRAINT_NONE) return; } /* Do not panic for oom kills triggered by sysrq */ @@ -1021,8 +1015,8 @@ EXPORT_SYMBOL_GPL(unregister_oom_notifier); bool out_of_memory(struct oom_control *oc) { unsigned long freed = 0; - enum oom_constraint constraint = CONSTRAINT_NONE; + oc->constraint = CONSTRAINT_NONE; if (oom_killer_disabled) return false; @@ -1057,10 +1051,10 @@ bool out_of_memory(struct oom_control *oc) * Check if there were limitations on the allocation (only relevant for * NUMA and memcg) that may require different handling. */ - constraint = constrained_alloc(oc); - if (constraint != CONSTRAINT_MEMORY_POLICY) + oc->constraint = constrained_alloc(oc); + if (oc->constraint != CONSTRAINT_MEMORY_POLICY) oc->nodemask = NULL; - check_panic_on_oom(oc, constraint); + check_panic_on_oom(oc); if (!is_memcg_oom(oc) && sysctl_oom_kill_allocating_task && current->mm && !oom_unkillable_task(current, NULL, oc->nodemask) &&