Received-SPF: pass (google.com: domain of linux-kernel+bounces-50258-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: <20240201125226.28372-1-ioworker0@gmail.com> <CAHbLzkqUyYy67Fp6Zv2oeGyawHZuHqkiDGruivcRMcCGj6a-_A@mail.gmail.com>
 <CAK1f24ng_+rFd0Z5TiuZ86QUtso_09g5ksZ+p8Va+m-tK85JoQ@mail.gmail.com>
In-Reply-To: <CAK1f24ng_+rFd0Z5TiuZ86QUtso_09g5ksZ+p8Va+m-tK85JoQ@mail.gmail.com>
From: Yang Shi <shy828301@gmail.com>
Date: Fri, 2 Feb 2024 09:43:30 -0800
Message-ID: <CAHbLzkq=7Z9PQFHOWnrbcS0YWexw40OSHbfQCNqV5+9gsDnYEQ@mail.gmail.com>
Subject: Re: [PATCH 1/1] mm/khugepaged: skip copying lazyfree pages on collapse
To: Lance Yang <ioworker0@gmail.com>
Cc: akpm@linux-foundation.org, mhocko@suse.com, zokeefe@google.com, 
	david@redhat.com, songmuchun@bytedance.com, peterx@redhat.com, 
	minchan@kernel.org, linux-mm@kvack.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Feb 2, 2024 at 3:23=E2=80=AFAM Lance Yang <ioworker0@gmail.com> wro=
te:
>
> On Fri, Feb 2, 2024 at 4:37=E2=80=AFAM Yang Shi <shy828301@gmail.com> wro=
te:
> >
> > On Thu, Feb 1, 2024 at 4:53=E2=80=AFAM Lance Yang <ioworker0@gmail.com>=
 wrote:
> > >
> > > The collapsing behavior of khugepaged with pages
> > > marked using MADV_FREE might cause confusion
> > > among users.
> > >
> > > For instance, allocate a 2MB chunk using mmap and
> > > later release it by MADV_FREE. Khugepaged will not
> > > collapse this chunk. From the user's perspective,
> > > it treats lazyfree pages as pte_none. However,
> > > for some pages marked as lazyfree with MADV_FREE,
> > > khugepaged might collapse this chunk and copy
> > > these pages to a new huge page. This inconsistency
> > > in behavior could be confusing for users.
> > >
> > > After a successful MADV_FREE operation, if there is
> > > no subsequent write, the kernel can free the pages
> > > at any time. Therefore, in my opinion, counting
> > > lazyfree pages in max_pte_none seems reasonable.
> > >
> > > Perhaps treating MADV_FREE like MADV_DONTNEED, not
> > > copying lazyfree pages when khugepaged collapses
> > > huge pages in the background better aligns with
> > > user expectations.
> > >
> > > Signed-off-by: Lance Yang <ioworker0@gmail.com>
> > > ---
> > >  mm/khugepaged.c | 10 +++++++++-
> > >  1 file changed, 9 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> > > index 2b219acb528e..6cbf46d42c6a 100644
> > > --- a/mm/khugepaged.c
> > > +++ b/mm/khugepaged.c
> > > @@ -777,6 +777,7 @@ static int __collapse_huge_page_copy(pte_t *pte,
> > >                                      pmd_t orig_pmd,
> > >                                      struct vm_area_struct *vma,
> > >                                      unsigned long address,
> > > +                                    struct collapse_control *cc,
> > >                                      spinlock_t *ptl,
> > >                                      struct list_head *compound_pagel=
ist)
> > >  {
> > > @@ -797,6 +798,13 @@ static int __collapse_huge_page_copy(pte_t *pte,
> > >                         continue;
> > >                 }
> > >                 src_page =3D pte_page(pteval);
> > > +
> > > +               if (cc->is_khugepaged
> > > +                               && !folio_test_swapbacked(page_folio(=
src_page))) {
> > > +                       clear_user_highpage(page, _address);
> > > +                       continue;
> >
> > If the page was written before khugepaged collapsed it, and khugepaged
> > collapsed the page before memory reclaim kicked in, didn't this
> > somehow cause data corruption?
> >
>
> Thanks a lot! Yang, you're correct; indeed, there is
> a potential issue with data corruption.
>
> I took a look at the check for lazyfree pages in
> smaps_pte_entry.
>
> Here's the modification:
> if (cc->is_khugepaged && !PageSwapBacked(src_page)
>         && !pte_dirty(pteval) && !PageDirty(src_page)) {
>         clear_user_highpage(page, _address);
>         continue;
> }

This may be ok. But as I said in another reply, this may still incur
data corruption.

>
> Could you please take a look?
>
> Thanks,
> Lance
>
> > > +               }
> > > +
> > >                 if (copy_mc_user_highpage(page, src_page, _address, v=
ma) > 0) {
> > >                         result =3D SCAN_COPY_MC;
> > >                         break;
> > > @@ -1205,7 +1213,7 @@ static int collapse_huge_page(struct mm_struct =
*mm, unsigned long address,
> > >         anon_vma_unlock_write(vma->anon_vma);
> > >
> > >         result =3D __collapse_huge_page_copy(pte, hpage, pmd, _pmd,
> > > -                                          vma, address, pte_ptl,
> > > +                                          vma, address, cc, pte_ptl,
> > >                                            &compound_pagelist);
> > >         pte_unmap(pte);
> > >         if (unlikely(result !=3D SCAN_SUCCEED))
> > > --
> > > 2.33.1
> > >