Re: [PATCH] fix unused/stale swap cache handling on memcg v3

[Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp>]

On Mon, 23 Mar 2009 14:22:42 +0900, KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com> wrote:
> On Mon, 23 Mar 2009 14:04:19 +0900
> Daisuke Nishimura <nishimura@mxp.nes.nec.co.jp> wrote:
> 
> > > Nice clean-up here :)
> > > 
> > Thanks, I'll send a cleanup patch for this part later.
> > 
> Thank you, I'll look into.
> 
> > > > @@ -1359,18 +1373,40 @@ charge_cur_mm:
> > > >  	return __mem_cgroup_try_charge(mm, mask, ptr, true);
> > > >  }
> > > >  
> > > > -void mem_cgroup_commit_charge_swapin(struct page *page, struct mem_cgroup *ptr)
> > > > +static void
> > > > +__mem_cgroup_commit_charge_swapin(struct page *page, struct mem_cgroup *ptr,
> > > > +					enum charge_type ctype)
> > > >  {
> > > > -	struct page_cgroup *pc;
> > > > +	unsigned long flags;
> > > > +	struct zone *zone = page_zone(page);
> > > > +	struct page_cgroup *pc = lookup_page_cgroup(page);
> > > > +	int locked = 0;
> > > >  
> > > >  	if (mem_cgroup_disabled())
> > > >  		return;
> > > >  	if (!ptr)
> > > >  		return;
> > > > -	pc = lookup_page_cgroup(page);
> > > > -	mem_cgroup_lru_del_before_commit_swapcache(page);
> > > > -	__mem_cgroup_commit_charge(ptr, pc, MEM_CGROUP_CHARGE_TYPE_MAPPED);
> > > > -	mem_cgroup_lru_add_after_commit_swapcache(page);
> > > > +
> > > > +	/*
> > > > +	 * Forget old LRU when this page_cgroup is *not* used. This Used bit
> > > > +	 * is guarded by lock_page() because the page is SwapCache.
> > > > +	 * If this pc is on orphan LRU, it is also removed from orphan LRU here.
> > > > +	 */
> > > > +	if (!PageCgroupUsed(pc)) {
> > > > +		locked = 1;
> > > > +		spin_lock_irqsave(&zone->lru_lock, flags);
> > > > +		mem_cgroup_del_lru_list(page, page_lru(page));
> > > > +	}
> > > Maybe nice. I tried to use lock_page_cgroup() in add_list but I can't ;(
> > > I think this works well. But I wonder...why you have to check PageCgroupUsed() ?
> > > And is it correct ? Removing PageCgroupUsed() bit check is nice.
> > > (This will be "usually returns true" check, anyway)
> > > 
> > I've just copied lru_del_before_commit_swapcache.
> > 
> ya, considering now, it seems to be silly quick-hack.
> 
> > As you say, this check will return false only in (C) case in memcg_test.txt,
> > and even in (C) case calling mem_cgroup_del_lru_list(and mem_cgroup_add_lru_list later)
> > would be no problem.
> > 
> > OK, I'll remove this check.
> > 
> Thanks,
> 
> > This is the updated version(w/o cache_charge cleanup).
> > 
> > BTW, Should I merge reclaim part based on your patch and post it ?
> > 
> I think not necessary. keeping changes minimum is important as BUGFIX.
> We can visit here again when new -RC stage starts.
> 
> no problem from my review.
> Acked-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>
> 
Just FYI, this version of orphan list framework works fine
w/o causing BUG more than 24h.

So, I believe we can implement reclaim part based on this
to fix the original problem.


Thanks,
Daisuke Nishimura.

--
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>