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

[Yang Shi <shy828301@gmail.com>]

On Tue, Oct 29, 2019 at 12:16 AM zhong jiang <zhongjiang@huawei.com> wrote:
>
> 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.

If gup still can return legitimate page, I'm supposed it means gup
happens before try_to_unmap(). If so try_to_unmap() would see the VMA
is VM_LOCKED, then it should just set Mlocked flag for the page, then
move_pages_to_lru() would put the page to unevictable lru instead of
evictable lru.

>                                                      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
> >>>>
> >>>>
> >>> .
> >>>
> >>
> > .
> >
>
>
>