Synopsis:
A WARN_ON_ONCE is hit twice in set_task_reclaim_state under the
following conditions:
- a memory cgroup has been created and a task assigned it it
- memory.limit_in_bytes has been set
- memory has filled up, likely from cache
In my usage, I create a cgroup under the current session scope and
assign a task to it. I then set memory.limit_in_bytes and
memory.soft_limit_in_bytes for the cgroup to reasonable values, say
1G/512M. The program accesses large files frequently and gradually
fills memory with the page cache. The warnings appears when the
entirety of the system memory is filled, presumably from other
programs.
If I wait until the program has filled the entirety of system memory
with cache and then assign a memory limit, the warnings appear
immediately.
It looks the warning is triggered because kswapd set reclaim_state then the memcg soft limit reclaim in the same kswapd set it again.
But, kswapd and memcg soft limit uses different reclaim_state from different scan control. It sounds not correct, they should use the same reclaim_state if they come from the same context if my understanding is correct.
From: Michal Hocko <mhocko@kernel.org> Date: 2019-08-26 10:55:26
On Fri 23-08-19 18:03:01, Yang Shi wrote:
On 8/23/19 3:00 PM, Adric Blake wrote:
quoted
Synopsis:
A WARN_ON_ONCE is hit twice in set_task_reclaim_state under the
following conditions:
- a memory cgroup has been created and a task assigned it it
- memory.limit_in_bytes has been set
- memory has filled up, likely from cache
In my usage, I create a cgroup under the current session scope and
assign a task to it. I then set memory.limit_in_bytes and
memory.soft_limit_in_bytes for the cgroup to reasonable values, say
1G/512M. The program accesses large files frequently and gradually
fills memory with the page cache. The warnings appears when the
entirety of the system memory is filled, presumably from other
programs.
If I wait until the program has filled the entirety of system memory
with cache and then assign a memory limit, the warnings appear
immediately.
It looks the warning is triggered because kswapd set reclaim_state then the
memcg soft limit reclaim in the same kswapd set it again.
Yes, this is indeed the case. The same seems possible from the direct
reclaim AFAICS.
But, kswapd and memcg soft limit uses different reclaim_state from different
scan control. It sounds not correct, they should use the same reclaim_state
if they come from the same context if my understanding is correct.
I haven't checked very closely and I might be wrong but setting the
reclaim state from the mem_cgroup_shrink_node doesn't make any sense in
the current code. The soft limit is always called from the global
reclaim and both kswapd and the direct reclaim already track reclaim
state correctly. We just haven't noticed until now beause the warning is
quite recent and mostly likely only few people tend to use soft limit
these days.
That being said, we should simply do this instead:
From 59d128214a62bf2d83c2a2a9cde887b4817275e7 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Mon, 26 Aug 2019 12:43:15 +0200
Subject: [PATCH] mm, memcg: do not set reclaim_state on soft limit reclaim
Adric Blake has noticed the following warning:
[38491.963105] WARNING: CPU: 7 PID: 175 at mm/vmscan.c:245 set_task_reclaim_state+0x1e/0x40
[...]
[38491.963239] Call Trace:
[38491.963246] mem_cgroup_shrink_node+0x9b/0x1d0
[38491.963250] mem_cgroup_soft_limit_reclaim+0x10c/0x3a0
[38491.963254] balance_pgdat+0x276/0x540
[38491.963258] kswapd+0x200/0x3f0
[38491.963261] ? wait_woken+0x80/0x80
[38491.963265] kthread+0xfd/0x130
[38491.963267] ? balance_pgdat+0x540/0x540
[38491.963269] ? kthread_park+0x80/0x80
[38491.963273] ret_from_fork+0x35/0x40
[38491.963276] ---[ end trace 727343df67b2398a ]---
which tells us that soft limit reclaim is about to overwrite the
reclaim_state configured up in the call chain (kswapd in this case but
the direct reclaim is equally possible). This means that reclaim stats
would get misleading once the soft reclaim returns and another reclaim
is done.
Fix the warning by dropping set_task_reclaim_state from the soft reclaim
which is always called with reclaim_state set up.
Reported-by: Adric Blake <redacted>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
mm/vmscan.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
@@ -3220,6 +3220,7 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,#ifdef CONFIG_MEMCG+/* Only used by soft limit reclaim. Do not reuse for anything else. */unsignedlongmem_cgroup_shrink_node(structmem_cgroup*memcg,gfp_tgfp_mask,boolnoswap,pg_data_t*pgdat,
@@ -3235,7 +3236,8 @@ unsigned long mem_cgroup_shrink_node(struct mem_cgroup *memcg,};unsignedlonglru_pages;-set_task_reclaim_state(current,&sc.reclaim_state);+WARN_ON_ONCE(!current->reclaim_state);+sc.gfp_mask=(gfp_mask&GFP_RECLAIM_MASK)|(GFP_HIGHUSER_MOVABLE&~GFP_RECLAIM_MASK);
@@ -3253,7 +3255,6 @@ unsigned long mem_cgroup_shrink_node(struct mem_cgroup *memcg,trace_mm_vmscan_memcg_softlimit_reclaim_end(sc.nr_reclaimed);-set_task_reclaim_state(current,NULL);*nr_scanned=sc.nr_scanned;returnsc.nr_reclaimed;
From: Michal Hocko <mhocko@kernel.org> Date: 2019-08-27 10:43:17
If there are no objection to the patch I will post it as a standalong
one.
On Mon 26-08-19 12:55:21, Michal Hocko wrote:
quoted hunk
From 59d128214a62bf2d83c2a2a9cde887b4817275e7 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Mon, 26 Aug 2019 12:43:15 +0200
Subject: [PATCH] mm, memcg: do not set reclaim_state on soft limit reclaim
Adric Blake has noticed the following warning:
[38491.963105] WARNING: CPU: 7 PID: 175 at mm/vmscan.c:245 set_task_reclaim_state+0x1e/0x40
[...]
[38491.963239] Call Trace:
[38491.963246] mem_cgroup_shrink_node+0x9b/0x1d0
[38491.963250] mem_cgroup_soft_limit_reclaim+0x10c/0x3a0
[38491.963254] balance_pgdat+0x276/0x540
[38491.963258] kswapd+0x200/0x3f0
[38491.963261] ? wait_woken+0x80/0x80
[38491.963265] kthread+0xfd/0x130
[38491.963267] ? balance_pgdat+0x540/0x540
[38491.963269] ? kthread_park+0x80/0x80
[38491.963273] ret_from_fork+0x35/0x40
[38491.963276] ---[ end trace 727343df67b2398a ]---
which tells us that soft limit reclaim is about to overwrite the
reclaim_state configured up in the call chain (kswapd in this case but
the direct reclaim is equally possible). This means that reclaim stats
would get misleading once the soft reclaim returns and another reclaim
is done.
Fix the warning by dropping set_task_reclaim_state from the soft reclaim
which is always called with reclaim_state set up.
Reported-by: Adric Blake <redacted>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
mm/vmscan.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
@@ -3220,6 +3220,7 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,#ifdef CONFIG_MEMCG+/* Only used by soft limit reclaim. Do not reuse for anything else. */unsignedlongmem_cgroup_shrink_node(structmem_cgroup*memcg,gfp_tgfp_mask,boolnoswap,pg_data_t*pgdat,
@@ -3235,7 +3236,8 @@ unsigned long mem_cgroup_shrink_node(struct mem_cgroup *memcg,};unsignedlonglru_pages;-set_task_reclaim_state(current,&sc.reclaim_state);+WARN_ON_ONCE(!current->reclaim_state);+sc.gfp_mask=(gfp_mask&GFP_RECLAIM_MASK)|(GFP_HIGHUSER_MOVABLE&~GFP_RECLAIM_MASK);
@@ -3253,7 +3255,6 @@ unsigned long mem_cgroup_shrink_node(struct mem_cgroup *memcg,trace_mm_vmscan_memcg_softlimit_reclaim_end(sc.nr_reclaimed);-set_task_reclaim_state(current,NULL);*nr_scanned=sc.nr_scanned;returnsc.nr_reclaimed;
On Tue, Aug 27, 2019 at 6:43 PM Michal Hocko [off-list ref] wrote:
If there are no objection to the patch I will post it as a standalong
one.
I have no objection to your patch. It could fix the issue.
I still think that it is not proper to use a new scan_control here as
it breaks the global reclaim context.
This context switch from global reclaim to memcg reclaim is very
subtle change to the subsequent processing, that may cause some
unexpected behavior.
Anyway, we can send this patch as a standalong one.
Feel free to add:
Acked-by: Yafang Shao <redacted>
On Mon 26-08-19 12:55:21, Michal Hocko wrote:
quoted
From 59d128214a62bf2d83c2a2a9cde887b4817275e7 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Mon, 26 Aug 2019 12:43:15 +0200
Subject: [PATCH] mm, memcg: do not set reclaim_state on soft limit reclaim
Adric Blake has noticed the following warning:
[38491.963105] WARNING: CPU: 7 PID: 175 at mm/vmscan.c:245 set_task_reclaim_state+0x1e/0x40
[...]
[38491.963239] Call Trace:
[38491.963246] mem_cgroup_shrink_node+0x9b/0x1d0
[38491.963250] mem_cgroup_soft_limit_reclaim+0x10c/0x3a0
[38491.963254] balance_pgdat+0x276/0x540
[38491.963258] kswapd+0x200/0x3f0
[38491.963261] ? wait_woken+0x80/0x80
[38491.963265] kthread+0xfd/0x130
[38491.963267] ? balance_pgdat+0x540/0x540
[38491.963269] ? kthread_park+0x80/0x80
[38491.963273] ret_from_fork+0x35/0x40
[38491.963276] ---[ end trace 727343df67b2398a ]---
which tells us that soft limit reclaim is about to overwrite the
reclaim_state configured up in the call chain (kswapd in this case but
the direct reclaim is equally possible). This means that reclaim stats
would get misleading once the soft reclaim returns and another reclaim
is done.
Fix the warning by dropping set_task_reclaim_state from the soft reclaim
which is always called with reclaim_state set up.
Reported-by: Adric Blake <redacted>
Signed-off-by: Michal Hocko <mhocko@suse.com>
---
mm/vmscan.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
@@ -3220,6 +3220,7 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,#ifdef CONFIG_MEMCG+/* Only used by soft limit reclaim. Do not reuse for anything else. */unsignedlongmem_cgroup_shrink_node(structmem_cgroup*memcg,gfp_tgfp_mask,boolnoswap,pg_data_t*pgdat,
@@ -3235,7 +3236,8 @@ unsigned long mem_cgroup_shrink_node(struct mem_cgroup *memcg,};unsignedlonglru_pages;-set_task_reclaim_state(current,&sc.reclaim_state);+WARN_ON_ONCE(!current->reclaim_state);+sc.gfp_mask=(gfp_mask&GFP_RECLAIM_MASK)|(GFP_HIGHUSER_MOVABLE&~GFP_RECLAIM_MASK);
@@ -3253,7 +3255,6 @@ unsigned long mem_cgroup_shrink_node(struct mem_cgroup *memcg,trace_mm_vmscan_memcg_softlimit_reclaim_end(sc.nr_reclaimed);-set_task_reclaim_state(current,NULL);*nr_scanned=sc.nr_scanned;returnsc.nr_reclaimed;--
From: Michal Hocko <mhocko@kernel.org> Date: 2019-08-27 11:50:18
On Tue 27-08-19 19:43:49, Yafang Shao wrote:
On Tue, Aug 27, 2019 at 6:43 PM Michal Hocko [off-list ref] wrote:
quoted
If there are no objection to the patch I will post it as a standalong
one.
I have no objection to your patch. It could fix the issue.
I still think that it is not proper to use a new scan_control here as
it breaks the global reclaim context.
This context switch from global reclaim to memcg reclaim is very
subtle change to the subsequent processing, that may cause some
unexpected behavior.
Why would it break it? Could you be more specific please?
Anyway, we can send this patch as a standalong one.
Feel free to add:
Acked-by: Yafang Shao <redacted>
On Tue, Aug 27, 2019 at 7:50 PM Michal Hocko [off-list ref] wrote:
On Tue 27-08-19 19:43:49, Yafang Shao wrote:
quoted
On Tue, Aug 27, 2019 at 6:43 PM Michal Hocko [off-list ref] wrote:
quoted
If there are no objection to the patch I will post it as a standalong
one.
I have no objection to your patch. It could fix the issue.
I still think that it is not proper to use a new scan_control here as
it breaks the global reclaim context.
This context switch from global reclaim to memcg reclaim is very
subtle change to the subsequent processing, that may cause some
unexpected behavior.
Why would it break it? Could you be more specific please?
Hmm, I have explained it when replying to Hillf's patch.
The most suspcious one is settting target_mem_cgroup here, because we
only use it to judge whether it is in global reclaim.
While the memcg softlimit reclaim is really in global reclaims.
Another example the reclaim_idx, if is not same with reclaim_idx in
page allocation context, the reclaimed pages may not be used by the
allocator, especially in the direct reclaim.
And some other things in scan_control.
quoted
Anyway, we can send this patch as a standalong one.
Feel free to add:
Acked-by: Yafang Shao <redacted>
From: Michal Hocko <mhocko@kernel.org> Date: 2019-08-27 12:03:38
On Tue 27-08-19 19:56:16, Yafang Shao wrote:
On Tue, Aug 27, 2019 at 7:50 PM Michal Hocko [off-list ref] wrote:
quoted
On Tue 27-08-19 19:43:49, Yafang Shao wrote:
quoted
On Tue, Aug 27, 2019 at 6:43 PM Michal Hocko [off-list ref] wrote:
quoted
If there are no objection to the patch I will post it as a standalong
one.
I have no objection to your patch. It could fix the issue.
I still think that it is not proper to use a new scan_control here as
it breaks the global reclaim context.
This context switch from global reclaim to memcg reclaim is very
subtle change to the subsequent processing, that may cause some
unexpected behavior.
Why would it break it? Could you be more specific please?
Hmm, I have explained it when replying to Hillf's patch.
The most suspcious one is settting target_mem_cgroup here, because we
only use it to judge whether it is in global reclaim.
While the memcg softlimit reclaim is really in global reclaims.
But we are reclaim the target_mem_cgroup hierarchy. This is the whole
point of the soft reclaim. Push down that hierarchy below the configured
limit. And that is why we absolutely have to switch the reclaim context.
Another example the reclaim_idx, if is not same with reclaim_idx in
page allocation context, the reclaimed pages may not be used by the
allocator, especially in the direct reclaim.
Again, we do not care about that as well. All we care about is to
reclaim _some_ memory to get below the soft limit. This is the semantic
that is not really great but this is how the Soft reclaim has
traditionally worked and why we keep claiming that people shouldn't
really use it. It does lead to over reclaim and that is a design rather
than a bug.
On Tue, Aug 27, 2019 at 8:03 PM Michal Hocko [off-list ref] wrote:
On Tue 27-08-19 19:56:16, Yafang Shao wrote:
quoted
On Tue, Aug 27, 2019 at 7:50 PM Michal Hocko [off-list ref] wrote:
quoted
On Tue 27-08-19 19:43:49, Yafang Shao wrote:
quoted
On Tue, Aug 27, 2019 at 6:43 PM Michal Hocko [off-list ref] wrote:
quoted
If there are no objection to the patch I will post it as a standalong
one.
I have no objection to your patch. It could fix the issue.
I still think that it is not proper to use a new scan_control here as
it breaks the global reclaim context.
This context switch from global reclaim to memcg reclaim is very
subtle change to the subsequent processing, that may cause some
unexpected behavior.
Why would it break it? Could you be more specific please?
Hmm, I have explained it when replying to Hillf's patch.
The most suspcious one is settting target_mem_cgroup here, because we
only use it to judge whether it is in global reclaim.
While the memcg softlimit reclaim is really in global reclaims.
But we are reclaim the target_mem_cgroup hierarchy. This is the whole
point of the soft reclaim. Push down that hierarchy below the configured
limit. And that is why we absolutely have to switch the reclaim context.
One obvious issue is the reclaim couters may not correct.
See shrink_inactive_list().
The pages relcaimed in memcg softlimit will not be counted to
PGSCAN_{DIRECT, KSWAPD} and
PGSTEAL_{DIRECT, KSWAPD}.
That may cause some misleading. For example, if these counters are not
changed, we will think that direct relcaim doesn't occur, while it
really occurs.
May issues are also in some other code around the usage of
global_reclaim(). I'm not sure of it.
quoted
Another example the reclaim_idx, if is not same with reclaim_idx in
page allocation context, the reclaimed pages may not be used by the
allocator, especially in the direct reclaim.
Again, we do not care about that as well. All we care about is to
reclaim _some_ memory to get below the soft limit. This is the semantic
that is not really great but this is how the Soft reclaim has
traditionally worked and why we keep claiming that people shouldn't
really use it. It does lead to over reclaim and that is a design rather
than a bug.
From: Michal Hocko <mhocko@kernel.org> Date: 2019-08-27 12:55:27
On Tue 27-08-19 20:19:34, Yafang Shao wrote:
On Tue, Aug 27, 2019 at 8:03 PM Michal Hocko [off-list ref] wrote:
quoted
On Tue 27-08-19 19:56:16, Yafang Shao wrote:
quoted
On Tue, Aug 27, 2019 at 7:50 PM Michal Hocko [off-list ref] wrote:
quoted
On Tue 27-08-19 19:43:49, Yafang Shao wrote:
quoted
On Tue, Aug 27, 2019 at 6:43 PM Michal Hocko [off-list ref] wrote:
quoted
If there are no objection to the patch I will post it as a standalong
one.
I have no objection to your patch. It could fix the issue.
I still think that it is not proper to use a new scan_control here as
it breaks the global reclaim context.
This context switch from global reclaim to memcg reclaim is very
subtle change to the subsequent processing, that may cause some
unexpected behavior.
Why would it break it? Could you be more specific please?
Hmm, I have explained it when replying to Hillf's patch.
The most suspcious one is settting target_mem_cgroup here, because we
only use it to judge whether it is in global reclaim.
While the memcg softlimit reclaim is really in global reclaims.
But we are reclaim the target_mem_cgroup hierarchy. This is the whole
point of the soft reclaim. Push down that hierarchy below the configured
limit. And that is why we absolutely have to switch the reclaim context.
One obvious issue is the reclaim couters may not correct.
See shrink_inactive_list().
The pages relcaimed in memcg softlimit will not be counted to
PGSCAN_{DIRECT, KSWAPD} and PGSTEAL_{DIRECT, KSWAPD}.
And again this a long term behavior so I would be really curious why it
is considered a bug now. Really, the semantic of the soft limit is
weird. It has been grafted into the reclaim while it was doing a
semantically different thing. It doesn't really reflect kswapd or direct
reclaim targets.
--
Michal Hocko
SUSE Labs
If there are no objection to the patch I will post it as a standalong
one.
On Mon 26-08-19 12:55:21, Michal Hocko wrote:
quoted
From 59d128214a62bf2d83c2a2a9cde887b4817275e7 Mon Sep 17 00:00:00 2001
From: Michal Hocko <mhocko@suse.com>
Date: Mon, 26 Aug 2019 12:43:15 +0200
Subject: [PATCH] mm, memcg: do not set reclaim_state on soft limit reclaim
Adric Blake has noticed the following warning:
[38491.963105] WARNING: CPU: 7 PID: 175 at mm/vmscan.c:245 set_task_reclaim_state+0x1e/0x40
[...]
[38491.963239] Call Trace:
[38491.963246] mem_cgroup_shrink_node+0x9b/0x1d0
[38491.963250] mem_cgroup_soft_limit_reclaim+0x10c/0x3a0
[38491.963254] balance_pgdat+0x276/0x540
[38491.963258] kswapd+0x200/0x3f0
[38491.963261] ? wait_woken+0x80/0x80
[38491.963265] kthread+0xfd/0x130
[38491.963267] ? balance_pgdat+0x540/0x540
[38491.963269] ? kthread_park+0x80/0x80
[38491.963273] ret_from_fork+0x35/0x40
[38491.963276] ---[ end trace 727343df67b2398a ]---
which tells us that soft limit reclaim is about to overwrite the
reclaim_state configured up in the call chain (kswapd in this case but
the direct reclaim is equally possible). This means that reclaim stats
would get misleading once the soft reclaim returns and another reclaim
is done.
Fix the warning by dropping set_task_reclaim_state from the soft reclaim
which is always called with reclaim_state set up.
This is exactly what I thought. Looks good to me. Acked-by: Yang Shi [off-list ref]
@@ -3220,6 +3220,7 @@ unsigned long try_to_free_pages(struct zonelist *zonelist, int order,#ifdef CONFIG_MEMCG+/* Only used by soft limit reclaim. Do not reuse for anything else. */unsignedlongmem_cgroup_shrink_node(structmem_cgroup*memcg,gfp_tgfp_mask,boolnoswap,pg_data_t*pgdat,
@@ -3235,7 +3236,8 @@ unsigned long mem_cgroup_shrink_node(struct mem_cgroup *memcg,};unsignedlonglru_pages;-set_task_reclaim_state(current,&sc.reclaim_state);+WARN_ON_ONCE(!current->reclaim_state);+sc.gfp_mask=(gfp_mask&GFP_RECLAIM_MASK)|(GFP_HIGHUSER_MOVABLE&~GFP_RECLAIM_MASK);
@@ -3253,7 +3255,6 @@ unsigned long mem_cgroup_shrink_node(struct mem_cgroup *memcg,trace_mm_vmscan_memcg_softlimit_reclaim_end(sc.nr_reclaimed);-set_task_reclaim_state(current,NULL);*nr_scanned=sc.nr_scanned;returnsc.nr_reclaimed;