Re: [PATCH] mm: drop the 'anon_' prefix for swap-out mTHP counters

[Barry Song]

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.

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.

                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. 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.

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.

>
> [1]
> https://lore.kernel.org/all/05d0096e4ec3e572d1d52d33a31a661321ac1551.1713755580.git.baolin.wang@xxxxxxxxxxxxxxxxx/

Thanks
Barry