Received-SPF: pass (google.com: domain of linux-kernel+bounces-91980-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Precedence: bulk
MIME-Version: 1.0
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>
 <0a644230-f7a8-4091-9d00-ded6c8c3fc19@arm.com>
In-Reply-To: <0a644230-f7a8-4091-9d00-ded6c8c3fc19@arm.com>
From: Barry Song <21cnbao@gmail.com>
Date: Tue, 5 Mar 2024 22:15:05 +1300
Message-ID: <CAGsJ_4woFHT3eLzQ+Dg2dAUMve=wd=0SEZfZ4NqLyBVqeskkVg@mail.gmail.com>
Subject: Re: [RFC PATCH] mm: hold PTL from the first PTE while reclaiming a
 large folio
To: Ryan Roberts <ryan.roberts@arm.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>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Tue, Mar 5, 2024 at 10:11=E2=80=AFPM Ryan Roberts <ryan.roberts@arm.com>=
 wrote:
>
> On 05/03/2024 09:08, Barry Song wrote:
> > On Tue, Mar 5, 2024 at 9:54=E2=80=AFPM Ryan Roberts <ryan.roberts@arm.c=
om> wrote:
> >>
> >> On 04/03/2024 21:57, Barry Song wrote:
> >>> On Tue, Mar 5, 2024 at 1:21=E2=80=AFAM 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 occ=
urs 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 m=
THP,
> >>>> PTE-mapped large folios are much more common?
> >>>
> >>> right. as now large folios become a more common case, and it is my ca=
se
> >>> running in millions of phones.
> >>>
> >>> BTW, I feel we can somehow learn from hugeTLB, for example, we can re=
claim
> >>> all PTEs all together rather than iterating PTEs one by one. This wil=
l 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 lo=
oping
> >>> 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, whi=
ch makes
> >> things a bit easier too, whereas in the regular mm, they are now a var=
iable size.
> >>
> >> David and I introduced folio_pte_batch() to help gather batches of pte=
s, 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 woul=
d 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 e=
ach
> > 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=3D0;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?

no. page_vma_mapped_walk returns nr_pages times. We are doing
same check each time.  Each time, we do tlbi and set one PTE.

>
> >
> > by
> >
> > if (large folio && folio_pte_batch() =3D=3D nr_pages)
> >     set_ptes_to_swap_entries().

for this, we do check for one time, and we do much less tlbi.

> >
> >>
> >
> > Thanks,
> > Ryan

Thanks
Barry