Re: [RFC PATCH v2 3/3] mm: mlock: update mlock_pte_range to handle large folio

[Yosry Ahmed]

On Tue, Jul 18, 2023 at 7:10 PM Yin Fengwei <fengwei.yin@xxxxxxxxx> wrote:
>
>
>
> On 7/19/23 10:00, Yosry Ahmed wrote:
> > On Tue, Jul 18, 2023 at 6:57 PM Yin Fengwei <fengwei.yin@xxxxxxxxx> wrote:
> >>
> >>
> >> On 7/19/23 09:52, Yosry Ahmed wrote:
> >>> On Tue, Jul 18, 2023 at 6:32 PM Yosry Ahmed <yosryahmed@xxxxxxxxxx> wrote:
> >>>> On Tue, Jul 18, 2023 at 4:47 PM Yin Fengwei <fengwei.yin@xxxxxxxxx> wrote:
> >>>>>
> >>>>>
> >>>>> On 7/19/23 06:48, Yosry Ahmed wrote:
> >>>>>> On Sun, Jul 16, 2023 at 6:58 PM Yin Fengwei <fengwei.yin@xxxxxxxxx> wrote:
> >>>>>>>
> >>>>>>>
> >>>>>>> On 7/17/23 08:35, Yu Zhao wrote:
> >>>>>>>> On Sun, Jul 16, 2023 at 6:00 PM Yin, Fengwei <fengwei.yin@xxxxxxxxx> wrote:
> >>>>>>>>> On 7/15/2023 2:06 PM, Yu Zhao wrote:
> >>>>>>>>>> There is a problem here that I didn't have the time to elaborate: we
> >>>>>>>>>> can't mlock() a folio that is within the range but not fully mapped
> >>>>>>>>>> because this folio can be on the deferred split queue. When the split
> >>>>>>>>>> happens, those unmapped folios (not mapped by this vma but are mapped
> >>>>>>>>>> into other vmas) will be stranded on the unevictable lru.
> >>>>>>>>> This should be fine unless I missed something. During large folio split,
> >>>>>>>>> the unmap_folio() will be migrate(anon)/unmap(file) folio. Folio will be
> >>>>>>>>> munlocked in unmap_folio(). So the head/tail pages will be evictable always.
> >>>>>>>> It's close but not entirely accurate: munlock can fail on isolated folios.
> >>>>>>> Yes. The munlock just clear PG_mlocked bit but with PG_unevictable left.
> >>>>>>>
> >>>>>>> Could this also happen against normal 4K page? I mean when user try to munlock
> >>>>>>> a normal 4K page and this 4K page is isolated. So it become unevictable page?
> >>>>>> Looks like it can be possible. If cpu 1 is in __munlock_folio() and
> >>>>>> cpu 2 is isolating the folio for any purpose:
> >>>>>>
> >>>>>> cpu1                                        cpu2
> >>>>>>                                                 isolate folio
> >>>>>> folio_test_clear_lru() // 0
> >>>>>>                                                 putback folio // add
> >>>>>> to unevictable list
> >>>>>> folio_test_clear_mlocked()
> >>>>> Yes. Yu showed this sequence to me in another email. I thought the putback_lru()
> >>>>> could correct the none-mlocked but unevictable folio. But it doesn't because
> >>>>> of this race.
> >>>> (+Hugh Dickins for vis)
> >>>>
> >>>> Yu, I am not familiar with the split_folio() case, so I am not sure it
> >>>> is the same exact race I stated above.
> >>>>
> >>>> Can you confirm whether or not doing folio_test_clear_mlocked() before
> >>>> folio_test_clear_lru() would fix the race you are referring to? IIUC,
> >>>> in this case, we make sure we clear PG_mlocked before we try to to
> >>>> clear PG_lru. If we fail to clear it, then someone else have the folio
> >>>> isolated after we clear PG_mlocked, so we can be sure that when they
> >>>> put the folio back it will be correctly made evictable.
> >>>>
> >>>> Is my understanding correct?
> >>> Hmm, actually this might not be enough. In folio_add_lru() we will
> >>> call folio_batch_add_and_move(), which calls lru_add_fn() and *then*
> >>> sets PG_lru. Since we check folio_evictable() in lru_add_fn(), the
> >>> race can still happen:
> >>>
> >>>
> >>> cpu1                              cpu2
> >>>                                       folio_evictable() //false
> >>> folio_test_clear_mlocked()
> >>> folio_test_clear_lru() //false
> >>>                                       folio_set_lru()
> >>>
> >>> Relying on PG_lru for synchronization might not be enough with the
> >>> current code. We might need to revert 2262ace60713 ("mm/munlock:
> >>> delete smp_mb() from __pagevec_lru_add_fn()").
> >>>
> >>> Sorry for going back and forth here, I am thinking out loud.
> >>
> >> Yes. Currently, the order in lru_add_fn() is not correct.
> >>
> >> I think we should move folio_test_clear_lru(folio) into
> >>
> >> lru locked range. As the lru lock here was expected to
> >>
> >> use for sync here. Check the comment in lru_add_fn().
> >
> > Right, I am wondering if it would be better to just revert
> > 2262ace60713 and rely on the memory barrier and operations ordering
> > instead of the lru lock. The lru lock is heavily contended as-is, so
> > avoiding using it where possible is preferable I assume.
> My understanding is set_lru after add folio to lru list is correct.
> Once folio_set_lru(), others can do isolation of this folio. But if
> this folio is not in lru list yet, what could happen? It's not required
> to hold lru lock to do isolation.

IIUC, clearing PG_lru is an atomic lockless operation to make sure no
one else is isolating the folio, but then you need to hold the lruvec
lock and actually delete the folio from the lru to complete its
isolation. This is my read of folio_isolate_lru().

Anyway, whether we rely on the lruvec lock or memory barrier +
operation ordering doesn't make a huge difference (I think?). The code
seemed to work with the latter before mlock_count was introduced.

If we decide to go with the latter, I can integrate the fix into the
refresh of my mlock_count rework RFC series (as it would be dependent
on that series). If we decide to go with the lruvec, then it can be
done as part of this series, or separately.

Thanks.

>
> >
> >>
> >>
> >> Regards
> >>
> >> Yin, Fengwei
> >>
> >>
> >>>
> >>>> If yes, I can add this fix to my next version of the RFC series to
> >>>> rework mlock_count. It would be a lot more complicated with the
> >>>> current implementation (as I stated in a previous email).
> >>>>
> >>>>>>
> >>>>>> The page would be stranded on the unevictable list in this case, no?
> >>>>>> Maybe we should only try to isolate the page (clear PG_lru) after we
> >>>>>> possibly clear PG_mlocked? In this case if we fail to isolate we know
> >>>>>> for sure that whoever has the page isolated will observe that
> >>>>>> PG_mlocked is clear and correctly make the page evictable.
> >>>>>>
> >>>>>> This probably would be complicated with the current implementation, as
> >>>>>> we first need to decrement mlock_count to determine if we want to
> >>>>>> clear PG_mlocked, and to do so we need to isolate the page as
> >>>>>> mlock_count overlays page->lru. With the proposal in [1] to rework
> >>>>>> mlock_count, it might be much simpler as far as I can tell. I intend
> >>>>>> to refresh this proposal soon-ish.
> >>>>>>
> >>>>>> [1]https://lore.kernel.org/lkml/20230618065719.1363271-1-yosryahmed@xxxxxxxxxx/
> >>>>>>
> >>>>>>>
> >>>>>>> Regards
> >>>>>>> Yin, Fengwei
> >>>>>>>