Received-SPF: pass (google.com: domain of linux-kernel+bounces-91968-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <0a644230-f7a8-4091-9d00-ded6c8c3fc19@arm.com>
Date: Tue, 5 Mar 2024 09:11:10 +0000
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC PATCH] mm: hold PTL from the first PTE while reclaiming a
 large folio
Content-Language: en-GB
To: Barry Song <21cnbao@gmail.com>
Cc: akpm@linux-foundation.org, linux-mm@kvack.org, david@redhat.com,
 chrisl@kernel.org, yuzhao@google.com, hanchuanhua@oppo.com,
 linux-kernel@vger.kernel.org, willy@infradead.org, ying.huang@intel.com,
 xiang@kernel.org, mhocko@suse.com, shy828301@gmail.com,
 wangkefeng.wang@huawei.com, Barry Song <v-songbaohua@oppo.com>,
 Hugh Dickins <hughd@google.com>
References: <20240304103757.235352-1-21cnbao@gmail.com>
 <706b7129-85f6-4470-9fd9-f955a8e6bd7c@arm.com>
 <CAGsJ_4wx7oSzt4vn6B+LRoZetMhH-fDXRFrCFRyoqVOakLidjg@mail.gmail.com>
 <e5e14ef9-1bd2-45a8-818d-e92910e97f8f@arm.com>
 <CAGsJ_4xtVhka9J3c6zDySBEJ34+mn9OLiuP0JJ+Ag+p9n9K5EA@mail.gmail.com>
From: Ryan Roberts <ryan.roberts@arm.com>
In-Reply-To: <CAGsJ_4xtVhka9J3c6zDySBEJ34+mn9OLiuP0JJ+Ag+p9n9K5EA@mail.gmail.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

On 05/03/2024 09:08, Barry Song wrote:
> On Tue, Mar 5, 2024 at 9:54 PM Ryan Roberts <ryan.roberts@arm.com> wrote:
>>
>> On 04/03/2024 21:57, Barry Song wrote:
>>> On Tue, Mar 5, 2024 at 1:21 AM Ryan Roberts <ryan.roberts@arm.com> wrote:
>>>>
>>>> Hi Barry,
>>>>
>>>> On 04/03/2024 10:37, Barry Song wrote:
>>>>> From: Barry Song <v-songbaohua@oppo.com>
>>>>>
>>>>> page_vma_mapped_walk() within try_to_unmap_one() races with other
>>>>> PTEs modification such as break-before-make, while iterating PTEs
>>>>> of a large folio, it will only begin to acquire PTL after it gets
>>>>> a valid(present) PTE. break-before-make intermediately sets PTEs
>>>>> to pte_none. Thus, a large folio's PTEs might be partially skipped
>>>>> in try_to_unmap_one().
>>>>
>>>> I just want to check my understanding here - I think the problem occurs for
>>>> PTE-mapped, PMD-sized folios as well as smaller-than-PMD-size large folios? Now
>>>> that I've had a look at the code and have a better understanding, I think that
>>>> must be the case? And therefore this problem exists independently of my work to
>>>> support swap-out of mTHP? (From your previous report I was under the impression
>>>> that it only affected mTHP).
>>>
>>> I think this affects all large folios with PTEs entries more than 1. but hugeTLB
>>> is handled as a whole in try_to_unmap_one and its rmap is removed all
>>> together, i feel hugeTLB doesn't have this problem.
>>>
>>>>
>>>> Its just that the problem is becoming more pronounced because with mTHP,
>>>> PTE-mapped large folios are much more common?
>>>
>>> right. as now large folios become a more common case, and it is my case
>>> running in millions of phones.
>>>
>>> BTW, I feel we can somehow learn from hugeTLB, for example, we can reclaim
>>> all PTEs all together rather than iterating PTEs one by one. This will improve
>>> performance. for example, a batched
>>> set_ptes_to_swap_entries()
>>> {
>>> }
>>> then we only need to loop once for a large folio, right now we are looping
>>> nr_pages times.
>>
>> You still need a pte-pte loop somewhere. In hugetlb's case it's in the arch
>> implementation. HugeTLB ptes are all a fixed size for a given VMA, which makes
>> things a bit easier too, whereas in the regular mm, they are now a variable size.
>>
>> David and I introduced folio_pte_batch() to help gather batches of ptes, and it
>> uses the contpte bit to avoid iterating over intermediate ptes. And I'm adding
>> swap_pte_batch() which does a similar thing for swap entry batching in v4 of my
>> swap-out series.
>>
>> For your set_ptes_to_swap_entries() example, I'm not sure what it would do other
>> than loop over the PTEs setting an incremented swap entry to each one? How is
>> that more performant?
> 
> right now, while (page_vma_mapped_walk(&pvmw)) will loop nr_pages for each
> PTE, if each PTE, we do lots of checks within the loop.
> 
> by implementing set_ptes_to_swap_entries(), we can iterate once for
> page_vma_mapped_walk(), after folio_pte_batch() has confirmed
> the large folio is completely mapped, we set nr_pages swap entries
> all together.
> 
> we are replacing
> 
> for(i=0;i<nr_pages;i++)     /* page_vma_mapped_walk */
> {
>         lots of checks;
>         clear PTEn
>         set PTEn to swap
> }

OK so you are effectively hoisting "lots of checks" out of the loop?

> 
> by
> 
> if (large folio && folio_pte_batch() == nr_pages)
>     set_ptes_to_swap_entries().
> 
>>
> 
> Thanks,
> Ryan