On Mon, Nov 25, 2019 at 1:26 AM Alex Shi <alex.shi@xxxxxxxxxxxxxxxxx> wrote: > > > > > > 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. Actually no, unless PageLRU check and taking lruvec lock are atomic, the race mentioned by Johannes still exist. Shakeel
