Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx> writes: > On 2024/5/22 18:40, Barry Song wrote: >> On Wed, May 22, 2024 at 9:38 PM Baolin Wang >> <baolin.wang@xxxxxxxxxxxxxxxxx> wrote: >>> >>> >>> >>> On 2024/5/22 16:58, David Hildenbrand wrote: >>>> On 22.05.24 10:51, Baolin Wang wrote: >>>>> The mTHP swap related counters: 'anon_swpout' and >>>>> 'anon_swpout_fallback' are >>>>> confusing with an 'anon_' prefix, since the shmem can swap out >>>>> non-anonymous >>>>> pages. So drop the 'anon_' prefix to keep consistent with the old swap >>>>> counter >>>>> names. >>>>> >>>>> Suggested-by: "Huang, Ying" <ying.huang@xxxxxxxxx> >>>>> Signed-off-by: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx> >>>>> --- >>>> >>>> Am I daydreaming or did we add the anon_ for a reason and discussed the >>>> interaction with shmem? At least I remember some discussion around that. >>> >>> Do you mean the shmem mTHP allocation counters in previous >>> discussion[1]? But for 'anon_swpout' and 'anon_swpout_fallback', I can >>> not find previous discussions that provided a reason for adding the >>> ‘anon_’ prefix. Barry, any comments? Thanks. >> HI Baolin, >> We had tons of emails discussing about namin and I found this email, >> https://lore.kernel.org/all/bca6d142-15fd-4af5-9f71-821f891e8305@xxxxxxxxxx/ >> David had this comment, >> "I'm wondering if these should be ANON specific for now. We might want to >> add others (shmem, file) in the future." >> This is likely how the 'anon_' prefix started being added, although >> it >> wasn't specifically >> targeting swapout. > > That's what I missed before. Thanks Barry. > >> I sense your patch slightly alters the behavior of thp_swpout_fallback >> in /proc/vmstat. >> Previously, we didn't classify them as THP_SWPOUT_FALLBACK, even though we >> always split them. > > Sorry I did not get you here. I just re-name the mTHP swpout_fallback, > how can this patch change the THP_SWPOUT_FALLBACK statistic counted by > count_vm_event()? > >> if (folio_test_anon(folio) && folio_test_swapbacked(folio)) { >> ... >> if (!add_to_swap(folio)) { >> int __maybe_unused order = >> folio_order(folio); >> if >> (!folio_test_large(folio)) >> goto activate_locked_split; >> /* Fallback to swap normal pages */ >> if (split_folio_to_list(folio, >> folio_list)) >> goto activate_locked; >> #ifdef CONFIG_TRANSPARENT_HUGEPAGE >> if (nr_pages >= HPAGE_PMD_NR) { >> count_memcg_folio_events(folio, >> THP_SWPOUT_FALLBACK, 1); >> count_vm_event(THP_SWPOUT_FALLBACK); >> } >> count_mthp_stat(order, >> MTHP_STAT_ANON_SWPOUT_FALLBACK); >> #endif >> if (!add_to_swap(folio)) >> goto activate_locked_split; >> } >> } >> } else if (folio_test_swapbacked(folio) && >> folio_test_large(folio)) { >> /* Split shmem folio */ >> if (split_folio_to_list(folio, folio_list)) >> goto keep_locked; >> } >> If the goal is to incorporate pmd-mapped shmem under thp_swpout* in >> /proc/vmstat, >> and if there is consistency between /proc/vmstat and sys regarding >> their definitions, >> then I have no objection to this patch. > > I think this is the goal, moreover shmem will support large folio (not > only THP) in future, so swpout related counters should be defined as > clear as possible. > > However, shmem_swpout and shmem_swpout_* >> appear more intuitive, given that thp_swpout_* in /proc/vmstat has >> never shown any >> increments for shmem until now - we have been always splitting shmem in vmscan. > > This is somewhat similar to our previous discussion on the naming of > the shmem's mTHP counter[1], as David suggested, we should keep > counter name consistency for now and add more in the future as needed. > > [1] > https://lore.kernel.org/all/ce6be451-7c5a-402f-8340-be40699829c2@xxxxxxxxxx/ Yes. I don't find that it's necessary to distinguish anonymous and shmem mTHP swap-out now. If we need it in the future, we can add that at that time. >> By the way, if this patch is accepted, it must be included in >> version >> 6.10 to maintain >> ABI compatibility. Additionally, documentation must be updated accordingly. > > Sure. I missed update the documentation, and will do in next version. -- Best Regards, Huang, Ying
