Re: [patch 8/8] mm: make per-memcg lru lists exclusive

[Johannes Weiner <hannes@cmpxchg.org>]

On Fri, Aug 12, 2011 at 10:08:18AM -0700, Ying Han wrote:
> On Fri, Aug 12, 2011 at 1:34 AM, Johannes Weiner <hannes@cmpxchg.org> wrote:
> > And in reality, we only care about properly memcg-unaccounting the
> > old lru state before we change pc->mem_cgroup, so this becomes
> >
> >        if (!PageLRU(page))
> >                 return;
> >        spin_lock_irqsave(&zone->lru_lock, flags);
> >         if (!PageCgroupUsed(pc))
> >                mem_cgroup_lru_del(page);
> >         spin_unlock_irqrestore(&zone->lru_lock, flags);
> >
> > I don't see why we should care if the page stays physically linked
> > to the list.
> 
> Can you clarify that?

Well, I don't see anything wrong with leaving it on the LRU.  We just
need to unaccount the page from pc->mem_cgroup's lru stats before the
page is charged, pc->mem_cgroup overwritten, and the account lost.

> > The handling after committing the charge becomes this:
> >
> > -       if (likely(!PageLRU(page)))
> > -               return;
> >        spin_lock_irqsave(&zone->lru_lock, flags);
> >         lru = page_lru(page);
> >        if (PageLRU(page) && !PageCgroupAcctLRU(pc)) {
> >                del_page_from_lru_list(zone, page, lru);
> >                add_page_to_lru_list(zone, page, lru);
> >        }
> >
> > If the page is not on the LRU, someone else will put it there and link
> > it up properly.  If it is on the LRU and already memcg-accounted then
> > it must be on the right lruvec as setting pc->mem_cgroup and PCG_USED
> > is properly ordered.  Otherwise, it has to be physically moved to the
> > correct lruvec and memcg-accounted for.
> 
> While working on the zone->lru_lock patch, i have been questioning myself on
> the PageLRU and PageCgroupAcctLRU bit. Here is my question:
> 
> It looks to me that PageLRU indicates the page is linked to per-zone lru
> list, and PageCgroupAcctLRU indicates the page is charged to a memcg and
> also linked to memcg's private lru list. All of these work nicely when we
> have both global and private (per-memcg) lru list, but i can not put them
> together after this patch.
> 
> Now page is linked to private lru always either memcg or root. While linked
> to either lru list, the page could be uncharged (like swapcache). No matter
> what, i am thinking whether or not we can get rid of the AcctLRU bit from pc
> and use LRU bit only here.

As I said above: if after the commit the page is on the LRU (PageLRU
set), pc->mem_cgroup's lru stats may or may not include the page, and
the page may or may not be on the right lruvec.

If someone had the page isolated (reclaim?) while we charge it and put
it back, the page may either be charged or uncharged at the time of
putback.

	unused: PageLRU is set, but page possibly on the wrong lruvec
	(root_mem_cgroup's per default, see mem_cgroup_lru_add_list)
	and not properly accounted for.  We can detect this case by
	seeing AcctLRU cleared.

	used: PageLRU is set, page on the right lruvec and properly
	accounted.  We can detect this case by seeing that
	mem_cgroup_lru_add_list() set AcctLRU.

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>