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

[David Hildenbrand]

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

> 
>>
>>>
>>>>
>>>>
>>>> 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.

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.

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

-- 
Thanks,

David / dhildenb