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