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

[Baolin Wang]

On 2024/5/23 10:12, Barry Song wrote:
> 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.

OK, I see your point. IMO this patch will not change the behaviors of 
thp_swpout* in /proc/vmstat until adding support for large folio 
swap-out for shmem[1]. Anyway we can talk about this in that thread.

[1] 
https://lore.kernel.org/all/cover.1716285099.git.baolin.wang@xxxxxxxxxxxxxxxxx/

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

Sure. Will do. Thanks.