Messages in this thread | | | Subject | Re: [PATCH v2 2/4] mm/migration: remove unneeded lock page and PageMovable check | From | Miaohe Lin <> | Date | Tue, 31 May 2022 20:37:51 +0800 |
| |
On 2022/5/31 19:59, David Hildenbrand wrote: > Sorry for the late reply, was on vacation.
That's all right. Hope you have a great time. ;)
> >>>> >>>> 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)? >> >> Sorry, you're right. I thought the old page will be freed via putback_lru_page which will >> set PageLRU back instead of put_page directly. So if the above race occurs, PG_active and >> PG_unevictable will remain set while page goes to the buddy and check_free_page will complain >> about it. But it seems this is never witnessed? > > Maybe > > a) we were lucky so far and didn't trigger it > b) the whole code block is dead code because we are missing something > c) we are missing something else :)
I think I found the things we missed in another email [1]. [1]: https://lore.kernel.org/all/948ea45e-3b2b-e16c-5b8c-4c34de0ea593@huawei.com/
Paste the main content of [1] here:
" 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. " Or Am I miss something again? :)
> >> >>> >>> 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? >> >> For LRU pages, PG_active and PG_unevictable are cleared via __page_cache_release. And for isolated >> (LRU) pages, PG_active and PG_unevictable should be cleared ourselves? >> >>> b) Why is that code block that conditionally clears the flags of any >>> value and why can't we simply drop it? >>> >> >> To fix the issue, should we clear PG_active and PG_unevictable unconditionally here? > > I wonder if we should simply teach actual freeing code to simply clear > both flags when freeing an isolated page? IOW, to detect "isolated LRU" > is getting freed and fixup?
IMHO, clearing both flags are done by the caller indeed. Another example I found when I read the khugepaged code recently is pasted below:
collapse_file ... page_ref_unfreeze(page, 1); ClearPageActive(page); ClearPageUnevictable(page); unlock_page(page); put_page(page); index++; ...
Thanks!
>
| |