[PATCH] mm: memcontrol: fix root_mem_cgroup charging

[PATCH] mm: memcontrol: fix root_mem_cgroup charging

[Muchun Song]

CPU0:                                   CPU1:

objcg = get_obj_cgroup_from_current();
obj_cgroup_charge(objcg);
                                        memcg_reparent_objcgs();
                                            xchg(&objcg->memcg, root_mem_cgroup);
    // memcg == root_mem_cgroup
    memcg = obj_cgroup_memcg(objcg);
    __memcg_kmem_charge(memcg);
        // Do not charge to the root memcg
        try_charge(memcg);

If the objcg->memcg is reparented to the root_mem_cgroup,
obj_cgroup_charge() can pass root_mem_cgroup as the first
parameter to here. The root_mem_cgroup is skipped in the
try_charge(). So the page counters of it do not update.

When we uncharge this, we will decrease the page counters
(e.g. memory and memsw) of the root_mem_cgroup. This will
cause the page counters of the root_mem_cgroup to be out
of balance. Fix it by charging the page to the
root_mem_cgroup unconditional.

Fixes: bf4f059954dc ("mm: memcg/slab: obj_cgroup API")
Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
 mm/memcontrol.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 2db2aeac8a9e..edf604824d63 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3078,6 +3078,19 @@ static int __memcg_kmem_charge(struct mem_cgroup *memcg, gfp_t gfp,
 	if (ret)
 		return ret;
 
+	/*
+	 * If the objcg->memcg is reparented to the root_mem_cgroup,
+	 * obj_cgroup_charge() can pass root_mem_cgroup as the first
+	 * parameter to here. We should charge the page to the
+	 * root_mem_cgroup unconditional to keep it's page counters
+	 * balance.
+	 */
+	if (unlikely(mem_cgroup_is_root(memcg))) {
+		page_counter_charge(&memcg->memory, nr_pages);
+		if (do_memsw_account())
+			page_counter_charge(&memcg->memsw, nr_pages);
+	}
+
 	if (!cgroup_subsys_on_dfl(memory_cgrp_subsys) &&
 	    !page_counter_try_charge(&memcg->kmem, nr_pages, &counter)) {
 
-- 
2.11.0

Re: [External] Re: [PATCH] mm: memcontrol: fix root_mem_cgroup charging

[Muchun Song]

On Wed, Mar 3, 2021 at 2:58 AM Roman Gushchin <guro@fb.com> wrote:
>
> On Tue, Mar 02, 2021 at 04:18:23PM +0800, Muchun Song wrote:
> > CPU0:                                   CPU1:
> >
> > objcg = get_obj_cgroup_from_current();
> > obj_cgroup_charge(objcg);
> >                                         memcg_reparent_objcgs();
> >                                             xchg(&objcg->memcg, root_mem_cgroup);
> >     // memcg == root_mem_cgroup
> >     memcg = obj_cgroup_memcg(objcg);
> >     __memcg_kmem_charge(memcg);
> >         // Do not charge to the root memcg
> >         try_charge(memcg);
> >
> > If the objcg->memcg is reparented to the root_mem_cgroup,
> > obj_cgroup_charge() can pass root_mem_cgroup as the first
> > parameter to here. The root_mem_cgroup is skipped in the
> > try_charge(). So the page counters of it do not update.
> >
> > When we uncharge this, we will decrease the page counters
> > (e.g. memory and memsw) of the root_mem_cgroup. This will
> > cause the page counters of the root_mem_cgroup to be out
> > of balance. Fix it by charging the page to the
> > root_mem_cgroup unconditional.
>
> Is this a problem? It seems that we do not expose root memcg's counters
> except kmem and tcp.

In the page_counter_cancel(), we can see a WARN_ON_ONCE()
to catch this issue. Yeah, it is very hard to trigger this warn for
root memcg. But it actually can. Right?

If we do not care about the root memcg counter, we should not warn
for the root memcg.

> It seems that the described problem is not
> applicable to the kmem counter. Please, explain.

The kmem counter of the root memcg is updated unconditionally.
Because we do not check whether the memcg is root when we
charge pages to the kmem counter.

Thanks.

>
> Thanks!
>
> >
> > Fixes: bf4f059954dc ("mm: memcg/slab: obj_cgroup API")
> > Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> > ---
> >  mm/memcontrol.c | 13 +++++++++++++
> >  1 file changed, 13 insertions(+)
> >
> > diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> > index 2db2aeac8a9e..edf604824d63 100644
> > --- a/mm/memcontrol.c
> > +++ b/mm/memcontrol.c
> > @@ -3078,6 +3078,19 @@ static int __memcg_kmem_charge(struct mem_cgroup *memcg, gfp_t gfp,
> >       if (ret)
> >               return ret;
> >
> > +     /*
> > +      * If the objcg->memcg is reparented to the root_mem_cgroup,
> > +      * obj_cgroup_charge() can pass root_mem_cgroup as the first
> > +      * parameter to here. We should charge the page to the
> > +      * root_mem_cgroup unconditional to keep it's page counters
> > +      * balance.
> > +      */
> > +     if (unlikely(mem_cgroup_is_root(memcg))) {
> > +             page_counter_charge(&memcg->memory, nr_pages);
> > +             if (do_memsw_account())
> > +                     page_counter_charge(&memcg->memsw, nr_pages);
> > +     }
> > +
> >       if (!cgroup_subsys_on_dfl(memory_cgrp_subsys) &&
> >           !page_counter_try_charge(&memcg->kmem, nr_pages, &counter)) {
> >
> > --
> > 2.11.0
> >

Re: [PATCH] mm: memcontrol: fix root_mem_cgroup charging

[Michal Hocko]

On Wed 21-04-21 14:26:44, Muchun Song wrote:
> The below scenario can cause the page counters of the root_mem_cgroup
> to be out of balance.
> 
> CPU0:                                   CPU1:
> 
> objcg = get_obj_cgroup_from_current()
> obj_cgroup_charge_pages(objcg)
>                                         memcg_reparent_objcgs()
>                                             // reparent to root_mem_cgroup
>                                             WRITE_ONCE(iter->memcg, parent)
>     // memcg == root_mem_cgroup
>     memcg = get_mem_cgroup_from_objcg(objcg)
>     // do not charge to the root_mem_cgroup
>     try_charge(memcg)
> 
> obj_cgroup_uncharge_pages(objcg)
>     memcg = get_mem_cgroup_from_objcg(objcg)
>     // uncharge from the root_mem_cgroup
>     page_counter_uncharge(&memcg->memory)
> 
> This can cause the page counter to be less than the actual value,
> Although we do not display the value (mem_cgroup_usage) so there
> shouldn't be any actual problem, but there is a WARN_ON_ONCE in
> the page_counter_cancel(). Who knows if it will trigger? So it
> is better to fix it.

The changelog doesn't explain the fix and why you have chosen to charge
kmem objects to root memcg and left all other try_charge users intact.
The reason is likely that those are not reparented now but that just
adds an inconsistency.

Is there any reason you haven't simply matched obj_cgroup_uncharge_pages
to check for the root memcg and bail out early?

> Signed-off-by: Muchun Song <songmuchun@bytedance.com>
> ---
>  mm/memcontrol.c | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 1e68a9992b01..81b54bd9b9e0 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2686,8 +2686,8 @@ void mem_cgroup_handle_over_high(void)
>  	css_put(&memcg->css);
>  }
>  
> -static int try_charge(struct mem_cgroup *memcg, gfp_t gfp_mask,
> -		      unsigned int nr_pages)
> +static int __try_charge(struct mem_cgroup *memcg, gfp_t gfp_mask,
> +			unsigned int nr_pages)
>  {
>  	unsigned int batch = max(MEMCG_CHARGE_BATCH, nr_pages);
>  	int nr_retries = MAX_RECLAIM_RETRIES;
> @@ -2699,8 +2699,6 @@ static int try_charge(struct mem_cgroup *memcg, gfp_t gfp_mask,
>  	bool drained = false;
>  	unsigned long pflags;
>  
> -	if (mem_cgroup_is_root(memcg))
> -		return 0;
>  retry:
>  	if (consume_stock(memcg, nr_pages))
>  		return 0;
> @@ -2880,6 +2878,15 @@ static int try_charge(struct mem_cgroup *memcg, gfp_t gfp_mask,
>  	return 0;
>  }
>  
> +static inline int try_charge(struct mem_cgroup *memcg, gfp_t gfp_mask,
> +			     unsigned int nr_pages)
> +{
> +	if (mem_cgroup_is_root(memcg))
> +		return 0;
> +
> +	return __try_charge(memcg, gfp_mask, nr_pages);
> +}
> +
>  #if defined(CONFIG_MEMCG_KMEM) || defined(CONFIG_MMU)
>  static void cancel_charge(struct mem_cgroup *memcg, unsigned int nr_pages)
>  {
> @@ -3125,7 +3132,7 @@ static int obj_cgroup_charge_pages(struct obj_cgroup *objcg, gfp_t gfp,
>  
>  	memcg = get_mem_cgroup_from_objcg(objcg);
>  
> -	ret = try_charge(memcg, gfp, nr_pages);
> +	ret = __try_charge(memcg, gfp, nr_pages);
>  	if (ret)
>  		goto out;
>  
> -- 
> 2.11.0

-- 
Michal Hocko
SUSE Labs

[PATCH] mm: memcontrol: fix root_mem_cgroup charging

[Muchun Song]

The below scenario can cause the page counters of the root_mem_cgroup
to be out of balance.

CPU0:                                   CPU1:

objcg = get_obj_cgroup_from_current()
obj_cgroup_charge_pages(objcg)
                                        memcg_reparent_objcgs()
                                            // reparent to root_mem_cgroup
                                            WRITE_ONCE(iter->memcg, parent)
    // memcg == root_mem_cgroup
    memcg = get_mem_cgroup_from_objcg(objcg)
    // do not charge to the root_mem_cgroup
    try_charge(memcg)

obj_cgroup_uncharge_pages(objcg)
    memcg = get_mem_cgroup_from_objcg(objcg)
    // uncharge from the root_mem_cgroup
    page_counter_uncharge(&memcg->memory)

This can cause the page counter to be less than the actual value,
Although we do not display the value (mem_cgroup_usage) so there
shouldn't be any actual problem, but there is a WARN_ON_ONCE in
the page_counter_cancel(). Who knows if it will trigger? So it
is better to fix it.

Signed-off-by: Muchun Song <songmuchun@bytedance.com>
---
 mm/memcontrol.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 1e68a9992b01..81b54bd9b9e0 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2686,8 +2686,8 @@ void mem_cgroup_handle_over_high(void)
 	css_put(&memcg->css);
 }
 
-static int try_charge(struct mem_cgroup *memcg, gfp_t gfp_mask,
-		      unsigned int nr_pages)
+static int __try_charge(struct mem_cgroup *memcg, gfp_t gfp_mask,
+			unsigned int nr_pages)
 {
 	unsigned int batch = max(MEMCG_CHARGE_BATCH, nr_pages);
 	int nr_retries = MAX_RECLAIM_RETRIES;
@@ -2699,8 +2699,6 @@ static int try_charge(struct mem_cgroup *memcg, gfp_t gfp_mask,
 	bool drained = false;
 	unsigned long pflags;
 
-	if (mem_cgroup_is_root(memcg))
-		return 0;
 retry:
 	if (consume_stock(memcg, nr_pages))
 		return 0;
@@ -2880,6 +2878,15 @@ static int try_charge(struct mem_cgroup *memcg, gfp_t gfp_mask,
 	return 0;
 }
 
+static inline int try_charge(struct mem_cgroup *memcg, gfp_t gfp_mask,
+			     unsigned int nr_pages)
+{
+	if (mem_cgroup_is_root(memcg))
+		return 0;
+
+	return __try_charge(memcg, gfp_mask, nr_pages);
+}
+
 #if defined(CONFIG_MEMCG_KMEM) || defined(CONFIG_MMU)
 static void cancel_charge(struct mem_cgroup *memcg, unsigned int nr_pages)
 {
@@ -3125,7 +3132,7 @@ static int obj_cgroup_charge_pages(struct obj_cgroup *objcg, gfp_t gfp,
 
 	memcg = get_mem_cgroup_from_objcg(objcg);
 
-	ret = try_charge(memcg, gfp, nr_pages);
+	ret = __try_charge(memcg, gfp, nr_pages);
 	if (ret)
 		goto out;
 
-- 
2.11.0