Re: [PATCH 2/12] mm: rmap use pte lock not mmap_sem to set PageMlocked

Vlastimil Babka
2015-10-19T20:52:52Z

On 10/19/2015 09:17 PM, Hugh Dickins wrote:
>> Now if CPU0 is the last mapper, it will unmap the page anyway
>> further in exit_mmap(). If not, it stays mlocked.
>>
>> The key problem is that page lock doesn't cover the TestClearPageMlocked(page)
>> part on CPU0.
>
> Thank you for expanding: your diagram beats my words. Yes, I now agree
> with you again - but reserve the right the change my mind an infinite
> number of times as we look into this for longer.

Good :)

> You can see why mm/mlock.c is not my favourite source file, and every
> improvement to it seems to make it worse.

Thank you for not explicitly pointing out the authorship of the current
pagevec-based munlock_vma_pages_range. In the unknown author's defense,
it was my first series.

> It doesn't help that most of
> the functions named "munlock" are about trying to set the mlocked bit.

That's hopefully a much older issue. And I may add that it doesn't help
that although we do atomic TestAndSet/Clear operations, it still subtly
relies on other locks for correctness.

> And while it's there on our screens, let me note that "page_mapcount > 1"
> "improvement" of mine is, I believe, less valid in the current multistage
> procedure than when I first added it (though perhaps a look back would
> prove me just as wrong back then). But it errs on the safe side (never
> marking something unevictable when it's evictable) since PageMlocked has
> already been cleared, so I think that it's still an optimization well
> worth making for the common case.

Sure.

>> Your patch should help AFAICS. If CPU1 does the mlock under pte lock, the
>> TestClear... on CPU0 can happen only after that.
>> If CPU0 takes pte lock first, then CPU1 must see the VM_LOCKED flag cleared,
>> right?
>
> Right - thanks a lot for giving it more thought.
>
> Hugh