Soft limit memory management bug fixes
Subsystems (3)
cover Soft limit memory management bug fixes
During testing of tiered memory management based on memory soft limit, I found three issues with memory management using cgroup based soft limit in the mainline code. Fix the issues with the three patches in this series.
Tim Chen (3):
mm: Fix dropped memcg from mem cgroup soft limit tree
mm: Force update of mem cgroup soft limit tree on usage excess
mm: Fix missing mem cgroup soft limit tree updates
mm/memcontrol.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)
Signature · 1 line
2.20.1
PATCH 1/3 mm: Fix dropped memcg from mem cgroup soft limit tree
During soft limit memory reclaim, we will temporarily remove the target mem cgroup from the cgroup soft limit tree. We then perform memory reclaim, update the memory usage excess count and re-insert the mem cgroup back into the mem cgroup soft limit tree according to the new memory usage excess count.
However, when memory reclaim failed for a maximum number of attempts and we bail out of the reclaim loop, we forgot to put the target mem cgroup chosen for next reclaim back to the soft limit tree. This prevented pages in the mem cgroup from being reclaimed in the future even though the mem cgroup exceeded its soft limit. Fix the logic and put the mem cgroup back on the tree when page reclaim failed for the mem cgroup.
--- mm/memcontrol.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
mm/memcontrol.c +5 -1
@@ -3505,8 +3505,12 @@ unsigned long mem_cgroup_soft_limit_reclaim(pg_data_t *pgdat, int order, loop > MEM_CGROUP_MAX_SOFT_LIMIT_RECLAIM_LOOPS)) break; }while (!nr_reclaimed); -if (next_mz) +if (next_mz) { +spin_lock_irq (&mctz->lock); +__mem_cgroup_insert_exceeded (next_mz, mctz, excess); +spin_unlock_irq (&mctz->lock);css_put (&next_mz->memcg->css); + }return nr_reclaimed; }
Signature · 1 line
2.20.1
PATCH 2/3 mm: Force update of mem cgroup soft limit tree on usage excess
In reply to Tim ChenTo rate limit updates to the mem cgroup soft limit tree, we only perform updates every SOFTLIMIT_EVENTS_TARGET (defined as 1024) memory events.
However, this sampling based updates may miss a critical update: i.e. when the mem cgroup first exceeded its limit but it was not on the soft limit tree. It should be on the tree at that point so it could be subjected to soft limit page reclaim. If the mem cgroup had few memory events compared with other mem cgroups, we may not update it and place in on the mem cgroup soft limit tree for many memory events. And this mem cgroup excess usage could creep up and the mem cgroup could be hidden from the soft limit page reclaim for a long time.
Fix this issue by forcing an update to the mem cgroup soft limit tree if a mem cgroup has exceeded its memory soft limit but it is not on the mem cgroup soft limit tree.
--- mm/memcontrol.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)
mm/memcontrol.c +9 -2
@@ -985,15 +985,22 @@ static bool mem_cgroup_event_ratelimit(struct mem_cgroup *memcg,*/ static void memcg_check_events (struct mem_cgroup *memcg,struct page *page) { +struct mem_cgroup_per_node *mz; +bool force_update = false; + + mz =mem_cgroup_nodeinfo (memcg,page_to_nid (page)); +if (mz && !mz->on_tree &&soft_limit_excess (mz->memcg) >0 ) + force_update = true; +/* threshold event is triggered in finer grain than soft limit */ -if (unlikely (mem_cgroup_event_ratelimit (memcg, +if (unlikely ((force_update) ||mem_cgroup_event_ratelimit (memcg,MEM_CGROUP_TARGET_THRESH ))) {bool do_softlimit; do_softlimit =mem_cgroup_event_ratelimit (memcg,MEM_CGROUP_TARGET_SOFTLIMIT );mem_cgroup_threshold (memcg); -if (unlikely (do_softlimit)) +if (unlikely ((force_update) || do_softlimit))mem_cgroup_update_tree (memcg, page); } }
Signature · 1 line
2.20.1
PATCH 3/3 mm: Fix missing mem cgroup soft limit tree updates
In reply to Tim ChenOn a per node basis, the mem cgroup soft limit tree on each node tracks how much a cgroup has exceeded its soft limit memory limit and sorts the cgroup by its excess usage. On page release, the trees are not updated right away, until we have gathered a batch of pages belonging to the same cgroup. This reduces the frequency of updating the soft limit tree and locking of the tree and associated cgroup.
However, the batch of pages could contain pages from multiple nodes but only the soft limit tree from one node would get updated. Change the logic so that we update the tree in batch of pages, with each batch of pages all in the same mem cgroup and memory node. An update is issued for the batch of pages of a node collected till now whenever we encounter a page belonging to a different node.
--- mm/memcontrol.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
mm/memcontrol.c +5 -1
@@ -6804,6 +6804,7 @@ struct uncharge_gather {unsigned long pgpgout;unsigned long nr_kmem;struct page *dummy_page; +int nid; };static inline void uncharge_gather_clear (struct uncharge_gather *ug)
@@ -6849,7 +6850,9 @@ static void uncharge_page(struct page *page, struct uncharge_gather *ug)* exclusive access to the page. */ -if (ug->memcg !=page_memcg (page)) { +if (ug->memcg !=page_memcg (page) || +/* uncharge batch update soft limit tree on a node basis */ + (ug->dummy_page && ug->nid !=page_to_nid (page))) {if (ug->memcg) {uncharge_batch (ug);uncharge_gather_clear (ug);
@@ -6869,6 +6872,7 @@ static void uncharge_page(struct page *page, struct uncharge_gather *ug) ug->pgpgout++; ug->dummy_page = page; + ug->nid =page_to_nid (page); page->memcg_data =0 ;css_put (&ug->memcg->css); }
Signature · 1 line
2.20.1
Johannes Weiner
maintainerRe: [PATCH 3/3] mm: Fix missing mem cgroup soft limit tree updates
Hello Tim,
mm/memcontrol.c static void uncharge_page(struct page *page, struct uncharge_gather *ug) · line 6855
Quoted 8 lines · Tim Chen
mm/memcontrol.c +3
-1@@ -6849,7 +6850,9 @@ static void uncharge_page(struct page *page, struct uncharge_gather *ug)* exclusive access to the page. */ -if (ug->memcg !=page_memcg (page)) { +if (ug->memcg !=page_memcg (page) || +/* uncharge batch update soft limit tree on a node basis */ + (ug->dummy_page && ug->nid !=page_to_nid (page))) {
The fix makes sense to me.
However, unconditionally breaking up the batch by node can unnecessarily regress workloads in cgroups that do not have a soft limit configured, and cgroup2 which doesn't have soft limits at all. Consider an interleaving allocation policy for example.
Can you please further gate on memcg->soft_limit != PAGE_COUNTER_MAX, or at least on !cgroup_subsys_on_dfl(memory_cgrp_subsys)?
Thanks
Re: [PATCH 3/3] mm: Fix missing mem cgroup soft limit tree updates
Quoted 14 lines · Johannes Weiner
Hello Tim,
mm/memcontrol.c static void uncharge_page(struct page *page, struct uncharge_gather *ug) · line 6855
Quoted 8 lines · Tim Chen
mm/memcontrol.c +3
-1@@ -6849,7 +6850,9 @@ static void uncharge_page(struct page *page, struct uncharge_gather *ug)* exclusive access to the page. */ -if (ug->memcg !=page_memcg (page)) { +if (ug->memcg !=page_memcg (page) || +/* uncharge batch update soft limit tree on a node basis */ + (ug->dummy_page && ug->nid !=page_to_nid (page))) {The fix makes sense to me.
Quoted 8 lines
However, unconditionally breaking up the batch by node can unnecessarily regress workloads in cgroups that do not have a soft limit configured, and cgroup2 which doesn't have soft limits at all. Consider an interleaving allocation policy for example.
Can you please further gate on
memcg->soft_limit!=PAGE_COUNTER_MAX, or at least on !cgroup_subsys_on_dfl(memory_cgrp_subsys)?
Sure. Will fix this.
Tim
Re: [PATCH 1/3] mm: Fix dropped memcg from mem cgroup soft limit tree
In reply to Tim ChenQuoted 13 lines · Tim Chen
During soft limit memory reclaim, we will temporarily remove the target mem cgroup from the cgroup soft limit tree. We then perform memory reclaim, update the memory usage excess count and re-insert the mem cgroup back into the mem cgroup soft limit tree according to the new memory usage excess count.
However, when memory reclaim failed for a maximum number of attempts and we bail out of the reclaim loop, we forgot to put the target mem cgroup chosen for next reclaim back to the soft limit tree. This prevented pages in the mem cgroup from being reclaimed in the future even though the mem cgroup exceeded its soft limit. Fix the logic and put the mem cgroup back on the tree when page reclaim failed for the mem cgroup.
Quoted 2 lines
Reviewed-by Ying Huang <[email protected]>
It seems this goes all the way to when it has been introduced by 4e41695356fb ("memory controller: soft limit reclaim on contention"). Please add a Fixes tag pointing to the above one. While this looks like a rare event to happen because there should be some reclaimable memory usually.
Thanks!
Quoted 24 lines
--- mm/memcontrol.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)mm/memcontrol.c +5
-1@@ -3505,8 +3505,12 @@ unsigned long mem_cgroup_soft_limit_reclaim(pg_data_t *pgdat, int order, loop > MEM_CGROUP_MAX_SOFT_LIMIT_RECLAIM_LOOPS)) break; }while (!nr_reclaimed); -if (next_mz) +if (next_mz) { +spin_lock_irq (&mctz->lock); +__mem_cgroup_insert_exceeded (next_mz, mctz, excess); +spin_unlock_irq (&mctz->lock);css_put (&next_mz->memcg->css); + }return nr_reclaimed; }Signature · 1 line
2.20.1
Signature · 2 lines
Michal Hocko SUSE Labs
Re: [PATCH 2/3] mm: Force update of mem cgroup soft limit tree on usage excess
In reply to Tim ChenQuoted 11 lines · Tim Chen
To rate limit updates to the mem cgroup soft limit tree, we only perform updates every
SOFTLIMIT_EVENTS_TARGET(defined as 1024) memory events.However, this sampling based updates may miss a critical update: i.e. when the mem cgroup first exceeded its limit but it was not on the soft limit tree. It should be on the tree at that point so it could be subjected to soft limit page reclaim. If the mem cgroup had few memory events compared with other mem cgroups, we may not update it and place in on the mem cgroup soft limit tree for many memory events. And this mem cgroup excess usage could creep up and the mem cgroup could be hidden from the soft limit page reclaim for a long time.
Have you observed this happening in the real life? I do agree that the threshold based updates of the tree is not ideal but the whole soft reclaim code is far from optimal. So why do we care only now? The feature is essentially dead and fine tuning it sounds like a step back to me.
Quoted 41 lines
Fix this issue by forcing an update to the mem cgroup soft limit tree if a mem cgroup has exceeded its memory soft limit but it is not on the mem cgroup soft limit tree.
Reviewed-by Ying Huang <[email protected]>--- mm/memcontrol.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-)mm/memcontrol.c +9
-2@@ -985,15 +985,22 @@ static bool mem_cgroup_event_ratelimit(struct mem_cgroup *memcg,*/ static void memcg_check_events (struct mem_cgroup *memcg,struct page *page) { +struct mem_cgroup_per_node *mz; +bool force_update = false; + + mz =mem_cgroup_nodeinfo (memcg,page_to_nid (page)); +if (mz && !mz->on_tree &&soft_limit_excess (mz->memcg) >0 ) + force_update = true; +/* threshold event is triggered in finer grain than soft limit */ -if (unlikely (mem_cgroup_event_ratelimit (memcg, +if (unlikely ((force_update) ||mem_cgroup_event_ratelimit (memcg,MEM_CGROUP_TARGET_THRESH ))) {bool do_softlimit; do_softlimit =mem_cgroup_event_ratelimit (memcg,MEM_CGROUP_TARGET_SOFTLIMIT );mem_cgroup_threshold (memcg); -if (unlikely (do_softlimit)) +if (unlikely ((force_update) || do_softlimit))mem_cgroup_update_tree (memcg, page); } }Signature · 1 line
2.20.1
Signature · 2 lines
Michal Hocko SUSE Labs
Re: [PATCH 3/3] mm: Fix missing mem cgroup soft limit tree updates
In reply to Tim ChenQuoted 7 lines · Tim Chen
On a per node basis, the mem cgroup soft limit tree on each node tracks how much a cgroup has exceeded its soft limit memory limit and sorts the cgroup by its excess usage. On page release, the trees are not updated right away, until we have gathered a batch of pages belonging to the same cgroup. This reduces the frequency of updating the soft limit tree and locking of the tree and associated cgroup.
Quoted 6 lines
However, the batch of pages could contain pages from multiple nodes but only the soft limit tree from one node would get updated. Change the logic so that we update the tree in batch of pages, with each batch of pages all in the same mem cgroup and memory node. An update is issued for the batch of pages of a node collected till now whenever we encounter a page belonging to a different node.
I do agree with Johannes here. This shouldn't be done unconditionally for all memcgs. Wouldn't it be much better to do the fix up in the mem_cgroup_soft_reclaim path instead. Simply check the excess before doing any reclaim?
Btw. have you seen this triggering a noticeable misbehaving? I would expect this to have a rather small effect considering how many sources of memcg_check_events we have.
Unless I have missed something this has been introduced by 747db954cab6 ("mm: memcontrol: use page lists for uncharge batching"). Please add Fixes tag as well if this is really worth fixing.
Quoted 39 lines
Reviewed-by Ying Huang <[email protected]>--- mm/memcontrol.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)mm/memcontrol.c +5
-1@@ -6804,6 +6804,7 @@ struct uncharge_gather {unsigned long pgpgout;unsigned long nr_kmem;struct page *dummy_page; +int nid; };static inline void uncharge_gather_clear (struct uncharge_gather *ug)@@ -6849,7 +6850,9 @@ static void uncharge_page(struct page *page, struct uncharge_gather *ug)* exclusive access to the page. */ -if (ug->memcg !=page_memcg (page)) { +if (ug->memcg !=page_memcg (page) || +/* uncharge batch update soft limit tree on a node basis */ + (ug->dummy_page && ug->nid !=page_to_nid (page))) {if (ug->memcg) {uncharge_batch (ug);uncharge_gather_clear (ug);@@ -6869,6 +6872,7 @@ static void uncharge_page(struct page *page, struct uncharge_gather *ug) ug->pgpgout++; ug->dummy_page = page; + ug->nid =page_to_nid (page); page->memcg_data =0 ;css_put (&ug->memcg->css); }Signature · 1 line
2.20.1
Signature · 2 lines
Michal Hocko SUSE Labs