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

[KAMEZAWA Hiroyuki]

On Fri, 26 Sep 2008 15:17:48 +0530
Balbir Singh <balbir@xxxxxxxxxxxxxxxxxx> wrote:

> 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@xxxxxxxxxxxxxx>
> 
> 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;
> 
yea, maybe.
I always wonder what I should do when I want to add comment to if-then-else...

But ok, will remove {}.

Thanks,
-Kame



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