Re: [PATCH] mm: put the page into the correct list when shrink_page_list fails to reclaim.

[zhong jiang <zhongjiang@huawei.com>]

On 2019/10/29 12:12, Yang Shi wrote:
> On Mon, Oct 28, 2019 at 7:52 PM zhong jiang <zhongjiang@huawei.com> wrote:
>> On 2019/10/29 2:47, Yang Shi wrote:
>>> On Mon, Oct 28, 2019 at 6:37 AM zhong jiang <zhongjiang@huawei.com> 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@huawei.com>
>>>> ---
>>>>  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
>>>>
>>>>
>>> .
>>>
>>
> .
>