linux-kernel - Re: [patch 7/9] mm: memcontrol: do not acquire page

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: <20140523133938.GC22135@dhcp22.suse.cz>
Date:	Fri, 23 May 2014 15:39:38 +0200
From:	Michal Hocko <mhocko@...e.cz>
To:	Johannes Weiner <hannes@...xchg.org>
Cc:	linux-mm@...ck.org, Hugh Dickins <hughd@...gle.com>,
	Tejun Heo <tj@...nel.org>, cgroups@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [patch 7/9] mm: memcontrol: do not acquire page_cgroup lock for
 kmem pages

I am adding Vladimir to CC

On Wed 30-04-14 16:25:41, Johannes Weiner wrote:
> Kmem page charging and uncharging is serialized by means of exclusive
> access to the page.  Do not take the page_cgroup lock and don't set
> pc->flags atomically.
> 
> Signed-off-by: Johannes Weiner <hannes@...xchg.org>

The patch is correct I just have some comments below.
Anyway
Acked-by: Michal Hocko <mhocko@...e.cz>

> ---
>  mm/memcontrol.c | 16 +++-------------
>  1 file changed, 3 insertions(+), 13 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index c528ae9ac230..d3961fce1d54 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3535,10 +3535,8 @@ void __memcg_kmem_commit_charge(struct page *page, struct mem_cgroup *memcg,
>  	}
>  

	/*
	 * given page is newly allocated and invisible to everybody but
	 * the caller so there is no need to use page_cgroup lock nor
	 * SetPageCgroupUsed
	 */

would be helpful?

>  	pc = lookup_page_cgroup(page);
> -	lock_page_cgroup(pc);
>  	pc->mem_cgroup = memcg;
> -	SetPageCgroupUsed(pc);
> -	unlock_page_cgroup(pc);
> +	pc->flags = PCG_USED;
>  }
>  
>  void __memcg_kmem_uncharge_pages(struct page *page, int order)
> @@ -3548,19 +3546,11 @@ void __memcg_kmem_uncharge_pages(struct page *page, int order)
>  
>  
>  	pc = lookup_page_cgroup(page);
> -	/*
> -	 * Fast unlocked return. Theoretically might have changed, have to
> -	 * check again after locking.
> -	 */

This comment was there since the code has been merged. Maybe it was true
at the time but after "mm: get rid of __GFP_KMEMCG" it is definitely out
of date.

	/*
	 * the pages is going away and will be freed and nobody can see
	 * it anymore so no need to take page_cgroup lock.
	 */
>  	if (!PageCgroupUsed(pc))
>  		return;
>  
> -	lock_page_cgroup(pc);
> -	if (PageCgroupUsed(pc)) {
> -		memcg = pc->mem_cgroup;
> -		ClearPageCgroupUsed(pc);
> -	}
> -	unlock_page_cgroup(pc);

maybe add
	WARN_ON_ONCE(pc->flags != PCG_USED);

to check for an unexpected flags usage in the kmem path?

> +	memcg = pc->mem_cgroup;
> +	pc->flags = 0;
>  
>  	/*
>  	 * We trust that only if there is a memcg associated with the page, it
> -- 
> 1.9.2
> 

-- 
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/