Re: [RFC PATCH v2 18/28] mm/damon: trace probe_hits

[SeongJae Park <sj@kernel.org>]

On Wed, 13 May 2026 14:07:32 -0400 Steven Rostedt <rostedt@goodmis.org> wrote:

> On Tue, 12 May 2026 07:36:33 -0700
> SeongJae Park <sj@kernel.org> wrote:
> 
> > Introduce a new tracepoint for exposing the per-region per-probe
> > positive sample count via tracefs.
> > 
> > Signed-off-by: SeongJae Park <sj@kernel.org>
> > ---
> >  include/trace/events/damon.h | 36 ++++++++++++++++++++++++++++++++++++
> >  mm/damon/core.c              |  7 +++++++
> >  2 files changed, 43 insertions(+)
> > 
> > diff --git a/include/trace/events/damon.h b/include/trace/events/damon.h
> > index 7e25f4469b81b..d7b94c7640217 100644
> > --- a/include/trace/events/damon.h
> > +++ b/include/trace/events/damon.h
> > @@ -130,6 +130,42 @@ TRACE_EVENT(damon_monitor_intervals_tune,
> >  	TP_printk("sample_us=%lu", __entry->sample_us)
> >  );
> >  
> > +TRACE_EVENT(damon_aggregated_v2,
> > +
> > +	TP_PROTO(unsigned int target_id, struct damon_region *r,
> > +		unsigned int nr_regions, unsigned int nr_probes),
> > +
> > +	TP_ARGS(target_id, r, nr_regions, nr_probes),
> > +
> > +	TP_STRUCT__entry(
> > +		__field(unsigned long, target_id)
> > +		__field(unsigned long, start)
> > +		__field(unsigned long, end)
> > +		__field(unsigned int, nr_regions)
> > +		__field(unsigned int, nr_accesses)
> > +		__field(unsigned int, age)
> > +		__dynamic_array(unsigned char, probe_hits, nr_probes)
> > +	),
> > +
> > +	TP_fast_assign(
> > +		__entry->target_id = target_id;
> > +		__entry->start = r->ar.start;
> > +		__entry->end = r->ar.end;
> > +		__entry->nr_regions = nr_regions;
> > +		__entry->nr_accesses = r->nr_accesses;
> > +		__entry->age = r->age;
> > +		memcpy(__get_dynamic_array(probe_hits), r->probe_hits,
> > +			sizeof(*r->probe_hits) * nr_probes);
> > +	),
> > +
> > +	TP_printk("target_id=%lu nr_regions=%u %lu-%lu: %u %u probe_hits=%s",
> > +			__entry->target_id, __entry->nr_regions,
> > +			__entry->start, __entry->end,
> > +			__entry->nr_accesses, __entry->age,
> > +			__print_hex(__get_dynamic_array(probe_hits),
> > +				__get_dynamic_array_len(probe_hits)))
> > +);
> > +
> >  TRACE_EVENT(damon_aggregated,
> >  
> >  	TP_PROTO(unsigned int target_id, struct damon_region *r,
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index fe6c789f2cecb..14b15c9876516 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -1905,6 +1905,11 @@ static void kdamond_reset_aggregated(struct damon_ctx *c)
> >  {
> >  	struct damon_target *t;
> >  	unsigned int ti = 0;	/* target's index */
> > +	unsigned int nr_probes = 0;
> > +	struct damon_probe *probe;
> > +
> > +	damon_for_each_probe(probe, c)
> > +		nr_probes++;
> 
> Is the above logic needed when the tracepoint isn't enabled? If not, then you could add:
> 
> 	if (trace_damon_aggregated_v2_enabled()) {
> 		damon_for_each_probe(probe, c)
> 			nr_probes++;
> 	}
> 
> And change the tracepoint to be a conditional tracepoint:
> 
> TRACE_EVENT_CONDITION(damon_aggregated_v2,
> 
> 	TP_PROTO(..),
> 
> 	TP_ARGS(..),
> 
> 	TP_CONDITION(nr_probes > 0),
> 
> 	[..]
> 
> And then the tracepoint is only triggered if nr_probes is greater than zero
> (to handle races between the tracepoint being enabled in between the above
> check and where it triggers).

It is not needed when the tracepoint isn't enabled.  I will follow your
suggestion in the next revision.  Thank you for the nice suggestion, Steven!

Btw, if you don't mind, may I ask your opinion about the name having '_v2'
suffix?  I chose that as an RFC phase temporal name that doesn't break the
compatibility, planning to give it a better name later.  But I start feeling
just extending the original one might be another option because tracepoints are
not strict stable ABI to my understanding, and the change of the TP_prink
format should be simple enough (append the probe_hits= part) that the user
space could reasonably deal with.


Thanks,
SJ

[...]