Message-ID: <20110812083458.GB6916@cmpxchg.org>
Date:	Fri, 12 Aug 2011 10:34:58 +0200
From:	Johannes Weiner <hannes@...xchg.org>
To:	Ying Han <yinghan@...gle.com>
Cc:	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	Daisuke Nishimura <nishimura@....nes.nec.co.jp>,
	Balbir Singh <balbir@...ux.vnet.ibm.com>,
	Michal Hocko <mhocko@...e.cz>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Rik van Riel <riel@...hat.com>,
	Minchan Kim <minchan.kim@...il.com>,
	KOSAKI Motohiro <kosaki.motohiro@...fujitsu.com>,
	Mel Gorman <mgorman@...e.de>, Greg Thelen <gthelen@...gle.com>,
	Michel Lespinasse <walken@...gle.com>, linux-mm@...ck.org,
	linux-kernel@...r.kernel.org
Subject: Re: [patch 8/8] mm: make per-memcg lru lists exclusive

On Thu, Aug 11, 2011 at 01:33:05PM -0700, Ying Han wrote:
> > Johannes, I wonder if we should include the following patch:
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 674823e..1513deb 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -832,7 +832,7 @@ static void
> mem_cgroup_lru_del_before_commit_swapcache(struct page *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 (!PageCgroupUsed(pc))
> +       if (PageLRU(page) && !PageCgroupUsed(pc))
>                 del_page_from_lru(zone, page);
>         spin_unlock_irqrestore(&zone->lru_lock, flags);

Yes, as the first PageLRU check is outside the lru_lock, PageLRU may
indeed go away before grabbing the lock.  The page will already be
unlinked and the LRU accounting will be off.

The deeper problem, however, is that del_page_from_lru is wrong.  We
can not keep the page off the LRU while leaving PageLRU set, or it
won't be very meaningful after the commit, anyway.  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.  The PageLRU check outside the lock is still fine as the
accounting has been done already if !PageLRU and a putback without
PageCgroupUsed will not re-account to pc->mem_cgroup, as the comment
above this code explains nicely.

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.

The old unlocked PageLRU check in after_commit is no longer possible
because setting PG_lru is not ordered against setting the list head,
which means the page could be linked to the wrong lruvec while this
CPU would not yet observe PG_lru and do the relink.  So this needs
strong ordering.  Given that this code is hairy enough as it is, I
just removed the preliminary check for now and do the check only under
the lock instead of adding barriers here and to the lru linking sites.

Thanks for making me write this out, few thinks put one's
understanding of a problem to the test like this.

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

Powered by blists - more mailing lists