linux-kernel - Re: [PATCH 1/3] mm, memcg: Don't put offlined memcg into local stock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YVefHLo1+6lgw3aB@carbon.dhcp.thefacebook.com>
Date:   Fri, 1 Oct 2021 16:51:56 -0700
From:   Roman Gushchin <guro@...com>
To:     Waiman Long <longman@...hat.com>
CC:     Johannes Weiner <hannes@...xchg.org>,
        Michal Hocko <mhocko@...nel.org>,
        Vladimir Davydov <vdavydov.dev@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Vlastimil Babka <vbabka@...e.cz>,
        <linux-kernel@...r.kernel.org>, <cgroups@...r.kernel.org>,
        <linux-mm@...ck.org>, Shakeel Butt <shakeelb@...gle.com>,
        Muchun Song <songmuchun@...edance.com>
Subject: Re: [PATCH 1/3] mm, memcg: Don't put offlined memcg into local stock

On Fri, Oct 01, 2021 at 03:09:36PM -0400, Waiman Long wrote:
> When freeing a page associated with an offlined memcg, refill_stock()
> will put it into local stock delaying its demise until another memcg
> comes in to take its place in the stock. To avoid that, we now check
> for offlined memcg and go directly in this case to the slowpath for
> the uncharge via the repurposed cancel_charge() function.

Hi Waiman!

I'm afraid it can make a cleanup of a dying cgroup slower: for every
released page we'll potentially traverse the whole cgroup tree and
decrease atomic page counters.

I'm not sure I understand the benefits we get from this change which
do justify the slowdown on the cleanup path.

Thanks!

> 
> Signed-off-by: Waiman Long <longman@...hat.com>
> ---
>  mm/memcontrol.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 4b32896d87a2..4568363062c1 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -2167,6 +2167,8 @@ static bool consume_stock(struct mem_cgroup *memcg, unsigned int nr_pages)
>  	return ret;
>  }
>  
> +static void cancel_charge(struct mem_cgroup *memcg, unsigned int nr_pages);
> +
>  /*
>   * Returns stocks cached in percpu and reset cached information.
>   */
> @@ -2178,9 +2180,7 @@ static void drain_stock(struct memcg_stock_pcp *stock)
>  		return;
>  
>  	if (stock->nr_pages) {
> -		page_counter_uncharge(&old->memory, stock->nr_pages);
> -		if (do_memsw_account())
> -			page_counter_uncharge(&old->memsw, stock->nr_pages);
> +		cancel_charge(old, stock->nr_pages);
>  		stock->nr_pages = 0;
>  	}
>  
> @@ -2219,6 +2219,14 @@ static void refill_stock(struct mem_cgroup *memcg, unsigned int nr_pages)
>  	struct memcg_stock_pcp *stock;
>  	unsigned long flags;
>  
> +	/*
> +	 * An offlined memcg shouldn't be put into stock.
> +	 */
> +	if (unlikely(memcg->kmem_state != KMEM_ONLINE)) {
> +		cancel_charge(memcg, nr_pages);
> +		return;
> +	}
> +
>  	local_irq_save(flags);
>  
>  	stock = this_cpu_ptr(&memcg_stock);
> @@ -2732,7 +2740,6 @@ static inline int try_charge(struct mem_cgroup *memcg, gfp_t gfp_mask,
>  	return try_charge_memcg(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)
>  {
>  	if (mem_cgroup_is_root(memcg))
> @@ -2742,7 +2749,6 @@ static void cancel_charge(struct mem_cgroup *memcg, unsigned int nr_pages)
>  	if (do_memsw_account())
>  		page_counter_uncharge(&memcg->memsw, nr_pages);
>  }
> -#endif
>  
>  static void commit_charge(struct folio *folio, struct mem_cgroup *memcg)
>  {
> -- 
> 2.18.1
>