Received-SPF: pass (google.com: domain of linux-kernel+bounces-2098-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20231215074619.173787-1-aravinda.prasad@intel.com>
 <CAOUHufZW8ZbNDnu3Cfc61oJ4FkXK+AbT90XpS5Ei++1_5mcFFQ@mail.gmail.com> <MW5PR11MB5907BA0C9546C069925C9955F293A@MW5PR11MB5907.namprd11.prod.outlook.com>
In-Reply-To: <MW5PR11MB5907BA0C9546C069925C9955F293A@MW5PR11MB5907.namprd11.prod.outlook.com>
From: Yu Zhao <yuzhao@google.com>
Date: Fri, 15 Dec 2023 22:41:42 -0700
Message-ID: <CAOUHufbDzy5dMcLR9ex25VdB_QBmSrW_We-2+KftZVYKNn4s9g@mail.gmail.com>
Subject: Re: mm/DAMON: Profiling enhancements for DAMON
To: "Prasad, Aravinda" <aravinda.prasad@intel.com>
Cc: "damon@lists.linux.dev" <damon@lists.linux.dev>, "linux-mm@kvack.org" <linux-mm@kvack.org>, 
	"sj@kernel.org" <sj@kernel.org>, 
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "s2322819@ed.ac.uk" <s2322819@ed.ac.uk>, 
	"Kumar, Sandeep4" <sandeep4.kumar@intel.com>, "Huang, Ying" <ying.huang@intel.com>, 
	"Hansen, Dave" <dave.hansen@intel.com>, "Williams, Dan J" <dan.j.williams@intel.com>, 
	"Subramoney, Sreenivas" <sreenivas.subramoney@intel.com>, 
	"Kervinen, Antti" <antti.kervinen@intel.com>, 
	"Kanevskiy, Alexander" <alexander.kanevskiy@intel.com>, Alan Nair <alan.nair@intel.com>, 
	Juergen Gross <jgross@suse.com>, Ryan Roberts <ryan.roberts@arm.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Fri, Dec 15, 2023 at 3:08=E2=80=AFAM Prasad, Aravinda
<aravinda.prasad@intel.com> wrote:
>
> > On Fri, Dec 15, 2023 at 12:42=E2=80=AFAM Aravinda Prasad
> > <aravinda.prasad@intel.com> wrote:
> > ...
> >
> > > This patch proposes profiling different levels of the application=E2=
=80=99s
> > > page table tree to detect whether a region is accessed or not. This
> > > patch is based on the observation that, when the accessed bit for a
> > > page is set, the accessed bits at the higher levels of the page table
> > > tree (PMD/PUD/PGD) corresponding to the path of the page table walk
> > > are also set. Hence, it is efficient to  check the accessed bits at
> > > the higher levels of the page table tree to detect whether a region i=
s
> > > accessed or not.
> >
> > This patch can crash on Xen. See commit 4aaf269c768d("mm: introduce
> > arch_has_hw_nonleaf_pmd_young()")
>
> Will fix as suggested in the commit.
>
> >
> > MGLRU already does this in the correct way. See mm/vmscan.c.
>
> I don't see access bits at PUD or PGD checked for 4K page size. Can you
> point me to the code where access bits are checked at PUD and PGD level?

There isn't any, because *the system* bottlenecks at the PTE level and
at moving memory between tiers. Optimizing at the PUD/PGD levels has
insignificant ROI for the system.

And food for thought:
1. Can a PUD/PGD cover memory from different tiers?
2. Can the A-bit in non-leaf entries work for EPT?

> > This patch also can cause USER DATA CORRUPTION. See commit
> > c11d34fa139e ("mm/damon/ops-common: atomically test and clear young
> > on ptes and pmds").
>
> Ok. Will atomically test and set the access bits.
>
> >
> > The quality of your patch makes me very much doubt the quality of your
> > paper, especially your results on Google's kstaled and MGLRU in table 6=
.2.
>
> The results are very much reproducible. We have not used kstaled/MGLRU fo=
r
> the data in Figure 3, but we linearly scan pages similar to kstaled by im=
plementing
> a kernel thread for scanning.

You have not used MGLRU, and yet your results are very much reproducible.

> Our argument for kstaled/MGLRU is that, scanning individual pages at 4K
> granularity may not be efficient for large footprint applications.

Your argument for MGLRU is based on a wrong assumption, as I have
already pointed out.

> Instead,
> access bits at the higher level of the page table tree can be used. In th=
e
> paper we have demonstrated this with DAMON but the concept can be
> applied to kstaled/MGLRU as well.

You got it backward: MGLRU introduced the concept; you fabricated a
comparison table.