Re: [RFC PATCH] mm/damon/core: use number of passed access sampling as a timer

[SeongJae Park]

On Sun, 27 Aug 2023 00:37:27 +0000 SeongJae Park <sj@xxxxxxxxxx> wrote:

> DAMON sleeps for sampling interval after each sampling, and check if
> it's time for further doing aggregation and ops updating using
> ktime_get_coarse_ts64() and baseline timestamps for the two periodic
> operations.  That's for making the operations occur at deterministic
> timing.  However, it turned out it could still result in indeterministic
> and even not-that-intuitive results.
> 
> After all, timer functions, and especially sleep functions that DAMON
> uses to wait for specific timing, could contain some errors.  Those
> errors are legal, so no problem.  However, depending on such legal
> timing errors, the nr_accesses can be larger than aggregation interval
> divided by sampling interval.  For example, with the default setting (5
> ms sampling interval and 100 ms aggregation interval) we frequently show
> regions having nr_accesses larger than 20.  Also, if the execution of a
> DAMOS scheme takes a long time, next aggregation could happen before
> enough number of samples are collected.
> 
> Since access check sampling is the smallest unit work of DAMON, using
> the number of passed sampling intervals as the DAMON-internal timer can
> easily avoid these problems.  That is, convert aggregation and ops
> update intervals to numbers of sampling intervals that need to be passed
> before those operations be executed, count the number of passed sampling
> intervals, and invoke the operations as soon as the specific amount of
> sampling intervals passed.  Make the change.
> 
> Signed-off-by: SeongJae Park <sj@xxxxxxxxxx>
> ---
>  include/linux/damon.h | 14 ++++++--
>  mm/damon/core.c       | 84 +++++++++++++++++++------------------------
>  2 files changed, 48 insertions(+), 50 deletions(-)
> 
[...]
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 988dc39e44b1..83af336bb0e6 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
[...]
> @@ -577,6 +580,9 @@ static void damon_update_monitoring_results(struct damon_ctx *ctx,
>   */
>  int damon_set_attrs(struct damon_ctx *ctx, struct damon_attrs *attrs)
>  {
> +	unsigned long sample_interval;
> +	unsigned long remaining_interval_us;
> +
>  	if (attrs->min_nr_regions < 3)
>  		return -EINVAL;
>  	if (attrs->min_nr_regions > attrs->max_nr_regions)
> @@ -584,6 +590,20 @@ int damon_set_attrs(struct damon_ctx *ctx, struct damon_attrs *attrs)
>  	if (attrs->sample_interval > attrs->aggr_interval)
>  		return -EINVAL;
>  
> +	sample_interval = attrs->sample_interval ? attrs->sample_interval : 1;
> +
> +	/* adjust next_aggregation_sis */
> +	remaining_interval_us = ctx->attrs.sample_interval *
> +		(ctx->next_aggregation_sis - ctx->passed_sample_intervals);
> +	ctx->next_aggregation_sis = ctx->passed_sample_intervals +
> +		remaining_interval_us / sample_interval;
> +
> +	/* adjust next_ops_update_sis */
> +	remaining_interval_us = ctx->attrs.sample_interval *
> +		(ctx->next_ops_update_sis - ctx->passed_sample_intervals);
> +	ctx->next_ops_update_sis = ctx->passed_sample_intervals +
> +		remaining_interval_us / sample_interval;

The above remaining interval based adjustments mean we will respect the old
intervals for one more time.  However, user wants to change it right now, so
respecting it yet makes no sense.  Also, since this function is to be called
while no aggregation is ongoing, the remaining_interval_us for
next_aggregation_sis will be always zero.  So, simply ignoring past and
resetting the timings as below may make better sense.

    ctx->next_aggregation_sis = ctx->passed_sample_intervals +
    		attrs->aggr_interval / sample_interval;

    ctx->next_ops_update_sis = ctx->passed_sample_intervals +
    		attrs->ops_update_interval / sample_interval;

I will replace the adjustments as above.


Thanks,
SJ

[...]