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

[David Hildenbrand <david@redhat.com>]

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

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?
b) Why is that code block that conditionally clears the flags of any
value and why can't we simply drop it?

-- 
Thanks,

David / dhildenb