Received-SPF: pass (google.com: domain of linux-kernel+bounces-81529-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <df416acb-d913-4e67-b810-cd991003242e@intel.com>
 <20240226083526.26002-1-ioworker0@gmail.com> <8b909691-ca53-43b9-aab1-dba3ef3577cd@arm.com>
 <517e4c23-11f8-4ded-a502-354c482c4e51@redhat.com>
In-Reply-To: <517e4c23-11f8-4ded-a502-354c482c4e51@redhat.com>
From: Lance Yang <ioworker0@gmail.com>
Date: Mon, 26 Feb 2024 21:47:00 +0800
Message-ID: <CAK1f24nP=g3xD=3FR11Qg0V3gsrioPcDU7a+9pjPAEORLzcd3A@mail.gmail.com>
Subject: Re: [PATCH 1/1] mm/madvise: enhance lazyfreeing with mTHP in madvise_free
To: David Hildenbrand <david@redhat.com>
Cc: Ryan Roberts <ryan.roberts@arm.com>, fengwei.yin@intel.com, akpm@linux-foundation.org, 
	linux-kernel@vger.kernel.org, linux-mm@kvack.org, mhocko@suse.com, 
	minchan@kernel.org, peterx@redhat.com, shy828301@gmail.com, 
	songmuchun@bytedance.com, wangkefeng.wang@huawei.com, zokeefe@google.com, 
	21cnbao@gmail.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Feb 26, 2024 at 9:04=E2=80=AFPM David Hildenbrand <david@redhat.com=
> wrote:
>
> On 26.02.24 13:57, Ryan Roberts wrote:
> > On 26/02/2024 08:35, Lance Yang wrote:
> >> Hey Fengwei,
> >>
> >> Thanks for taking time to review!
> >>
> >>> On Mon, Feb 26, 2024 at 10:38=E2=80=AFAM Yin Fengwei <fengwei.yin@int=
el.com> wrote:
> >>>> On Sun, Feb 25, 2024 at 8:32=E2=80=AFPM Lance Yang <ioworker0@gmail.=
com> wrote:
> >> [...]
> >>>> --- a/mm/madvise.c
> >>>> +++ b/mm/madvise.c
> >>>> @@ -676,11 +676,43 @@ static int madvise_free_pte_range(pmd_t *pmd, =
unsigned long addr,
> >>>>                 */
> >>>>                if (folio_test_large(folio)) {
> >>>>                        int err;
> >>>> +                     unsigned long next_addr, align;
> >>>>
> >>>> -                     if (folio_estimated_sharers(folio) !=3D 1)
> >>>> -                             break;
> >>>> -                     if (!folio_trylock(folio))
> >>>> -                             break;
> >>>> +                     if (folio_estimated_sharers(folio) !=3D 1 ||
> >>>> +                         !folio_trylock(folio))
> >>>> +                             goto skip_large_folio;
> >>>> +
> >>>> +                     align =3D folio_nr_pages(folio) * PAGE_SIZE;
> >>>> +                     next_addr =3D ALIGN_DOWN(addr + align, align);
> >>> There is a possible corner case:
> >>> If there is a cow folio associated with this folio and the cow folio
> >>> has smaller size than this folio for whatever reason, this change can=
't
> >>> handle it correctly.
> >>
> >> Thanks for pointing that out; it's very helpful to me!
> >> I made some changes. Could you please check if this corner case is now=
 resolved?
> >>
> >> As a diff against this patch.
> >>
> >> diff --git a/mm/madvise.c b/mm/madvise.c
> >> index bcbf56595a2e..c7aacc9f9536 100644
> >> --- a/mm/madvise.c
> >> +++ b/mm/madvise.c
> >> @@ -686,10 +686,12 @@ static int madvise_free_pte_range(pmd_t *pmd, un=
signed long addr,
> >>                      next_addr =3D ALIGN_DOWN(addr + align, align);
> >>
> >>                      /*
> >> -                     * If we mark only the subpages as lazyfree,
> >> -                     * split the large folio.
> >> +                     * If we mark only the subpages as lazyfree, or
> >> +                     * if there is a cow folio associated with this f=
olio,
> >> +                     * then split the large folio.
> >>                       */
> >> -                    if (next_addr > end || next_addr - addr !=3D alig=
n)
> >> +                    if (next_addr > end || next_addr - addr !=3D alig=
n ||
> >> +                        folio_total_mapcount(folio) !=3D folio_nr_pag=
es(folio))
> >
> > I still don't think this is correct. I think you were previously assumi=
ng that
> > if you see a page from a large folio then the whole large folio should =
be
> > contiguously mapped? This new check doesn't validate that assumption re=
liably;
> > you need to iterate through every pte to generate a batch, like David d=
oes in
> > folio_pte_batch() for this to be safe.
> >
> > An example of when this check is insufficient; let's say you have a 4 p=
age anon
> > folio mapped contiguously in a process (total_mapcount=3D4). The proces=
s is forked
> > (total_mapcount=3D8). Then each process munmaps the second 2 pages
> > (total_mapcount=3D4). In place of the munmapped 2 pages, 2 new pages ar=
e mapped.
> > Then call madvise. It's probably even easier to trigger for file-backed=
 memory
> > (I think this code path is used for both file and anon?)
>
> What would work here is using folio_pte_batch() to get how many PTEs are
> mapped *here*, then comparing the the batch size to folio_nr_pages(). If
> both match, we are mapping all subpages.

Thanks! I'll use folio_pte_batch() here in v2.

Best,
Lance

>
> --
> Cheers,
>
> David / dhildenb
>