| Message ID | 20230419092007.186938-1-haifeng.xu@shopee.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm, vmpressure: remove redundant check in vmpressure() | expand |
On Wed, 19 Apr 2023 09:20:07 +0000 Haifeng Xu <haifeng.xu@shopee.com> wrote: > There are three places, vmpressure_prio(), shrink_node_memcgs() and > shrink_node(), which invoke vmpressure(). But only shrink_node_memcgs() > sets tree to false and the memcg used in it is not NULL, so we don't > check it again in vmpressure(). > > ... > > --- a/mm/vmpressure.c > +++ b/mm/vmpressure.c > @@ -284,7 +284,7 @@ void vmpressure(gfp_t gfp, struct mem_cgroup *memcg, bool tree, > enum vmpressure_levels level; > > /* For now, no users for root-level efficiency */ > - if (!memcg || mem_cgroup_is_root(memcg)) > + if (mem_cgroup_is_root(memcg)) > return; > > spin_lock(&vmpr->sr_lock); try_to_free_pages()-> do_try_to_free_pages()-> vmpressure_prio()-> vmpressure()-> crash what am I missing here? It does appear that vmpressure() could be simplified with if (!memcg) memcg = root_mem_cgroup; so the test you identified goes away and the memcg_to_vmpressure() call becomes simpler. But that's such a small change it doesn't seem worth the effort.
On 2023/4/20 04:03, Andrew Morton wrote: > On Wed, 19 Apr 2023 09:20:07 +0000 Haifeng Xu <haifeng.xu@shopee.com> wrote: > >> There are three places, vmpressure_prio(), shrink_node_memcgs() and >> shrink_node(), which invoke vmpressure(). But only shrink_node_memcgs() >> sets tree to false and the memcg used in it is not NULL, so we don't >> check it again in vmpressure(). >> >> ... >> >> --- a/mm/vmpressure.c >> +++ b/mm/vmpressure.c >> @@ -284,7 +284,7 @@ void vmpressure(gfp_t gfp, struct mem_cgroup *memcg, bool tree, >> enum vmpressure_levels level; >> >> /* For now, no users for root-level efficiency */ >> - if (!memcg || mem_cgroup_is_root(memcg)) >> + if (mem_cgroup_is_root(memcg)) >> return; >> >> spin_lock(&vmpr->sr_lock); > > try_to_free_pages()-> > do_try_to_free_pages()-> > vmpressure_prio()-> > vmpressure()-> > crash > > what am I missing here? > > vmpressure_prio() set tree to true, so the crash won't happen. > It does appear that vmpressure() could be simplified with > > if (!memcg) > memcg = root_mem_cgroup; > > so the test you identified goes away and the memcg_to_vmpressure() call > becomes simpler. But that's such a small change it doesn't seem worth > the effort. memcg_to_vmpressure() has taken this case that the memcg is NULL into account and we do not need to change it. The test I identified is in the branch that the tree is set to false. In this branch, memcg can never be NULL because only shrink_node_memcgs() will step into it.
diff --git a/mm/vmpressure.c b/mm/vmpressure.c index b52644771cc4..3e4251a55e56 100644 --- a/mm/vmpressure.c +++ b/mm/vmpressure.c @@ -284,7 +284,7 @@ void vmpressure(gfp_t gfp, struct mem_cgroup *memcg, bool tree, enum vmpressure_levels level; /* For now, no users for root-level efficiency */ - if (!memcg || mem_cgroup_is_root(memcg)) + if (mem_cgroup_is_root(memcg)) return; spin_lock(&vmpr->sr_lock);
There are three places, vmpressure_prio(), shrink_node_memcgs() and shrink_node(), which invoke vmpressure(). But only shrink_node_memcgs() sets tree to false and the memcg used in it is not NULL, so we don't check it again in vmpressure(). Signed-off-by: Haifeng Xu <haifeng.xu@shopee.com> --- mm/vmpressure.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)