Message-ID: <56f013a8-b585-4683-a998-83ea0dc53d95@redhat.com>
Date:   Wed, 8 Jun 2022 12:05:16 +0200
From:   David Hildenbrand <david@...hat.com>
To:     Miaohe Lin <linmiaohe@...wei.com>
Cc:     ying.huang@...el.com, hch@....de, dhowells@...hat.com,
        cl@...ux.com, linux-mm@...ck.org, linux-kernel@...r.kernel.org,
        akpm@...ux-foundation.org, mike.kravetz@...cle.com,
        naoya.horiguchi@....com, Minchan Kim <minchan@...nel.org>
Subject: Re: [PATCH v2 2/4] mm/migration: remove unneeded lock page and
 PageMovable check

On 07.06.22 04:20, Miaohe Lin wrote:
> On 2022/6/2 16:47, David Hildenbrand wrote:
>> On 02.06.22 09:40, Miaohe Lin wrote:
>>> On 2022/6/1 18:31, David Hildenbrand wrote:
>>>> On 31.05.22 14:37, Miaohe Lin wrote:
>>>>> On 2022/5/31 19:59, David Hildenbrand wrote:
>>>>>> Sorry for the late reply, was on vacation.
>>>>>
>>>>> That's all right. Hope you have a great time. ;)
>>>>>
>>>>>>
>>>>>>>>>
>>>>>>>>> But for isolated page, PageLRU is cleared. So when the isolated page is released, __clear_page_lru_flags
>>>>>>>>> won't be called. So we have to clear the PG_active and PG_unevictable here manully. So I think
>>>>>>>>> this code block works. Or am I miss something again?
>>>>>>>>
>>>>>>>> Let's assume the following: page as freed by the owner and we enter
>>>>>>>> unmap_and_move().
>>>>>>>>
>>>>>>>>
>>>>>>>> #1: enter unmap_and_move() // page_count is 1
>>>>>>>> #2: enter isolate_movable_page() // page_count is 1
>>>>>>>> #2: get_page_unless_zero() // page_count is now 2
>>>>>>>> #1: if (page_count(page) == 1) { // does not trigger
>>>>>>>> #2: put_page(page); // page_count is now 1
>>>>>>>> #1: put_page(page); // page_count is now 0 -> freed
>>>>>>>>
>>>>>>>>
>>>>>>>> #1 will trigger __put_page() -> __put_single_page() ->
>>>>>>>> __page_cache_release() will not clear the flags because it's not an LRU
>>>>>>>> page at that point in time, right (-> isolated)?
>>>>>>>
>>>>>>> Sorry, you're right. I thought the old page will be freed via putback_lru_page which will
>>>>>>> set PageLRU back instead of put_page directly. So if the above race occurs, PG_active and
>>>>>>> PG_unevictable will remain set while page goes to the buddy and check_free_page will complain
>>>>>>> about it. But it seems this is never witnessed?
>>>>>>
>>>>>> Maybe
>>>>>>
>>>>>> a) we were lucky so far and didn't trigger it
>>>>>> b) the whole code block is dead code because we are missing something
>>>>>> c) we are missing something else :)
>>>>>
>>>>> I think I found the things we missed in another email [1].
>>>>> [1]: https://lore.kernel.org/all/948ea45e-3b2b-e16c-5b8c-4c34de0ea593@huawei.com/
>>>>>
>>>>> Paste the main content of [1] here:
>>>>>
>>>>> "
>>>>> There are 3 cases in unmap_and_move:
>>>>>
>>>>> 1.page is freed through "if (page_count(page) == 1)" code block. This works
>>>>> as PG_active and PG_unevictable are cleared here.
>>>>>
>>>>> 2. Failed to migrate the page. The page won't be release so we don't care about it.
>>>>
>>>> Right, page is un-isolated.
>>>>
>>>>>
>>>>> 3. The page is migrated successfully. The PG_active and PG_unevictable are cleared
>>>>> via folio_migrate_flags():
>>>>>
>>>>> 	if (folio_test_clear_active(folio)) {
>>>>> 		VM_BUG_ON_FOLIO(folio_test_unevictable(folio), folio);
>>>>> 		folio_set_active(newfolio);
>>>>> 	} else if (folio_test_clear_unevictable(folio))
>>>>> 		folio_set_unevictable(newfolio);
>>>>
>>>> Right.
>>>>
>>>>>
>>>>> For the above race case, the page won't be freed through "if (page_count(page) == 1)" code block.
>>>>> It will just be migrated and freed via put_page() after folio_migrate_flags() having cleared PG_active
>>>>> and PG_unevictable.
>>>>> "
>>>>> Or Am I miss something again? :)
>>>>
>>>> For #1, I'm still not sure what would happen on a speculative reference.
>>>>
>>>> It's worth summarizing that
>>>>
>>>> a) free_pages_prepare() will clear both flags via page->flags &=
>>>> ~PAGE_FLAGS_CHECK_AT_PREP;
>>>>
>>>> b) free_pages_prepare() will bail out if any flag is set in
>>>> check_free_page().
>>>>
>>>> As we've never seen b) in the wild, this certainly has low priority, and
>>>> maybe it really cannot happen right now.
>>>>
>>>> However, maybe really allowing these flags to be set when freeing the
>>>> page and removing the "page_count(page) == 1" case from migration code
>>>> would be the clean thing to do.
>>>
>>> IMHO, check_free_page is used to catch possible problem. There's the comment of PAGE_FLAGS_CHECK_AT_FREE:
>>>
>>> /*
>>>  * Flags checked when a page is freed.  Pages being freed should not have
>>>  * these flags set.  If they are, there is a problem.
>>>  */
>>> #define PAGE_FLAGS_CHECK_AT_FREE
>>>
>>> There might be an assumption: when page is freed, it shouldn't be an active or unevictable page. It should be
>>> inactive and evictable. So allowing these flags to be set when freeing the page might not be a good idea?
>>
>> Yeah, and we'd be lifting that restriction because there is good reason
>> to do so.
>>
>> Maybe we *could* special case for isolated pages; however, that adds
>> runtime overhead. Of course, we could perform different checks for e.g.,
>> DEBUG_VM vs !DEBUG_VM.
> 
> I found there is one assumption about PG_active and PG_unevictable, i.e. in __folio_clear_lru_flags:
> 
> 	/* this shouldn't happen, so leave the flags to bad_page() */
> 	if (folio_test_active(folio) && folio_test_unevictable(folio))
> 		return;
> 
> If PG_active and PG_unevictable are both set, this case will be caught in the bad_page() via check_free_page().
> There might be some other assumptions about PG_active and PG_unevictable. So I think it's not safe to lift that
> restriction.
> 
> But maybe we could limit this check within DEBUG_VM as you suggested. Am I supposed to do it?

Well, if you want, you can look into ways of cleaning that up and
removing the "if there is more than one reference, the owner hasn't
freed the page" condition, because there are corner cases where the
owner might have freed the page but speculative references keep the
refcount temporarily incremented.

-- 
Thanks,

David / dhildenb

Powered by blists - more mailing lists