Received-SPF: pass (google.com: domain of linux-kernel+bounces-186873-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
From: "Huang, Ying" <ying.huang@intel.com>
To: Baolin Wang <baolin.wang@linux.alibaba.com>, Barry Song <21cnbao@gmail.com>
Cc: 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: <c55648d4-cec2-48ca-9ca9-c8fc2aecc741@linux.alibaba.com> (Baolin
	Wang's message of "Wed, 22 May 2024 19:24:45 +0800")
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>
	<c55648d4-cec2-48ca-9ca9-c8fc2aecc741@linux.alibaba.com>
Date: Thu, 23 May 2024 09:02:10 +0800
Message-ID: <87a5khbast.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

Baolin Wang <baolin.wang@linux.alibaba.com> writes:

> On 2024/5/22 18:40, Barry Song wrote:
>> 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.=
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(fol=
io)) {
>>                          ...
>>                                  if (!add_to_swap(folio)) {
>>                                          int __maybe_unused order =3D
>> folio_order(folio);
>>                                          if
>> (!folio_test_large(folio))
>>                                                  goto activate_locked_sp=
lit;
>>                                          /* Fallback to swap normal page=
s */
>>                                          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_event=
s(folio,
>>                                                          THP_SWPOUT_FALL=
BACK, 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_sp=
lit;
>>                                  }
>>                          }
>>                  } 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.=20
>
> 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.c=
om/

Yes.  I don't find that it's necessary to distinguish anonymous and
shmem mTHP swap-out now.  If we need it in the future, we can add that
at that time.

>> 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 according=
ly.
>
> Sure. I missed update the documentation, and will do in next version.

--
Best Regards,
Huang, Ying