* Re: [PATCH v2] mm/oom_kill.c: remove the unmatched comments [not found] <20201027144529.GA3558@rlk> @ 2020-10-27 14:58 ` Michal Hocko 2020-10-27 15:11 ` Hui Su 0 siblings, 1 reply; 8+ messages in thread From: Michal Hocko @ 2020-10-27 14:58 UTC (permalink / raw) To: Hui Su; +Cc: akpm, linux-mm, linux-kernel On Tue 27-10-20 22:45:29, Hui Su wrote: > is_dump_unreclaim_slabs() just check whether nr_unreclaimable > slabs amount is greater than user memory, not match witch comment. As I've tried to explain, the comment is not explaining what the function does but how it should be used. It is not a kerneldoc afterall. So it is a good match. I can see how that might confuse somebody so I am not against changing this but the changelog shouldn't really be confusing on its own. What do you think about the following instead. " Comment for is_dump_unreclaim_slabs is not really clear whether it is meant to instruct how to use the function or whether it is an outdated information of the past implementation of the function. it doesn't realy help that is_dump_unreclaim_slabs is hard to grasp on its own. Rename the helper to should_dump_unreclaim_slabs which should make it clear what it is meant to do and drop the comment as the purpose should be pretty evident now. " > So delete the comment, and rename it to should_dump_unreclaim_slabs(). > > Signed-off-by: Hui Su <sh_def@163.com> > --- > mm/oom_kill.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c > index 8b84661a6410..d181e24d7193 100644 > --- a/mm/oom_kill.c > +++ b/mm/oom_kill.c > @@ -170,11 +170,7 @@ static bool oom_unkillable_task(struct task_struct *p) > return false; > } > > -/* > - * Print out unreclaimble slabs info when unreclaimable slabs amount is greater > - * than all user memory (LRU pages) > - */ > -static bool is_dump_unreclaim_slabs(void) > +static bool should_dump_unreclaim_slabs(void) > { > unsigned long nr_lru; > > @@ -463,7 +459,7 @@ static void dump_header(struct oom_control *oc, struct task_struct *p) > mem_cgroup_print_oom_meminfo(oc->memcg); > else { > show_mem(SHOW_MEM_FILTER_NODES, oc->nodemask); > - if (is_dump_unreclaim_slabs()) > + if (should_dump_unreclaim_slabs()) > dump_unreclaimable_slab(); > } > if (sysctl_oom_dump_tasks) > -- > 2.25.1 > -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] mm/oom_kill.c: remove the unmatched comments 2020-10-27 14:58 ` [PATCH v2] mm/oom_kill.c: remove the unmatched comments Michal Hocko @ 2020-10-27 15:11 ` Hui Su 2020-10-27 19:23 ` Michal Hocko 0 siblings, 1 reply; 8+ messages in thread From: Hui Su @ 2020-10-27 15:11 UTC (permalink / raw) To: Michal Hocko, akpm, linux-mm, linux-kernel On Tue, Oct 27, 2020 at 03:58:14PM +0100, Michal Hocko wrote: > On Tue 27-10-20 22:45:29, Hui Su wrote: > > is_dump_unreclaim_slabs() just check whether nr_unreclaimable > > slabs amount is greater than user memory, not match witch comment. > > As I've tried to explain, the comment is not explaining what the > function does but how it should be used. It is not a kerneldoc afterall. > So it is a good match. I can see how that might confuse somebody so I am > not against changing this but the changelog shouldn't really be > confusing on its own. What do you think about the following instead. > Hi, Michal: Thanks for your fast reply, your changlog is much more accurate. And should i resend a patch V3 use the changlog below? Thanks. > " > Comment for is_dump_unreclaim_slabs is not really clear whether it is > meant to instruct how to use the function or whether it is an outdated > information of the past implementation of the function. it doesn't realy > help that is_dump_unreclaim_slabs is hard to grasp on its own. > Rename the helper to should_dump_unreclaim_slabs which should make it > clear what it is meant to do and drop the comment as the purpose should > be pretty evident now. > " > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] mm/oom_kill.c: remove the unmatched comments 2020-10-27 15:11 ` Hui Su @ 2020-10-27 19:23 ` Michal Hocko 2020-10-28 15:31 ` [PATCH v3] mm/oom_kill: remove comment and rename is_dump_unreclaim_slabs() Hui Su 0 siblings, 1 reply; 8+ messages in thread From: Michal Hocko @ 2020-10-27 19:23 UTC (permalink / raw) To: Hui Su; +Cc: akpm, linux-mm, linux-kernel On Tue 27-10-20 23:11:56, Hui Su wrote: > On Tue, Oct 27, 2020 at 03:58:14PM +0100, Michal Hocko wrote: > > On Tue 27-10-20 22:45:29, Hui Su wrote: > > > is_dump_unreclaim_slabs() just check whether nr_unreclaimable > > > slabs amount is greater than user memory, not match witch comment. > > > > As I've tried to explain, the comment is not explaining what the > > function does but how it should be used. It is not a kerneldoc afterall. > > So it is a good match. I can see how that might confuse somebody so I am > > not against changing this but the changelog shouldn't really be > > confusing on its own. What do you think about the following instead. > > > > Hi, Michal: > > Thanks for your fast reply, your changlog is much more accurate. > > And should i resend a patch V3 use the changlog below? Yes, just repost in reply to this email. With the updated changelog Feel free to add Acked-by: Michal Hocko <mhocko@suse.com> > > Thanks. > > > " > > Comment for is_dump_unreclaim_slabs is not really clear whether it is > > meant to instruct how to use the function or whether it is an outdated > > information of the past implementation of the function. it doesn't realy > > help that is_dump_unreclaim_slabs is hard to grasp on its own. > > Rename the helper to should_dump_unreclaim_slabs which should make it > > clear what it is meant to do and drop the comment as the purpose should > > be pretty evident now. > > " > > > > -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v3] mm/oom_kill: remove comment and rename is_dump_unreclaim_slabs() 2020-10-27 19:23 ` Michal Hocko @ 2020-10-28 15:31 ` Hui Su 2020-10-28 21:53 ` Andrew Morton 2020-10-30 18:27 ` [PATCH v4] mm/oom_kill: change " Hui Su 0 siblings, 2 replies; 8+ messages in thread From: Hui Su @ 2020-10-28 15:31 UTC (permalink / raw) To: Michal Hocko, akpm, linux-mm, linux-kernel, sh_def Comment for is_dump_unreclaim_slabs is not really clear whether it is meant to instruct how to use the function or whether it is an outdated information of the past implementation of the function. it doesn't realy help that is_dump_unreclaim_slabs is hard to grasp on its own. Rename the helper to should_dump_unreclaim_slabs which should make it clear what it is meant to do and drop the comment as the purpose should be pretty evident now. Signed-off-by: Hui Su <sh_def@163.com> Acked-by: Michal Hocko <mhocko@suse.com> --- mm/oom_kill.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/mm/oom_kill.c b/mm/oom_kill.c index 8b84661a6410..d181e24d7193 100644 --- a/mm/oom_kill.c +++ b/mm/oom_kill.c @@ -170,11 +170,7 @@ static bool oom_unkillable_task(struct task_struct *p) return false; } -/* - * Print out unreclaimble slabs info when unreclaimable slabs amount is greater - * than all user memory (LRU pages) - */ -static bool is_dump_unreclaim_slabs(void) +static bool should_dump_unreclaim_slabs(void) { unsigned long nr_lru; @@ -463,7 +459,7 @@ static void dump_header(struct oom_control *oc, struct task_struct *p) mem_cgroup_print_oom_meminfo(oc->memcg); else { show_mem(SHOW_MEM_FILTER_NODES, oc->nodemask); - if (is_dump_unreclaim_slabs()) + if (should_dump_unreclaim_slabs()) dump_unreclaimable_slab(); } if (sysctl_oom_dump_tasks) -- 2.29.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v3] mm/oom_kill: remove comment and rename is_dump_unreclaim_slabs() 2020-10-28 15:31 ` [PATCH v3] mm/oom_kill: remove comment and rename is_dump_unreclaim_slabs() Hui Su @ 2020-10-28 21:53 ` Andrew Morton 2020-10-29 7:51 ` Michal Hocko 2020-10-30 18:27 ` [PATCH v4] mm/oom_kill: change " Hui Su 1 sibling, 1 reply; 8+ messages in thread From: Andrew Morton @ 2020-10-28 21:53 UTC (permalink / raw) To: Hui Su; +Cc: Michal Hocko, linux-mm, linux-kernel On Wed, 28 Oct 2020 23:31:41 +0800 Hui Su <sh_def@163.com> wrote: > Comment for is_dump_unreclaim_slabs is not really clear whether it is > meant to instruct how to use the function or whether it is an outdated > information of the past implementation of the function. it doesn't realy > help that is_dump_unreclaim_slabs is hard to grasp on its own. > > Rename the helper to should_dump_unreclaim_slabs which should make it > clear what it is meant to do and drop the comment as the purpose > should be pretty evident now. > I think your recent attempt to improve the comment: /* * Check whether unreclaimable slabs amount is greater than all user * memory(LRU pages). */ was actually somewhat useful, and worth retaining. It would be better if it explained *why* we're doing this, rather than simply "what we are doing"? <looks at the code> It's actually quite unobvious why we're doing this! ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v3] mm/oom_kill: remove comment and rename is_dump_unreclaim_slabs() 2020-10-28 21:53 ` Andrew Morton @ 2020-10-29 7:51 ` Michal Hocko 0 siblings, 0 replies; 8+ messages in thread From: Michal Hocko @ 2020-10-29 7:51 UTC (permalink / raw) To: Andrew Morton; +Cc: Hui Su, linux-mm, linux-kernel On Wed 28-10-20 14:53:30, Andrew Morton wrote: > On Wed, 28 Oct 2020 23:31:41 +0800 Hui Su <sh_def@163.com> wrote: > > > Comment for is_dump_unreclaim_slabs is not really clear whether it is > > meant to instruct how to use the function or whether it is an outdated > > information of the past implementation of the function. it doesn't realy > > help that is_dump_unreclaim_slabs is hard to grasp on its own. > > > > Rename the helper to should_dump_unreclaim_slabs which should make it > > clear what it is meant to do and drop the comment as the purpose > > should be pretty evident now. > > > > I think your recent attempt to improve the comment: > > /* > * Check whether unreclaimable slabs amount is greater than all user > * memory(LRU pages). > */ > > was actually somewhat useful, and worth retaining. > > It would be better if it explained *why* we're doing this, rather than > simply "what we are doing"? > > <looks at the code> > > It's actually quite unobvious why we're doing this! It's quite arbitrary criterion to print slab information in the oom report. The current logic is to dump if there is more slabs than LRU pages which should be pretty obvious from the code. Why this rather than e.g. slab * k > lru? Well, no strong reason, AFAIK. We just want to catch too much slab memory cases. -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v4] mm/oom_kill: change comment and rename is_dump_unreclaim_slabs() 2020-10-28 15:31 ` [PATCH v3] mm/oom_kill: remove comment and rename is_dump_unreclaim_slabs() Hui Su 2020-10-28 21:53 ` Andrew Morton @ 2020-10-30 18:27 ` Hui Su 2020-11-02 7:28 ` Michal Hocko 1 sibling, 1 reply; 8+ messages in thread From: Hui Su @ 2020-10-30 18:27 UTC (permalink / raw) To: akpm, linux-mm, linux-kernel Change the comment of is_dump_unreclaim_slabs(), it just check whether nr_unreclaimable slabs amount is greater than user memory, and explain why we dump unreclaim slabs. Rename it to should_dump_unreclaim_slab() maybe better. Signed-off-by: Hui Su <sh_def@163.com> --- mm/oom_kill.c | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/mm/oom_kill.c b/mm/oom_kill.c index 8b84661a6410..04b19b7b5435 100644 --- a/mm/oom_kill.c +++ b/mm/oom_kill.c @@ -170,11 +170,13 @@ static bool oom_unkillable_task(struct task_struct *p) return false; } -/* - * Print out unreclaimble slabs info when unreclaimable slabs amount is greater - * than all user memory (LRU pages) - */ -static bool is_dump_unreclaim_slabs(void) +/** + * Check whether unreclaimable slab amount is greater than + * all user memory(LRU pages). + * dump_unreclaimable_slab() could help in the case that + * oom due to too much unreclaimable slab used by kernel. +*/ +static bool should_dump_unreclaim_slab(void) { unsigned long nr_lru; @@ -463,7 +465,7 @@ static void dump_header(struct oom_control *oc, struct task_struct *p) mem_cgroup_print_oom_meminfo(oc->memcg); else { show_mem(SHOW_MEM_FILTER_NODES, oc->nodemask); - if (is_dump_unreclaim_slabs()) + if (should_dump_unreclaim_slab()) dump_unreclaimable_slab(); } if (sysctl_oom_dump_tasks) -- 2.29.0 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v4] mm/oom_kill: change comment and rename is_dump_unreclaim_slabs() 2020-10-30 18:27 ` [PATCH v4] mm/oom_kill: change " Hui Su @ 2020-11-02 7:28 ` Michal Hocko 0 siblings, 0 replies; 8+ messages in thread From: Michal Hocko @ 2020-11-02 7:28 UTC (permalink / raw) To: Hui Su; +Cc: akpm, linux-mm, linux-kernel On Sat 31-10-20 02:27:04, Hui Su wrote: > Change the comment of is_dump_unreclaim_slabs(), it just check > whether nr_unreclaimable slabs amount is greater than user > memory, and explain why we dump unreclaim slabs. > > Rename it to should_dump_unreclaim_slab() maybe better. > > Signed-off-by: Hui Su <sh_def@163.com> Acked-by: Michal Hocko <mhocko@suse.com> Thanks! > --- > mm/oom_kill.c | 14 ++++++++------ > 1 file changed, 8 insertions(+), 6 deletions(-) > > diff --git a/mm/oom_kill.c b/mm/oom_kill.c > index 8b84661a6410..04b19b7b5435 100644 > --- a/mm/oom_kill.c > +++ b/mm/oom_kill.c > @@ -170,11 +170,13 @@ static bool oom_unkillable_task(struct task_struct *p) > return false; > } > > -/* > - * Print out unreclaimble slabs info when unreclaimable slabs amount is greater > - * than all user memory (LRU pages) > - */ > -static bool is_dump_unreclaim_slabs(void) > +/** > + * Check whether unreclaimable slab amount is greater than > + * all user memory(LRU pages). > + * dump_unreclaimable_slab() could help in the case that > + * oom due to too much unreclaimable slab used by kernel. > +*/ > +static bool should_dump_unreclaim_slab(void) > { > unsigned long nr_lru; > > @@ -463,7 +465,7 @@ static void dump_header(struct oom_control *oc, struct task_struct *p) > mem_cgroup_print_oom_meminfo(oc->memcg); > else { > show_mem(SHOW_MEM_FILTER_NODES, oc->nodemask); > - if (is_dump_unreclaim_slabs()) > + if (should_dump_unreclaim_slab()) > dump_unreclaimable_slab(); > } > if (sysctl_oom_dump_tasks) > -- > 2.29.0 > > -- Michal Hocko SUSE Labs ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-11-02 7:28 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <20201027144529.GA3558@rlk> 2020-10-27 14:58 ` [PATCH v2] mm/oom_kill.c: remove the unmatched comments Michal Hocko 2020-10-27 15:11 ` Hui Su 2020-10-27 19:23 ` Michal Hocko 2020-10-28 15:31 ` [PATCH v3] mm/oom_kill: remove comment and rename is_dump_unreclaim_slabs() Hui Su 2020-10-28 21:53 ` Andrew Morton 2020-10-29 7:51 ` Michal Hocko 2020-10-30 18:27 ` [PATCH v4] mm/oom_kill: change " Hui Su 2020-11-02 7:28 ` Michal Hocko
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).