Re: [patch 7/9] mm: memcontrol: do not acquire page_cgroup lock for kmem pages

[Michal Hocko <mhocko@suse.cz>]

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@cmpxchg.org>

The patch is correct I just have some comments below.
Anyway
Acked-by: Michal Hocko <mhocko@suse.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