On 2021/7/13 15:25, Yu Zhao wrote:
> On Mon, Jul 12, 2021 at 1:12 AM Miaohe Lin <linmiaohe@xxxxxxxxxx> wrote:
>>
>> On 2021/7/11 7:22, Yu Zhao wrote:
>>> On Sat, Jul 10, 2021 at 4:03 AM Miaohe Lin <linmiaohe@xxxxxxxxxx> wrote:
>>>>
>>>> If the MADV_FREE pages are redirtied before they could be reclaimed, put
>>>> the pages back to anonymous LRU list by setting SwapBacked flag and the
>>>> pages will be reclaimed in normal swapout way. Otherwise MADV_FREE pages
>>>> won't be reclaimed as expected.
>>>>
>>>> Fixes: 802a3a92ad7a ("mm: reclaim MADV_FREE pages")
>>>
>>> This is not a bug -- the dirty check isn't needed but it was copied
>>> from __remove_mapping().
>>
>> Yes, this is not a bug and harmless. When we reach here, page should not be
>> dirtied because PageDirty is handled above and there is no way to redirty it
>> again as pagetable references are all gone and it's not in the swap cache.
>>
>>>
>>> The page has only one reference left, which is from the isolation.
>>> After the caller puts the page back on lru and drops the reference,
>>> the page will be freed anyway. It doesn't matter which lru it goes.
>>
>> But it looks buggy as it didn't perform the expected ops from code view.
>> Should I drop the Fixes tag and send a v2 version?
>
> I don't understand the logic here -- it looks pretty obvious to me
> that, if we want to change anything, we should delete the dirty check,
> not add another line that would enforce the belief that the dirty
> check is needed.
>
The dirty check could be removed even with the page_ref_freeze check because no one can grab
the page refcnt after the page is successfully unmapped.
Does the change below makes sense for you?
Many Thanks.
diff --git a/mm/vmscan.c b/mm/vmscan.c
index 6e26b3c93242..c31925320b33 100644
--- a/mm/vmscan.c
+++ b/mm/vmscan.c
@@ -1624,15 +1624,11 @@ static unsigned int shrink_page_list(struct list_head *page_list,
}
if (PageAnon(page) && !PageSwapBacked(page)) {
- /* follow __remove_mapping for reference */
- if (!page_ref_freeze(page, 1))
- goto keep_locked;
- if (PageDirty(page)) {
- SetPageSwapBacked(page);
- page_ref_unfreeze(page, 1);
- goto keep_locked;
- }
-
+ /*
+ * No one can grab the page refcnt or redirty the page
+ * after the page is successfully unmapped.
+ */
+ WARN_ON_ONCE(!page_ref_freeze(page, 1));
count_vm_event(PGLAZYFREED);
count_memcg_page_event(page, PGLAZYFREED);
} else if (!mapping || !__remove_mapping(mapping, page, true,
>>
>> Many thanks for reply!
>>
>>>
>>>> Signed-off-by: Miaohe Lin <linmiaohe@xxxxxxxxxx>
>>>> ---
>>>> mm/vmscan.c | 1 +
>>>> 1 file changed, 1 insertion(+)
>>>>
>>>> diff --git a/mm/vmscan.c b/mm/vmscan.c
>>>> index a7602f71ec04..6483fe0e2065 100644
>>>> --- a/mm/vmscan.c
>>>> +++ b/mm/vmscan.c
>>>> @@ -1628,6 +1628,7 @@ static unsigned int shrink_page_list(struct list_head *page_list,
>>>> if (!page_ref_freeze(page, 1))
>>>> goto keep_locked;
>>>> if (PageDirty(page)) {
>>>> + SetPageSwapBacked(page);
>>>> page_ref_unfreeze(page, 1);
>>>> goto keep_locked;
>>>> }
>>>> --
>>>> 2.23.0
>>>>
>>>>
>>> .
>>>
>>
> .
>
