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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date:	Sun, 14 Aug 2011 20:01:36 -0700
From:	Ying Han <yinghan@...gle.com>
To:	Johannes Weiner <hannes@...xchg.org>
Cc:	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>,
	Daisuke Nishimura <nishimura@....nes.nec.co.jp>,
	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 Fri, Aug 12, 2011 at 12:17 PM, Johannes Weiner <hannes@...xchg.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@...xchg.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 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/