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

[Miaohe Lin]

On 2022/5/11 23:23, David Hildenbrand wrote:
> On 09.05.22 10:51, Miaohe Lin wrote:
>> On 2022/4/29 18:07, David Hildenbrand wrote:
snip
>>
>> z3fold, zsmalloc will do __ClearPageMovable when the page is going to be released.
>> So I think we shouldn't see a movable page here:
>>
>> void __ClearPageMovable(struct page *page)
>> {
>> 	VM_BUG_ON_PAGE(!PageMovable(page), page);
>> 	/*
>> 	 * Clear registered address_space val with keeping PAGE_MAPPING_MOVABLE
>> 	 * flag so that VM can catch up released page by driver after isolation.
>> 	 * With it, VM migration doesn't try to put it back.
>> 	 */
>> 	page->mapping = (void *)((unsigned long)page->mapping &
>> 				PAGE_MAPPING_MOVABLE);
>> }
>>
>> But it seems there is no guarantee for PageIsolated flag. Or am I miss something?
> 
> At least the code we have now:
> 
> if (unlikely(__PageMovable(page)))
> 	ClearPageIsolated(page);
> 
> Should be dead code. So PG_isolated could remain set.
> 
> 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.

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

Thanks!

>