Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Message-ID: <0724b4c4-15f6-e429-f945-f57c619c7270@redhat.com>
Date:   Tue, 31 May 2022 13:59:54 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.9.0
Subject: Re: [PATCH v2 2/4] mm/migration: remove unneeded lock page and
 PageMovable check
Content-Language: en-US
To:     Miaohe Lin <linmiaohe@huawei.com>
Cc:     ying.huang@intel.com, hch@lst.de, dhowells@redhat.com,
        cl@linux.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        akpm@linux-foundation.org, mike.kravetz@oracle.com,
        naoya.horiguchi@nec.com, Minchan Kim <minchan@kernel.org>
References: <20220425132723.34824-1-linmiaohe@huawei.com>
 <20220425132723.34824-3-linmiaohe@huawei.com>
 <525298ad-5e6a-2f8d-366d-4dcb7eebd093@redhat.com>
 <f5f933dc-450c-f3ac-34e6-d6dc1d901efd@huawei.com>
 <4cf144a9-fff5-d993-4fcb-7f2dfa6e71bb@redhat.com>
 <924de987-202b-a97e-e6d2-6bdab530f190@huawei.com>
 <c566dc2c-fc70-e410-5272-767fa28cbba4@redhat.com>
 <025d0dc8-a446-b720-14a8-97c041055f48@huawei.com>
 <143ab5dd-85a9-3338-53b7-e46c9060b20e@redhat.com>
 <6ba7e2bd-28c1-53ff-a6b7-072c79714dee@huawei.com>
From:   David Hildenbrand <david@redhat.com>
Organization: Red Hat
In-Reply-To: <6ba7e2bd-28c1-53ff-a6b7-072c79714dee@huawei.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk

Sorry for the late reply, was on vacation.

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

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

-- 
Thanks,

David / dhildenb