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 <[email protected]>
>
> 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
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 <[email protected]>
Acked-by: Michal Hocko <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
Acked-by: Michal Hocko <[email protected]>
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