On 2019/10/29 12:12, Yang Shi wrote:
> On Mon, Oct 28, 2019 at 7:52 PM zhong jiang <zhongjiang@xxxxxxxxxx> wrote:
>> On 2019/10/29 2:47, Yang Shi wrote:
>>> On Mon, Oct 28, 2019 at 6:37 AM zhong jiang <zhongjiang@xxxxxxxxxx> wrote:
>>>> Recently, I notice an race case between mlock syscall and shrink_page_list.
>>>>
>>>> one cpu run mlock syscall to make an range of the vma locked in memory. And
>>>> The specified pages will escaped from evictable list from unevictable.
>>>> Meanwhile, another cpu scan and isolate the specified pages to reclaim.
>>>> shrink_page_list hold the page lock to shrink the page and follow_page_pte
>>>> will fails to get the page lock, hence we fails to mlock the page to make
>>>> it Unevictabled.
>>>>
>>>> shrink_page_list fails to reclaim the page due to some reason. it will putback
>>>> the page to evictable lru. But the page actually belongs to an locked range of
>>>> the vma. it is unreasonable to do that. It is better to put the page to unevictable
>>>> lru.
>>> Yes, there is definitely race between mlock() and vmscan, and in the
>>> above case it might stay in evictable LRUs one more round, but it
>>> should be not harmful since try_to_unmap() would move the page to
>>> unevictable list eventually.
>> The key is how to make sure try_to_unmap alway will be called before the page is freed.
>> It is possibility page_mapped(page) is false due to some condition.
> Is it a problem? The gup just needs to refault the page in.
Hi, Yang
if a page of the vma is not mapped , mlock will make sure it will refault in the memory.
But I mean that we focus on the page has been in evictable lru, Meanwhile mlock fails to
move the page from evictable lru to unevictable lru.
cpu 0 cpu 1
isolate_lru_pages
(start .. end) pages exists in evictable lru.
mlock shrink_page_list
lock_page(page)
follow_page_pte
---> fails to hold pageloced --》goto out;
try_to_unmap
return page.
move_pages_to_lru --> putback to evictable lru
put_page && munmap --> page is unmapped and evictable lru.
The page of vma became an clean page, hence we can free the page easily. It is no need to try_to_unmap.
I miss something ? :-)
Thanks,
zhong jiang
>> Thanks,
>> zhong jiang
>>>> The patch set PageMlocked when mlock fails to get the page locked. shrink_page_list
>>>> fails to reclaim the page will putback to the correct list. if it success to reclaim
>>>> the page, we should ClearPageMlocked in time to prevent the warning from free_pages_prepare.
>>>>
>>>> Signed-off-by: zhong jiang <zhongjiang@xxxxxxxxxx>
>>>> ---
>>>> mm/gup.c | 28 ++++++++++++++++++----------
>>>> mm/vmscan.c | 9 ++++++++-
>>>> 2 files changed, 26 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/mm/gup.c b/mm/gup.c
>>>> index c2b3e11..c26d28c 100644
>>>> --- a/mm/gup.c
>>>> +++ b/mm/gup.c
>>>> @@ -283,16 +283,24 @@ static struct page *follow_page_pte(struct vm_area_struct *vma,
>>>> * handle it now - vmscan will handle it later if and
>>>> * when it attempts to reclaim the page.
>>>> */
>>>> - if (page->mapping && trylock_page(page)) {
>>>> - lru_add_drain(); /* push cached pages to LRU */
>>>> - /*
>>>> - * Because we lock page here, and migration is
>>>> - * blocked by the pte's page reference, and we
>>>> - * know the page is still mapped, we don't even
>>>> - * need to check for file-cache page truncation.
>>>> - */
>>>> - mlock_vma_page(page);
>>>> - unlock_page(page);
>>>> + if (page->mapping) {
>>>> + if (trylock_page(page)) {
>>>> + lru_add_drain(); /* push cached pages to LRU */
>>>> + /*
>>>> + * Because we lock page here, and migration is
>>>> + * blocked by the pte's page reference, and we
>>>> + * know the page is still mapped, we don't even
>>>> + * need to check for file-cache page truncation.
>>>> + */
>>>> + mlock_vma_page(page);
>>>> + unlock_page(page);
>>>> + } else {
>>>> + /*
>>>> + * Avoid putback the page to evictable list when
>>>> + * the page is in the locked vma.
>>>> + */
>>>> + SetPageMlocked(page);
>>>> + }
>>>> }
>>>> }
>>>> out:
>>>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>>>> index 1154b3a..f7d1301 100644
>>>> --- a/mm/vmscan.c
>>>> +++ b/mm/vmscan.c
>>>> @@ -1488,8 +1488,15 @@ static unsigned long shrink_page_list(struct list_head *page_list,
>>>> */
>>>> if (unlikely(PageTransHuge(page)))
>>>> (*get_compound_page_dtor(page))(page);
>>>> - else
>>>> + else {
>>>> + /*
>>>> + * There is an race between mlock and shrink_page_list
>>>> + * when mlock fails to get the PageLocked().
>>>> + */
>>>> + if (unlikely(PageMlocked(page)))
>>>> + ClearPageMlocked(page);
>>>> list_add(&page->lru, &free_pages);
>>>> + }
>>>> continue;
>>>>
>>>> activate_locked_split:
>>>> --
>>>> 1.7.12.4
>>>>
>>>>
>>> .
>>>
>>
> .
>
