Received-SPF: pass (google.com: domain of linux-kernel+bounces-91963-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
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>
In-Reply-To: <e5e14ef9-1bd2-45a8-818d-e92910e97f8f@arm.com>
From: Barry Song <21cnbao@gmail.com>
Date: Tue, 5 Mar 2024 22:08:34 +1300
Message-ID: <CAGsJ_4xtVhka9J3c6zDySBEJ34+mn9OLiuP0JJ+Ag+p9n9K5EA@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 9:54=E2=80=AFPM Ryan Roberts <ryan.roberts@arm.com> =
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.c=
om> 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 occur=
s for
> >> PTE-mapped, PMD-sized folios as well as smaller-than-PMD-size large fo=
lios? Now
> >> that I've had a look at the code and have a better understanding, I th=
ink that
> >> must be the case? And therefore this problem exists independently of m=
y work to
> >> support swap-out of mTHP? (From your previous report I was under the i=
mpression
> >> that it only affected mTHP).
> >
> > I think this affects all large folios with PTEs entries more than 1. bu=
t 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 mTH=
P,
> >> 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 recl=
aim
> > 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 loop=
ing
> > nr_pages times.
>
> You still need a pte-pte loop somewhere. In hugetlb's case it's in the ar=
ch
> 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 variab=
le 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 a=
dding
> 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 d=
o other
> than loop over the PTEs setting an incremented swap entry to each one? Ho=
w 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=3D0;i<nr_pages;i++)     /* page_vma_mapped_walk */
{
        lots of checks;
        clear PTEn
        set PTEn to swap
}

by

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

>

Thanks,
Ryan