Received-SPF: pass (google.com: domain of linux-kernel+bounces-186881-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
From: "Huang, Ying" <ying.huang@intel.com>
To: Barry Song <21cnbao@gmail.com>
Cc: Baolin Wang <baolin.wang@linux.alibaba.com>,  David Hildenbrand
 <david@redhat.com>,  akpm@linux-foundation.org,  willy@infradead.org,
  ryan.roberts@arm.com,  ziy@nvidia.com,  linux-mm@kvack.org,
  linux-kernel@vger.kernel.org
Subject: Re: [PATCH] mm: drop the 'anon_' prefix for swap-out mTHP counters
In-Reply-To: <CAGsJ_4zSuOTPi+zkS_kvS5T0MsdMBR+2gpXukJt0aMPrEnCDZg@mail.gmail.com>
	(Barry Song's message of "Wed, 22 May 2024 22:40:20 +1200")
References: <0e2a6f232e7579a2e4407ecf075531980d97f286.1716367360.git.baolin.wang@linux.alibaba.com>
	<22ac01a3-ddbb-4114-88cd-ad1a31982dad@redhat.com>
	<51ba1fc1-fd77-4601-8d27-459162fd008c@linux.alibaba.com>
	<CAGsJ_4zSuOTPi+zkS_kvS5T0MsdMBR+2gpXukJt0aMPrEnCDZg@mail.gmail.com>
Date: Thu, 23 May 2024 09:14:10 +0800
Message-ID: <875xv5ba8t.fsf@yhuang6-desk2.ccr.corp.intel.com>
User-Agent: Gnus/5.13 (Gnus v5.13)
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

Barry Song <21cnbao@gmail.com> writes:

> On Wed, May 22, 2024 at 9:38=E2=80=AFPM 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 tha=
t.
>>
>> 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
>> =E2=80=98anon_=E2=80=99 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.c=
om/
>
> 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.

For example, before commit 5ed890ce5147 ("mm: vmscan: avoid split during
shrink_folio_list()"), if folio_entire_mapcount() =3D=3D 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.

>
>                 if (folio_test_anon(folio) && folio_test_swapbacked(folio=
)) {
>                         ...
>                                 if (!add_to_swap(folio)) {
>                                         int __maybe_unused order =3D
> folio_order(folio);
>
>                                         if (!folio_test_large(folio))
>                                                 goto activate_locked_spli=
t;
>                                         /* Fallback to swap normal pages =
*/
>                                         if (split_folio_to_list(folio,
> folio_list))
>                                                 goto activate_locked;
> #ifdef CONFIG_TRANSPARENT_HUGEPAGE
>                                         if (nr_pages >=3D HPAGE_PMD_NR) {
>                                                 count_memcg_folio_events(=
folio,
>                                                         THP_SWPOUT_FALLBA=
CK, 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_spli=
t;
>                                 }
>                         }
>                 } 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_s=
wpout_*
> 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 v=
mscan.
>
> 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 accordingl=
y.
>
>>
>> [1]
>> https://lore.kernel.org/all/05d0096e4ec3e572d1d52d33a31a661321ac1551.171=
3755580.git.baolin.wang@linux.alibaba.com/
>

--
Best Regards,
Huang, Ying