Re: [PATCH v2 2/4] mm/migration: remove unneeded lock page and PageMovable check

[Miaohe Lin <linmiaohe@huawei.com>]

On 2022/5/13 0:50, David Hildenbrand wrote:
> On 12.05.22 15:26, Miaohe Lin wrote:
>> On 2022/5/12 15:10, David Hildenbrand wrote:
>>>>> If PG_isolated is still set, it will get cleared in the buddy when
>>>>> freeing the page via
>>>>>
>>>>> 	page->flags &= ~PAGE_FLAGS_CHECK_AT_PREP;
>>>>
>>>> Yes, check_free_page only complains about flags belonging to PAGE_FLAGS_CHECK_AT_FREE and PG_isolated
>>>> will be cleared in the buddy when freeing the page. But it might not be a good idea to reply on this ?
>>>> IMHO, it should be better to clear the PG_isolated explicitly ourselves.
>>>
>>> I think we can pretty much rely on this handling in the buddy :)
>>
>> So is the below code change what you're suggesting?
>>
>> 	if (page_count(page) == 1) {
>> 		/* page was freed from under us. So we are done. */
>> 		ClearPageActive(page);
>> 		ClearPageUnevictable(page);
>> -		if (unlikely(__PageMovable(page)))
>> -			ClearPageIsolated(page);
>> 		goto out;
>> 	}
> 
> Yeah, unless I am missing something important :)
> 
>>>
>>>>
>>>>>
>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Also, I am not sure how reliable that page count check is here: if we'd
>>>>>>> have another speculative reference to the page, we might see
>>>>>>> "page_count(page) > 1" and not take that path, although the previous
>>>>>>> owner released the last reference.
>>>>>>
>>>>>> IIUC, there should not be such speculative reference. The driver should have taken care
>>>>>> of it.
>>>>>
>>>>> How can you prevent any kind of speculative references?
>>>>>
>>>>> See isolate_movable_page() as an example, which grabs a speculative
>>>>> reference to then find out that the page is already isolated by someone
>>>>> else, to then back off.
>>>>
>>>> You're right. isolate_movable_page will be an speculative references case. But the page count check here
>>>> is just an optimization. If we encounter speculative references, it still works with useless effort of
>>>> migrating to be released page.
>>>
>>>
>>> Not really. The issue is that PAGE_FLAGS_CHECK_AT_FREE contains
>>> PG_active and PG_unevictable.
>>>
>>> We only clear those 2 flags if "page_count(page) == 1". Consequently,
>>> with a speculative reference, we'll run into the check_free_page_bad()
>>> when dropping the last reference.
>>
>> It seems if a speculative reference happens after the "page_count(page) == 1" check,
>> it's ok because we cleared the PG_active and PG_unevictable. And if it happens before
>> the check, this code block is skipped and the page will be freed after migration. The
>> PG_active and PG_unevictable will be correctly cleared when page is actually freed via
>> __folio_clear_active. (Please see below comment)
>>
>>>
>>> This is just shaky. Special casing on "page_count(page) == 1" for
>>> detecting "was this freed by the owner" is not 100% water proof.
>>>
>>> In an ideal world, we'd just get rid of that whole block of code and let
>>> the actual freeing code clear PG_active and PG_unevictable. But that
>>> would require changes to free_pages_prepare().
>>>
>>>
>>> Now I do wonder, if we ever even have PG_active or PG_unevictable still
>>> set when the page was freed by the owner in this code. IOW, maybe that
>>> is dead code as well and we can just remove the whole shaky
>>> "page_count(page) == 1" code block.
>>
>> Think about below common scene: Anonymous page is actively used by the sole owner process, so it
>> will have PG_active set. Then process exited while vm tries to migrate that page. So the page
>> should have refcnt == 1 while PG_active is set? Note normally PG_active should be cleared when
>> the page is released:
>>
>> __put_single_page
>>   PageLRU
>>     __clear_page_lru_flags
>>       __folio_clear_active
>>       __folio_clear_unevictable
>>
>> 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?

> 
> We did not run that code block that would clear PG_active and
> PG_unevictable.
> 
> Which still leaves the questions:
> 
> a) If PG_active and PG_unevictable was cleared, where?

For LRU pages, PG_active and PG_unevictable are cleared via __page_cache_release. And for isolated
(LRU) pages, PG_active and PG_unevictable should be cleared ourselves?

> b) Why is that code block that conditionally clears the flags of any
> value and why can't we simply drop it?
> 

To fix the issue, should we clear PG_active and PG_unevictable unconditionally here?

Thanks a lot!