Received-SPF: pass (google.com: domain of linux-kernel+bounces-186200-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
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>
In-Reply-To: <c55648d4-cec2-48ca-9ca9-c8fc2aecc741@linux.alibaba.com>
From: Lance Yang <ioworker0@gmail.com>
Date: Wed, 22 May 2024 20:11:33 +0800
Message-ID: <CABzRoyYqCSZafj2wmdgaYutOZtNAL2hozZV24mMjYrgf_BpJmQ@mail.gmail.com>
Subject: Re: [PATCH] mm: drop the 'anon_' prefix for swap-out mTHP counters
To: Baolin Wang <baolin.wang@linux.alibaba.com>
Cc: Barry Song <21cnbao@gmail.com>, David Hildenbrand <david@redhat.com>, akpm@linux-foundation.org, 
	willy@infradead.org, ying.huang@intel.com, ryan.roberts@arm.com, 
	ziy@nvidia.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Baolin,

On Wed, May 22, 2024 at 7:24=E2=80=AFPM Baolin Wang
<baolin.wang@linux.alibaba.com> wrote:
>
>
>
> 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 sw=
ap
> >>>> 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 t=
he
> >>> interaction with shmem? At least I remember some discussion around th=
at.
> >>
> >> 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()?

Currently, PMD-mapped shmem folios are not accounted for in
THP_SWPOUT and related counters.

So, IMO, if we intend to account for them in those counters in the
future, removing
the 'anon_' prefix from the mTHP swap counters would be reasonable :)

Thanks,
Lance

>
> >                  if (folio_test_anon(folio) && folio_test_swapbacked(fo=
lio)) {
> >                          ...
> >                                  if (!add_to_swap(folio)) {
> >                                          int __maybe_unused order =3D
> > folio_order(folio);
> >
> >                                          if (!folio_test_large(folio))
> >                                                  goto activate_locked_s=
plit;
> >                                          /* Fallback to swap normal pag=
es */
> >                                          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_even=
ts(folio,
> >                                                          THP_SWPOUT_FAL=
LBACK, 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_s=
plit;
> >                                  }
> >                          }
> >                  } 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.c=
om/
> >
> > 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 accordin=
gly.
>
> Sure. I missed update the documentation, and will do in next version.
>