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

[Baolin Wang <baolin.wang@linux.alibaba.com>]

On 2024/5/23 10:12, Barry Song wrote:
> On Thu, May 23, 2024 at 1:38 PM Baolin Wang
> <baolin.wang@linux.alibaba.com> wrote:
>>
>>
>>
>> On 2024/5/23 09:14, Huang, Ying wrote:
>>> Barry Song <21cnbao@gmail.com> writes:
>>>
>>>> On Wed, May 22, 2024 at 9:38 PM Baolin Wang
>>>> <baolin.wang@linux.alibaba.com> 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@intel.com>
>>>>>>> Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com>
>>>>>>> ---
>>>>>>
>>>>>> 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@redhat.com/
>>>>
>>>> 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@linux.alibaba.com/

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