Re: [PATCH] memcg: memswap controller core swapcache fixes

[KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>]

On Mon, 24 Nov 2008 14:43:44 +0900
KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:

> On Sun, 23 Nov 2008 22:11:07 +0000 (GMT)
> Hugh Dickins <hugh@veritas.com> wrote:
> 
> > Two SwapCache bug fixes to mmotm's memcg-memswap-controller-core.patch:
> > 
> > One bug is independent of my current changes: there is no guarantee that
> > the page passed to mem_cgroup_try_charge_swapin() is still in SwapCache.
> > 
> 
> Ah, yes. I'm wrong that the page may not be SwapCache when lock_page() is
> called...

Ah...sorry again,

> > +	/*
> > +	 * A racing thread's fault, or swapoff, may have already updated
> > +	 * the pte, and even removed page from swap cache: return success
> > +	 * to go on to do_swap_page()'s pte_same() test, which should fail.
> > +	 */
> > +	if (!PageSwapCache(page))
> > +		return 0;
> > +
> >  	ent.val = page_private(page);

I think 
==
	if (!PageSwapCache(page))
		goto charge_cur_mm;
==
is better.

In following case,
==
	CPUA				CPUB
				    remove_from_swapcache
	lock_page()                 <==========================(*)
	try_charge_swapin()          
	....
	commit_charge()
==
At (*), the page may be fully unmapped and not charged
(and PCG_USED bit is cleared.)
If so, returing without any charge here means leak of charge.

Even if *charged* here,
  - mem_cgroup_cancel_charge_swapin (handles !pte_same() case.)
  - mem_cgroup_commit_charge_swapin (handles page is doubly charged case.)

try-commit-cancel is introduced to handle this kind of case and bug in my code
is accessing page->private without checking PageSwapCache().

Thanks,
-Kame


--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>