linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] mm: put the page into the correct list when shrink_page_list fails to reclaim.
@ 2019-10-28 13:33 zhong jiang
  2019-10-28 18:47 ` Yang Shi
  0 siblings, 1 reply; 6+ messages in thread
From: zhong jiang @ 2019-10-28 13:33 UTC (permalink / raw)
  To: akpm; +Cc: ira.weiny, jhubbard, kirill.shutemov, rppt, zhongjiang, linux-mm

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.

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



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm: put the page into the correct list when shrink_page_list fails to reclaim.
  2019-10-28 13:33 [PATCH] mm: put the page into the correct list when shrink_page_list fails to reclaim zhong jiang
@ 2019-10-28 18:47 ` Yang Shi
  2019-10-29  2:52   ` zhong jiang
  0 siblings, 1 reply; 6+ messages in thread
From: Yang Shi @ 2019-10-28 18:47 UTC (permalink / raw)
  To: zhong jiang
  Cc: Andrew Morton, ira.weiny, John Hubbard, Kirill A. Shutemov, rppt,
	Linux MM

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm: put the page into the correct list when shrink_page_list fails to reclaim.
  2019-10-28 18:47 ` Yang Shi
@ 2019-10-29  2:52   ` zhong jiang
  2019-10-29  4:12     ` Yang Shi
  0 siblings, 1 reply; 6+ messages in thread
From: zhong jiang @ 2019-10-29  2:52 UTC (permalink / raw)
  To: Yang Shi
  Cc: Andrew Morton, ira.weiny, John Hubbard, Kirill A. Shutemov, rppt,
	Linux MM

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.

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




^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm: put the page into the correct list when shrink_page_list fails to reclaim.
  2019-10-29  2:52   ` zhong jiang
@ 2019-10-29  4:12     ` Yang Shi
  2019-10-29  7:16       ` zhong jiang
  0 siblings, 1 reply; 6+ messages in thread
From: Yang Shi @ 2019-10-29  4:12 UTC (permalink / raw)
  To: zhong jiang
  Cc: Andrew Morton, ira.weiny, John Hubbard, Kirill A. Shutemov, rppt,
	Linux MM

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.

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm: put the page into the correct list when shrink_page_list fails to reclaim.
  2019-10-29  4:12     ` Yang Shi
@ 2019-10-29  7:16       ` zhong jiang
  2019-10-29 17:13         ` Yang Shi
  0 siblings, 1 reply; 6+ messages in thread
From: zhong jiang @ 2019-10-29  7:16 UTC (permalink / raw)
  To: Yang Shi
  Cc: Andrew Morton, ira.weiny, John Hubbard, Kirill A. Shutemov, rppt,
	Linux MM

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




^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] mm: put the page into the correct list when shrink_page_list fails to reclaim.
  2019-10-29  7:16       ` zhong jiang
@ 2019-10-29 17:13         ` Yang Shi
  0 siblings, 0 replies; 6+ messages in thread
From: Yang Shi @ 2019-10-29 17:13 UTC (permalink / raw)
  To: zhong jiang
  Cc: Andrew Morton, ira.weiny, John Hubbard, Kirill A. Shutemov, rppt,
	Linux MM

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2019-10-29 17:13 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-28 13:33 [PATCH] mm: put the page into the correct list when shrink_page_list fails to reclaim zhong jiang
2019-10-28 18:47 ` Yang Shi
2019-10-29  2:52   ` zhong jiang
2019-10-29  4:12     ` Yang Shi
2019-10-29  7:16       ` zhong jiang
2019-10-29 17:13         ` Yang Shi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox