Re: [PATCH v2] mm/oom_kill.c: remove the unmatched comments

Re: [PATCH v2] mm/oom_kill.c: remove the unmatched comments

[Michal Hocko]

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

Re: [PATCH v2] mm/oom_kill.c: remove the unmatched comments

[Hui Su]

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.
> "
> 

Re: [PATCH v2] mm/oom_kill.c: remove the unmatched comments

[Michal Hocko]

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

[PATCH v3] mm/oom_kill: remove comment and rename is_dump_unreclaim_slabs()

[Hui Su]

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

Re: [PATCH v3] mm/oom_kill: remove comment and rename is_dump_unreclaim_slabs()

[Andrew Morton]

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!

Re: [PATCH v3] mm/oom_kill: remove comment and rename is_dump_unreclaim_slabs()

[Michal Hocko]

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

[PATCH v4] mm/oom_kill: change comment and rename is_dump_unreclaim_slabs()

[Hui Su]

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

Re: [PATCH v4] mm/oom_kill: change comment and rename is_dump_unreclaim_slabs()

[Michal Hocko]

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