lkml.org 
[lkml]   [2024]   [May]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH] mm: drop the 'anon_' prefix for swap-out mTHP counters
From


On 2024/5/22 18:40, Barry Song wrote:
> 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.

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()?

> 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@redhat.com/
>
> 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.

\
 
 \ /
  Last update: 2024-05-22 13:25    [W:5.644 / U:0.048 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site