Re: [PATCH v5 4/9] mm: Add test_clear_young_fast_only MMU notifier

[Oliver Upton]

On Tue, Jun 11, 2024 at 09:49:59AM -0700, James Houghton wrote:
> On Mon, Jun 10, 2024 at 10:34 PM Yu Zhao <yuzhao@xxxxxxxxxx> wrote:
> >
> > On Mon, Jun 10, 2024 at 6:22 PM James Houghton <jthoughton@xxxxxxxxxx> wrote:
> > >
> > > This new notifier is for multi-gen LRU specifically
> >
> > Let me call it out before others do: we can't be this self-serving.

Establishing motivation for a change is always a good idea. The wording
could be a bit crisper, but the connection between the new MMU notifier
and MGLRU is valuable. I do not view the wording of the changeset as
excluding other users of the 'fast' notifier.

> I think consolidating the callbacks is cleanest, like you had it in
> v2. I really wasn't sure about this change honestly, but it was my
> attempt to incorporate feedback like this[3] from v4. I'll consolidate
> the callbacks like you had in v2.

My strong preference is to have the callers expectations of the
secondary MMU be explicit. Having ->${BLAH}_fast_only() makes this
abundantly clear both at the callsite and in the implementation.

> Instead of the bitmap like you had, I imagine we'll have some kind of
> flags argument that has bits like MMU_NOTIFIER_YOUNG_CLEAR,
> MMU_NOTIFIER_YOUNG_FAST_ONLY, and other ones as they come up. Does
> that sound ok?
> 
> Do idle page tracking and DAMON need this new "fast-only" notifier? Or
> do they benefit from a generic API in other ways? Sorry if I missed
> this from some other mail.

Let's also keep in mind we aren't establishing an ABI here. If we have
direct line of sight (i.e. patches) on how to leverage the new MMU
notifier for DAMON and idle page tracking then great, let's try and
build something that satisfies all users. Otherwise, it isn't
that big of a deal if the interface needs to change slightly when
someone decides to leverage the MMU notifier for something else.

> I've got feedback saying that tying the definition of "fast" to MGLRU
> specifically is helpful. So instead of MMU_NOTIFIER_YOUNG_FAST_ONLY,
> maybe MMU_NOTIFIER_YOUNG_LRU_GEN_FAST to mean "do fast-for-MGLRU
> notifier". It sounds like you'd prefer the more generic one.
> 
> Thanks for the feedback -- I don't want to keep this series lingering
> on the list, so I'll try and get newer versions out sooner rather than
> later.

Let's make sure we get alignment on this before you proceed, I don't get
the sense that we're getting to a common understanding of where to go
with this.

-- 
Thanks,
Oliver