Re: [PATCH 4/12] memcg make page->mapping NULL before calling uncharge

[Balbir Singh <balbir@linux.vnet.ibm.com>]

KAMEZAWA Hiroyuki wrote:
> This patch tries to make page->mapping to be NULL before
> mem_cgroup_uncharge_cache_page() is called.
> 
> "page->mapping == NULL" is a good check for "whether the page is still
> radix-tree or not".
> This patch also adds BUG_ON() to mem_cgroup_uncharge_cache_page();
> 
> 
> Signed-off-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

Looks good, small nit-pick below

>  #endif
>  	ClearPagePrivate(page);
>  	set_page_private(page, 0);
> -	page->mapping = NULL;
> +	/* page->mapping contains a flag for PageAnon() */
> +	if (PageAnon(page)) {
> +		/* This page is uncharged at try_to_unmap(). */
> +		page->mapping = NULL;
> +	} else {
> +		/* Obsolete file cache should be uncharged */
> +		page->mapping = NULL;
> +		mem_cgroup_uncharge_cache_page(page);
> +	}
> 

Isn't it better and correct coding style to do

	/*
	 * Uncharge obsolete file cache
	 */
	if (!PageAnon(page))
		mem_cgroup_uncharge_cache_page(page);
	/* else - uncharged at try_to_unmap() */
	page->mapping = NULL;


-- 
	Balbir