[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e629f595-df0a-71b2-35b4-b266ba1d0431@linux.alibaba.com>
Date: Mon, 25 Nov 2019 17:26:34 +0800
From: Alex Shi <alex.shi@...ux.alibaba.com>
To: Johannes Weiner <hannes@...xchg.org>
Cc: cgroups@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-mm@...ck.org, akpm@...ux-foundation.org,
mgorman@...hsingularity.net, tj@...nel.org, hughd@...gle.com,
khlebnikov@...dex-team.ru, daniel.m.jordan@...cle.com,
yang.shi@...ux.alibaba.com, willy@...radead.org,
shakeelb@...gle.com, Michal Hocko <mhocko@...nel.org>,
Vladimir Davydov <vdavydov.dev@...il.com>,
Roman Gushchin <guro@...com>,
Chris Down <chris@...isdown.name>,
Thomas Gleixner <tglx@...utronix.de>,
Vlastimil Babka <vbabka@...e.cz>, Qian Cai <cai@....pw>,
Andrey Ryabinin <aryabinin@...tuozzo.com>,
"Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
Jérôme Glisse <jglisse@...hat.com>,
Andrea Arcangeli <aarcange@...hat.com>,
David Rientjes <rientjes@...gle.com>,
"Aneesh Kumar K.V" <aneesh.kumar@...ux.ibm.com>,
swkhack <swkhack@...il.com>,
"Potyra, Stefan" <Stefan.Potyra@...ktrobit.com>,
Mike Rapoport <rppt@...ux.vnet.ibm.com>,
Stephen Rothwell <sfr@...b.auug.org.au>,
Colin Ian King <colin.king@...onical.com>,
Jason Gunthorpe <jgg@...pe.ca>,
Mauro Carvalho Chehab <mchehab+samsung@...nel.org>,
Peng Fan <peng.fan@....com>,
Nikolay Borisov <nborisov@...e.com>,
Ira Weiny <ira.weiny@...el.com>,
Kirill Tkhai <ktkhai@...tuozzo.com>,
Yafang Shao <laoar.shao@...il.com>
Subject: Re: [PATCH v4 3/9] mm/lru: replace pgdat lru_lock with lruvec lock
>
> But that leaves me with one more worry: compaction. We locked out
> charge moving now, so between that and knowing that the page is alive,
> we have page->mem_cgroup stable. But compaction doesn't know whether
> the page is alive - it comes from a pfn and finds out using PageLRU.
>
> In the current code, pgdat->lru_lock remains the same before and after
> the page is charged to a cgroup, so once compaction has that locked
> and it observes PageLRU, it can go ahead and isolate the page.
>
> But lruvec->lru_lock changes during charging, and then compaction may
> hold the wrong lock during isolation:
>
> compaction: generic_file_buffered_read:
>
> page_cache_alloc()
>
> !PageBuddy()
>
> lock_page_lruvec(page)
> lruvec = mem_cgroup_page_lruvec()
> spin_lock(&lruvec->lru_lock)
> if lruvec != mem_cgroup_page_lruvec()
> goto again
>
> add_to_page_cache_lru()
> mem_cgroup_commit_charge()
> page->mem_cgroup = foo
> lru_cache_add()
> __pagevec_lru_add()
> SetPageLRU()
>
> if PageLRU(page):
> __isolate_lru_page()
>
> I don't see what prevents the lruvec from changing under compaction,
> neither in your patches nor in Hugh's. Maybe I'm missing something?
>
Hi Johannes,
It looks my patch do the lruvec recheck/relock after PageLRU in compaction.c.
It should be fine for your question. So I will try more testing after all changes.
Thanks
Alex
@@ -949,10 +959,26 @@ static bool too_many_isolated(pg_data_t *pgdat)
if (!(cc->gfp_mask & __GFP_FS) && page_mapping(page))
goto isolate_fail;
+ rcu_read_lock();
+reget_lruvec:
+ lruvec = mem_cgroup_page_lruvec(page, pgdat);
+
/* If we already hold the lock, we can skip some rechecking */
- if (!locked) {
- locked = compact_lock_irqsave(&pgdat->lru_lock,
- &flags, cc);
+ if (lruvec != locked_lruvec) {
+ if (locked_lruvec) {
+ spin_unlock_irqrestore(&locked_lruvec->lru_lock,
+ flags);
+ locked_lruvec = NULL;
+ }
+ if (compact_lock_irqsave(&lruvec->lru_lock,
+ &flags, cc))
+ locked_lruvec = lruvec;
+
+
+ if (lruvec != mem_cgroup_page_lruvec(page, pgdat))
+ goto reget_lruvec;
+
+ rcu_read_unlock();
/* Try get exclusive access under lock */
if (!skip_updated) {
@@ -974,9 +1000,9 @@ static bool too_many_isolated(pg_data_t *pgdat)
low_pfn += compound_nr(page) - 1;
goto isolate_fail;
}
- }
+ } else
+ rcu_read_unlock();
- lruvec = mem_cgroup_page_lruvec(page, pgdat);
/* Try isolate the page */
if (__isolate_lru_page(page, isolate_mode) != 0)
@@ -1017,9 +1043,10 @@ static bool too_many_isolated(pg_data_t *pgdat)
* page anyway.
*/
if (nr_isolated) {
- if (locked) {
- spin_unlock_irqrestore(&pgdat->lru_lock, flags);
- locked = false;
+ if (locked_lruvec) {
+ spin_unlock_irqrestore(&locked_lruvec->lru_lock,
+ flags);
+ locked_lruvec = NULL;
}
putback_movable_pages(&cc->migratepages);
Powered by blists - more mailing lists