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

[Ying Han <yinghan@google.com>]

On Fri, Aug 12, 2011 at 12:17 PM, Johannes Weiner <hannes@cmpxchg.org> wrote:
> 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.

Thank you and this is a good example.

So PageLRU bit is consistent w/ whether or not the page is linked  to
a lru list (root or memcg), and AcctLRU indicates more on the memcg
charge/uncharge.

Here I am trying to summarize the possibilities of different flags of
a page linked to a lru list ( based on the implementation after this
patch series). please help to correct :

root lru:
1. PageLRU, Used, AcctLRU: page charged to root and linked to root lru
list. ( ext: page allocated under root cgroup )

2. PageLRU, !Used, !AcctLRU: page not charged and linked to root lru
list. ( ext: page uncharged before free, or like readahead swapcache)

3. PageLRU, Used, !AcctLRU: page del from root lru before uncharge, or
charged before add to root lru

4. PageLRU, !Used, AcctLRU: page uncharged before del from root lru
(ext: swapcache)

non-root lru:

1. PageLRU, Used, AcctLRU: page charged to memcg and linked to memcg lru list

2. PageLRU, !Used, !AcctLRU: not sure if this is possible

3. PageLRU, Used, !AcctLRU: page del from memcg lru before uncharge,
or charged before add to memcg lru

4. PageLRU, !Used, AcctLRU: page uncharged before del from memcg lru
(ext: swapcache)


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

This fits the case 2 above.

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

This fits the case 1 above.


Thanks

--Ying

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