Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230713150558.200545-1-fengwei.yin@intel.com>
 <CAOUHufZBoV-ujMF3v60Sj4bARb5vRg7cpcRsqTiKHB4CKZc_fw@mail.gmail.com>
 <7e09d69e-50a7-04c5-0a40-065e2d6fd658@intel.com> <CAOUHufahGsPLm9LWpTJ75MPa6pY11rFvnoqWM7bQgVGWyWEAAw@mail.gmail.com>
 <bbfc02f4-8d3d-bd6f-aeef-5d7bb3ff46f7@intel.com>
In-Reply-To: <bbfc02f4-8d3d-bd6f-aeef-5d7bb3ff46f7@intel.com>
From:   Yu Zhao <yuzhao@google.com>
Date:   Mon, 17 Jul 2023 10:29:20 -0600
Message-ID: <CAOUHufYGVW3ojHUmPcsee5okreuWBdFVcSHLKPBTW-tpr3U52A@mail.gmail.com>
Subject: Re: [RFC PATCH] madvise: make madvise_cold_or_pageout_pte_range()
 support large folio
To:     "Yin, Fengwei" <fengwei.yin@intel.com>
Cc:     linux-mm@kvack.org, linux-kernel@vger.kernel.org,
        akpm@linux-foundation.org, willy@infradead.org, david@redhat.com,
        ryan.roberts@arm.com, shy828301@gmail.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Sun, Jul 16, 2023 at 5:52=E2=80=AFPM Yin, Fengwei <fengwei.yin@intel.com=
> wrote:
>
> On 7/14/2023 11:41 PM, Yu Zhao wrote:
> > On Thu, Jul 13, 2023 at 11:57=E2=80=AFPM Yin, Fengwei <fengwei.yin@inte=
l.com> wrote:
> >>
> >>
> >>>> -               if (pageout_anon_only_filter && !folio_test_anon(fol=
io))
> >>>> +               /* Do not interfere with other mappings of this foli=
o */
> >>>> +               if (folio_mapcount(folio) !=3D 1)
> >>>>                         continue;
> >>>>
> >>>> -               VM_BUG_ON_FOLIO(folio_test_large(folio), folio);
> >>>> -
> >>>> -               if (pte_young(ptent)) {
> >>>> -                       ptent =3D ptep_get_and_clear_full(mm, addr, =
pte,
> >>>> -                                                       tlb->fullmm)=
;
> >>>> -                       ptent =3D pte_mkold(ptent);
> >>>> -                       set_pte_at(mm, addr, pte, ptent);
> >>>> -                       tlb_remove_tlb_entry(tlb, pte, addr);
> >>>> -               }
> >>>> -
> >>>> -               /*
> >>>> -                * We are deactivating a folio for accelerating recl=
aiming.
> >>>> -                * VM couldn't reclaim the folio unless we clear PG_=
young.
> >>>> -                * As a side effect, it makes confuse idle-page trac=
king
> >>>> -                * because they will miss recent referenced history.
> >>>> -                */
> >>>> -               folio_clear_referenced(folio);
> >>>> -               folio_test_clear_young(folio);
> >>>> -               if (folio_test_active(folio))
> >>>> -                       folio_set_workingset(folio);
> >>>> +pageout_cold_folio:
> >>>>                 if (pageout) {
> >>>>                         if (folio_isolate_lru(folio)) {
> >>>>                                 if (folio_test_unevictable(folio))
> >>>> @@ -529,8 +542,30 @@ static int madvise_cold_or_pageout_pte_range(pm=
d_t *pmd,
> >>>>                 arch_leave_lazy_mmu_mode();
> >>>>                 pte_unmap_unlock(start_pte, ptl);
> >>>>         }
> >>>> -       if (pageout)
> >>>> -               reclaim_pages(&folio_list);
> >>>> +
> >>>> +       if (pageout) {
> >>>> +               LIST_HEAD(reclaim_list);
> >>>> +
> >>>> +               while (!list_empty(&folio_list)) {
> >>>> +                       int refs;
> >>>> +                       unsigned long flags;
> >>>> +                       struct mem_cgroup *memcg =3D folio_memcg(fol=
io);
> >>>> +
> >>>> +                       folio =3D lru_to_folio(&folio_list);
> >>>> +                       list_del(&folio->lru);
> >>>> +
> >>>> +                       refs =3D folio_referenced(folio, 0, memcg, &=
flags);
> >>>> +
> >>>> +                       if ((flags & VM_LOCKED) || (refs =3D=3D -1))=
 {
> >>>> +                               folio_putback_lru(folio);
> >>>> +                               continue;
> >>>> +                       }
> >>>> +
> >>>> +                       folio_test_clear_referenced(folio);
> >>>> +                       list_add(&folio->lru, &reclaim_list);
> >>>> +               }
> >>>> +               reclaim_pages(&reclaim_list);
> >>>> +       }
> >>>
> >>> i overlooked the chunk above -- it's unnecessary: after we split the
> >>> large folio (and splice the base folios onto the same LRU list), we
> >>> continue at the position of the first base folio because of:
> >>>
> >>>   pte--;
> >>>   addr -=3D PAGE_SIZE;
> >>>   continue;
> >>>
> >>> And then we do pte_mkold(), which takes care of the A-bit.
> >> This patch moves the A-bit clear out of the folio isolation loop. So
> >> even the folio is split and loop restarts from the first base folio,
> >> the A-bit is not cleared. A-bit is only cleared in reclaim loop.
> >>
> >> There is one option for A-bit clearing:
> >>   - clear A-bit of base 4K page in isolation loop and leave large foli=
o
> >>     A-bit clearing to reclaim loop.
> >>
> >> This patch didn't use it because don't want to introduce A-bit clearin=
g
> >> in two places. But I am open about clearing base 4K page A-bit cleared=
 in
> >> isolation loop. Thanks.
> >
> > Sorry but why are we trying to do multiple things in one patch that I
> > assumed is supposed to simply fix madvise() for large anon folios? And
> > none of those things seems to have a clear rationale behind it.
> >
> > The only patch that makes sense at the moment (or the first patch of a
> > series) is what I said before:
> >
> > -  if (folio_mapcount(folio) !=3D 1)
> > +  if (folio_estimated_sharers(folio) !=3D 1)
> Definitely. As I replied to you, I will split the patch to two parts:
>   - just bug fixing. Include the filio_mapcount() -> folio_estimated_shar=
es().

I'm onboard with this fix.

>     And using ptep_clear_flush_young_notify() to clear the young of PTEs.

This is another fix (if it's a real problem), hence a separate patch.

>   - refactor for large folio.

Minchan will look at the last two.

> Let me know if this is OK. Thanks.

SGTM. Thanks.