Re: [PATCH v4 3/9] mm/lru: replace pgdat lru_lock with lruvec lock

[Shakeel Butt]

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