Re: [PATCH V1 2/2] mm/damon: Add 'age' of region tracepoint support

[SeongJae Park <sj@kernel.org>]

On Wed, 10 Nov 2021 20:13:14 +0800 Xin Hao <xhao@linux.alibaba.com> wrote:

> In patch "mm/damon: add a tracepoint", it adds a
> tracepoint for DAMON, it can monitor each region
> for each aggregation interval, Now the region add
> a new 'age' variable, some primitive would calculate
> the priority of each region as a weight, there put it
> into tracepoint, so we can easily track the change of
> its value through perf or damon-tools.

DAMON calculates the age using the address range and nr_accesses of the region,
which are already in the tracepoint.  In other words, user space can calculate
the age on their own.  Therefore I thought putting age in the tracepoint as
adding unnecessary information, at the moment of the implementation.

Of course, I would missing some use cases that need this information in the
tracepoint.  Furthermore, adding just one more value in the tracepoint wouldn't
incur a real issue.  But, I'd like to know why this is necessary and how much
benefit it provides.  Xin, could you please share that?


Thanks,
SJ

> 
> Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
> ---
>  include/trace/events/damon.h | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/include/trace/events/damon.h b/include/trace/events/damon.h
> index 2f422f4f1fb9..99ffa601e351 100644
> --- a/include/trace/events/damon.h
> +++ b/include/trace/events/damon.h
> @@ -22,6 +22,7 @@ TRACE_EVENT(damon_aggregated,
>  		__field(unsigned long, start)
>  		__field(unsigned long, end)
>  		__field(unsigned int, nr_accesses)
> +		__field(unsigned int, age)
>  	),
>  
>  	TP_fast_assign(
> @@ -30,11 +31,13 @@ TRACE_EVENT(damon_aggregated,
>  		__entry->start = r->ar.start;
>  		__entry->end = r->ar.end;
>  		__entry->nr_accesses = r->nr_accesses;
> +		__entry->age = r->age;
>  	),
>  
> -	TP_printk("target_id=%lu nr_regions=%u %lu-%lu: %u",
> +	TP_printk("target_id=%lu nr_regions=%u %lu-%lu: %u %u",
>  			__entry->target_id, __entry->nr_regions,
> -			__entry->start, __entry->end, __entry->nr_accesses)
> +			__entry->start, __entry->end,
> +			__entry->nr_accesses, __entry->age)
>  );
>  
>  #endif /* _TRACE_DAMON_H */
> -- 
> 2.31.0