Hi Baolin, On Wed, May 22, 2024 at 7:24 PM Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx> wrote: > > > > 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()? Currently, PMD-mapped shmem folios are not accounted for in THP_SWPOUT and related counters. So, IMO, if we intend to account for them in those counters in the future, removing the 'anon_' prefix from the mTHP swap counters would be reasonable :) Thanks, Lance > > > 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/ > > > > 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. >
