On Sun, Jul 16, 2023 at 5:52 PM Yin, Fengwei <fengwei.yin@xxxxxxxxx> wrote:
>
> On 7/14/2023 11:41 PM, Yu Zhao wrote:
> > On Thu, Jul 13, 2023 at 11:57 PM Yin, Fengwei <fengwei.yin@xxxxxxxxx> wrote:
> >>
> >>
> >>>> - if (pageout_anon_only_filter && !folio_test_anon(folio))
> >>>> + /* Do not interfere with other mappings of this folio */
> >>>> + if (folio_mapcount(folio) != 1)
> >>>> continue;
> >>>>
> >>>> - VM_BUG_ON_FOLIO(folio_test_large(folio), folio);
> >>>> -
> >>>> - if (pte_young(ptent)) {
> >>>> - ptent = ptep_get_and_clear_full(mm, addr, pte,
> >>>> - tlb->fullmm);
> >>>> - ptent = pte_mkold(ptent);
> >>>> - set_pte_at(mm, addr, pte, ptent);
> >>>> - tlb_remove_tlb_entry(tlb, pte, addr);
> >>>> - }
> >>>> -
> >>>> - /*
> >>>> - * We are deactivating a folio for accelerating reclaiming.
> >>>> - * VM couldn't reclaim the folio unless we clear PG_young.
> >>>> - * As a side effect, it makes confuse idle-page tracking
> >>>> - * 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(pmd_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 = folio_memcg(folio);
> >>>> +
> >>>> + folio = lru_to_folio(&folio_list);
> >>>> + list_del(&folio->lru);
> >>>> +
> >>>> + refs = folio_referenced(folio, 0, memcg, &flags);
> >>>> +
> >>>> + if ((flags & VM_LOCKED) || (refs == -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 -= 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 folio
> >> A-bit clearing to reclaim loop.
> >>
> >> This patch didn't use it because don't want to introduce A-bit clearing
> >> 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) != 1)
> > + if (folio_estimated_sharers(folio) != 1)
> Definitely. As I replied to you, I will split the patch to two parts:
> - just bug fixing. Include the filio_mapcount() -> folio_estimated_shares().
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.
