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)?
>
> 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?
>
I took a more close look of the code today. And I think the current code works:
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.
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);
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.
Am I miss something again ? ;)
Thanks a lot!
