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

[Miaohe Lin <linmiaohe@huawei.com>]

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;
	}
> 
>>
>>>
>>>>
>>>>>
>>>>>
>>>>> 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?

Thanks!

> 
> Ccing Minchan, who added clearing of the pageflags at that point.
>