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

[Barry Song]

On Thu, May 23, 2024 at 1:38 PM Baolin Wang
<baolin.wang@xxxxxxxxxxxxxxxxx> wrote:
>
>
>
> On 2024/5/23 09:14, Huang, Ying wrote:
> > Barry Song <21cnbao@xxxxxxxxx> writes:
> >
> >> 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.
> >
> > IIUC, "fallback" means you try to do something, but fail, so try
> > something else as fallback.  If so, then we don't need to count
> > splitting shmem large folio as fallback.
>
> Agree. In additon, IIUC we have never counted splitting shmem large
> folio as THP_SWPOUT_FALLBACK before or after this patch.

Hi Baolin,

My point is that THP_SWPOUT* has been dedicated to anonymous memory for years
because we have not had the capability to perform THP_SWPOUT for shared memory
before. This is the historical context of thp_swpout* in /proc/vmstat,
even though it is
not ideal. Therefore, placing shmem sysfs entries in
/sys/kernel/mm/transparent_hugepage/hugepages-2048kB/stats
allows us to monitor SWPOUT and SWPOUT FALLBACK for shmem without altering
the tradition of /proc/vmstat.

But I am not firm on this because I don't see the necessity to
differentiate shmem's
swpout from anon's swpout. They basically seem the same while anon mTHP
faults might be significantly different from file mTHP faults, in which case we
must distinguish them. So please send version 2 with the updated documentation.
I believe it should target v6.10-rc rather than v6.11 to avoid ABI
conflicts if it is
accepted.

>
> > For example, before commit 5ed890ce5147 ("mm: vmscan: avoid split during
> > shrink_folio_list()"), if folio_entire_mapcount() == 0, we will split
> > the THP.  But we will not count it as "fallback" because we haven't
> > tried to swap it out as a whole.

Thanks
Barry