Linux-mm Archive on lore.kernel.org
 help / color / mirror / Atom feed
From: SeongJae Park <sj@kernel.org>
To: Ravi Jonnalagadda <ravis.opensrc@gmail.com>
Cc: SeongJae Park <sj@kernel.org>,
	damon@lists.linux.dev, linux-mm@kvack.org,
	linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
	akpm@linux-foundation.org, corbet@lwn.net, bijan311@gmail.com,
	ajayjoshi@micron.com, honggyu.kim@sk.com, yunjeong.mun@sk.com,
	bharata@amd.com, Akinobu Mita <akinobu.mita@gmail.com>
Subject: Re: [RFC PATCH 0/7] mm/damon: hardware-sampled access reports + AMD IBS Op example
Date: Mon, 18 May 2026 23:19:04 -0700	[thread overview]
Message-ID: <20260519061905.89681-1-sj@kernel.org> (raw)
In-Reply-To: <20260516223439.4033-1-ravis.opensrc@gmail.com>

+ Akinobu

Hello Ravi,

On Sat, 16 May 2026 15:34:25 -0700 Ravi Jonnalagadda <ravis.opensrc@gmail.com> wrote:

> Hi all,
> 
> This is an RFC, not for merge.  The series exercises and validates
> damon_report_access() -- the consumer API SeongJae introduced in [1]
> -- as a substrate for ingesting access reports from hardware-sampling
> sources.  The series includes one worked-example backend, an AMD IBS
> Op module (damon_ibs.ko), that runs on Zen 3+ silicon via the
> existing perf event subsystem.

Thank you for sharing this great RFC series!

[...]
> Why a hardware-source primitive complements existing primitives
> ===============================================================
[...]
> Both primitives produce a view of hotness that converges to the
> true distribution over the aggregation interval.  For systems where
> the address space is small relative to the aggregation rate, this is
> the right tool.  On large heterogeneous-memory systems with goal-
> driven schemes asking the closed-loop tuner to converge on a target
> distribution, a complementary lower-latency view of accesses can
> tighten the loop -- reducing the time DAMON's nr_accesses takes to
> reflect the workload's actual access distribution, which in turn
> reduces ramp duration and oscillation amplitude during convergence
> of goal-driven schemes.
> 
> A hardware-sampling primitive provides this complementary view:
> hardware retirement records each access at its natural event rate,
> with a physical address per sample, independent of TLB state and
> independent of the unmap/fault path.

Yes, I fully agree.  Different multiple access check primitives have different
characteristics.

[...]

> Demonstration
> =============
[...]
> In both regimes, convergence to target is quick, and the workload's
> measured DRAM share then holds within 1.3 percentage points of
> target with standard deviation under 1.3 percentage points, sustained
> over runs of 15-30 minutes per target.

I understand this demonstration shows your AMD IBS-based version of DAMON is
functioning as expected.  Thank you for sharing this!

[...]
> What's in this series
> =====================
> 
>   Patch 1.  mm/damon/core: refcount ops owner module to prevent
>             rmmod UAF
>   Patch 2.  mm/damon/paddr: export damon_pa_* ops for IBS module
>   Patch 3.  mm/damon/core: replace mutex-protected report buffer
>             with per-CPU lockless ring
>   Patch 4.  mm/damon/core: flat-array snapshot + bsearch in ring-
>             drain loop
>   Patch 5.  mm/damon: add sysfs binding and dispatch hookup for
>             paddr_ibs operations
>   Patch 6.  mm/damon/core: accept paddr_ibs in node_eligible_mem_bp
>             ops check
>   Patch 7.  mm/damon/damon_ibs: add AMD IBS-based access sampling
>             backend
> 
> Patches 1, 3, and 4 are general infrastructure that benefits any
> consumer of damon_report_access().  Patches 2, 5, 6, and 7 are the
> worked-example backend (paddr_ibs ops, sysfs binding, IBS module).

I didn't read the detailed code of each patch.  But my high level understanding
is as below.

Patches 1 and 2 are needed for supporting loadable module-based DAMON operation
sets (access sampling backend).

Patch 3 is needed for supporting access check primitives that can provide the
access information in only nmi context.  It can also speedup the access
reporting in general, though.

Patch 4 makes DAMON's internal reported access information retrieval faster, so
will help any reporting-based DAMON operation set use case.

Patches 5-7 are required for only the IBS-based DAMON operations set
(paddr_ibs).

So I agree patch 4 is a general infrastructure improvement that benefits
multiple use cases.

Patch 3 is also arguably general infrastructure improvement, as it will make
the reporting faster in general.

Patch 1 is not technically coupled with paddr_ibs, and will be needed for
general loadable module based access check primitives.  But, should we support
lodable modules?  If so, why?

Patch 2 is also not technically coupled with paddr_ibs, to my understanding, so
should be categorized together with patch 1?  In other words, if we agree we
should support lodable modules based DAMON operation sets, this should be
useful for not only paddr_ibs but more general cases.

Correct me if I'm wrong.

> 
> 
> Patches worth folding into damon/next
> =====================================
> 
> Patches 1, 3, and 4 are not specific to IBS or to this RFC's
> backend.  Each is preparatory infrastructure that any consumer of
> damon_report_access() will need:
> 
>   - Patch 1 (refcount ops owner) -- any modular ops set, including
>     out-of-tree backends, needs clean module unload to avoid UAF
>     on damon_unregister_ops.
>   - Patch 3 (per-CPU lockless ring) -- damon_report_access() cannot
>     be called from NMI context with the current mutex-protected
>     buffer.  Hardware samplers all need NMI-safe submission.
>   - Patch 4 (flat-array snapshot + bsearch drain) -- the linear-
>     scan drain is O(reports x regions) and exceeds the sample
>     interval at high-CPU x large-region products.  Bsearch brings
>     it to O(reports x log regions).
> 
> If these belong directly on damon/next as preparatory patches for
> damon_report_access() rather than living inside an IBS-specific
> track, we are happy to rebase and resend them that way.

So I'm bit unsure about patch 1.  If we don't have a plan to support lodable
modules based DAMON operations set, we might not need it for now.

For patches 3 and 4, I agree those will be useful in general.  Nonetheless, I'd
slightly prefer to do that optimizations at the later part of the long term
project.

> 
> 
> Relation to prior and ongoing work
> ==================================
> 
> The IBS sampling pattern in patch 7 -- attr.config=0 to use IBS Op
> default config, dc_phy_addr_valid filter, NMI-safe sample submission
> -- is derived from concepts in Bharata B Rao's pghot RFC v5 [3].
> The attribution header is in mm/damon/damon_ibs.c and the patch
> carries a Suggested-by: trailer.
> 
> Bharata's pghot v7 [4] introduces a different IBS driver targeting
> the new IBS Memory Profiler (IBS-MProf) facility, which Bharata
> describes as a facility "that will be present in future AMD
> processors" -- a separate IBS instance from the one this RFC's
> backend uses. This version of driver based out of v5 [3] is an
> example of how DAMON can be benefited from AMD IBS Hardware
> source and validates importance of IBS information indepedently.
> It is not meant to be merged in the current form.
> @Bharata if you see a path where IBS samples can be consumed
> by DAMON at some point, will be happy to collaborate.
>  
> Akinobu Mita's perf-event-based access-check RFC [5] explores a
> configurable perf-event-driven access source for DAMON.  IBS has
> vendor-specific MSR setup beyond what perf_event_attr alone
> expresses (e.g. dc_phy_addr_valid filtering on the produced sample,
> not on the perf attr), so the IBS path here appears complementary
> to [5] -- operators choose based on whether their hardware sampler
> fits stock perf or needs additional kernel-side setup.

So apparently there are multiple approaches to develop and use h/w-based access
monitoring.  Akinobu and you are trying to do that using DAMON as the frontend,
and already made the working prototypes.  There were more people who showed
interest and will to contribute to this project other than you, too.  I 100%
agree h/w-based access monitoring can be useful, and I of course thinking using
DAMON as the fronend is the right approach.  I'm all for making this
upstreamed.

I was therefore spending time on thinking about in what long-term maintainable
shape this capability can successfully be upstreamed.  I suggested
damon_report_access() as the internal interface between DAMON and the h/w-based
access check primitives, and apparently we all (I, Ravi and Akinobu in this
context) agreed.  Akinobu thankfully revisioned his implementation based on
damon_report_access() interface.  Ravi also implemented this RFC based on the
interface.

After making the consensus with Akinobu, I was taking time on the user space
interface.  When I was discussing with Akinobu, my idea was extending the user
interface for the page faults based monitoring v3 [1].  But, recently I decided
to make this more general, so proposed data attributes monitoring extension [2]
at LSFMMBPF.  The patch series for the initial change [3] is merged into mm-new
for more testing, today.  The cover letter of the patch series is also sharing
how it will be extended for h/w based access monitoring in long term.

I of course want us to go in this direction.  I believe you already had chances
to take a look on the long term plan and didn't make some voice because you
don't strongly disagree about the plan.  If not, please make a voice.

Assuming you don't have concern on the long term plan yet, I will take time to
write down more formal and detailed plan.  It will explain the overall roadmap,
timeline and how we could collaborate.  On top of that, we could further
discuss.

> 
> 
> Specific asks
> =============
> 
> To SeongJae:
> 
>   1. Patches 1, 3, and 4 are infrastructure that benefits any consumer
>      of damon_report_access(), not just the IBS backend in this RFC.
>      Would these belong directly on damon/next as preparatory patches
>      for damon_report_access(), rather than living inside an
>      IBS-specific track?  Happy to rebase and resend them that way if
>      you'd prefer that shape.  Tested-by: tags can come along.

I'm still thinking about how we can collaborate well.  The answer for the above
question would be a part of that.  In other words, I have no good answer right
now, sorry.  Could you please give me more time to think more and share the
plan?  I will share the plan as another mail.  On the thread, we could further
discuss.  Of course, we could have DAMON beer/coffee/tea chats [4] like
additional discussions before/after/during the plan discussion.

So, long story short, we agreed this project (h/w-based data access monitoring)
should be upstreamed.  But give me little more time on thinking about how we
will do it and collaborate.  It will take some time.  Please bear in mind.
Sorry for making you wait, but I pretty sure and promise that we will
eventually make it.

[1] https://lore.kernel.org/20251208062943.68824-1-sj@kernel.org
[2] https://lwn.net/Articles/1071256/
[3] https://lore.kernel.org/20260518234119.97569-1-sj@kernel.org
[4] https://docs.google.com/document/d/1v43Kcj3ly4CYqmAkMaZzLiM2GEnWfgdGbZAH3mi2vpM/edit?usp=sharing


Thanks,
SJ

[...]


      parent reply	other threads:[~2026-05-19  6:19 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-05-16 22:34 [RFC PATCH 0/7] mm/damon: hardware-sampled access reports + AMD IBS Op example Ravi Jonnalagadda
2026-05-16 22:34 ` [RFC PATCH 1/7] mm/damon/core: refcount ops owner module to prevent rmmod UAF Ravi Jonnalagadda
2026-05-16 22:34 ` [RFC PATCH 2/7] mm/damon/paddr: export damon_pa_* ops for IBS module Ravi Jonnalagadda
2026-05-16 22:34 ` [RFC PATCH 3/7] mm/damon/core: replace mutex-protected report buffer with per-CPU lockless ring Ravi Jonnalagadda
2026-05-16 22:34 ` [RFC PATCH 4/7] mm/damon/core: flat-array snapshot + bsearch in ring-drain loop Ravi Jonnalagadda
2026-05-16 22:34 ` [RFC PATCH 5/7] mm/damon: add sysfs binding and dispatch hookup for paddr_ibs operations Ravi Jonnalagadda
2026-05-16 22:34 ` [RFC PATCH 6/7] mm/damon/core: accept paddr_ibs in node_eligible_mem_bp ops check Ravi Jonnalagadda
2026-05-16 22:34 ` [RFC PATCH 7/7] mm/damon/damon_ibs: add AMD IBS-based access sampling backend Ravi Jonnalagadda
2026-05-19  6:19 ` SeongJae Park [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20260519061905.89681-1-sj@kernel.org \
    --to=sj@kernel.org \
    --cc=ajayjoshi@micron.com \
    --cc=akinobu.mita@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=bharata@amd.com \
    --cc=bijan311@gmail.com \
    --cc=corbet@lwn.net \
    --cc=damon@lists.linux.dev \
    --cc=honggyu.kim@sk.com \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=ravis.opensrc@gmail.com \
    --cc=yunjeong.mun@sk.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox