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

[Ying Han <yinghan@google.com>]

[-- Attachment #1: Type: text/plain, Size: 4927 bytes --]

On Fri, Aug 12, 2011 at 1:34 AM, Johannes Weiner <hannes@cmpxchg.org> wrote:

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

For some reason, the first check of PageLRU was removed by some commit in my
source tree and I don't know why. Guess I have to double check w/ that.

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


Yes, leaving the LRU bit on while not linked to a LRU will cause various
problems. This is what it looks like on my tree:

-       if (!PageCgroupUsed(pc))
+       if (PageLRU(page) && !PageCgroupUsed(pc)) {
+              ClearPageLRU(page);
                del_page_from_lru(zone, page);
}
        spin_unlock_irqrestore(&zone->lru_lock, flags);

 We are working on the patch to break zone->lru_lock, and without this patch
the system crashes w/ running some swaptests. Sorry I didn't post the full
patch at the beginning since not sure the second "+" related to the lru_lock
patch or not.

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?


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

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.

I haven't got chance put up the patch doing that, and at the same time i
wonder maybe i missed something ?


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

Thank you for the detailed information :)

--Ying

[-- Attachment #2: Type: text/html, Size: 7192 bytes --]