Re: [PATCH v13 05/15] mm/damon: Adaptively adjust regions

[Leonard Foerster]

On 2020-05-25T11:15:02+02:00 SeongJae Park <sjpark@xxxxxxxxxx> wrote:

> From: SeongJae Park <sjpark@xxxxxxxxx>
> 
> At the beginning of the monitoring, DAMON constructs the initial regions
> by evenly splitting the memory mapped address space of the process into
> the user-specified minimal number of regions.  In this initial state,
> the assumption of the regions (pages in same region have similar access
> frequencies) is normally not kept and thus the monitoring quality could
> be low.  To keep the assumption as much as possible, DAMON adaptively
> merges and splits each region.
> 
> For each ``aggregation interval``, it compares the access frequencies of
> adjacent regions and merges those if the frequency difference is small.
> Then, after it reports and clears the aggregated access frequency of
> each region, it splits each region into two regions if the total number
> of regions is smaller than the half of the user-specified maximum number
> of regions.
> 
> In this way, DAMON provides its best-effort quality and minimal overhead
> while keeping the bounds users set for their trade-off.
> 
> Signed-off-by: SeongJae Park <sjpark@xxxxxxxxx>
> ---
> [...]
> +/*
> + * splits every target region into two randomly-sized regions
> + *
> + * This function splits every target region into two random-sized regions if
> + * current total number of the regions is equal or smaller than half of the
> + * user-specified maximum number of regions.  This is for maximizing the
> + * monitoring accuracy under the dynamically changeable access patterns.  If a
> + * split was unnecessarily made, later 'kdamond_merge_regions()' will revert
> + * it.
> + */
> +static void kdamond_split_regions(struct damon_ctx *ctx)
> +{
> +	struct damon_task *t;
> +	unsigned int nr_regions = 0;
> +	static unsigned int last_nr_regions;
> +	int nr_subregions = 2;
> +
> +	damon_for_each_task(t, ctx)
> +		nr_regions += nr_damon_regions(t);
> +
> +	if (nr_regions > ctx->max_nr_regions / 2)
> +		return;
> +
> +	/* If number of regions is not changed, we are maybe in corner case */
> +	if (last_nr_regions == nr_regions &&
> +			nr_regions < ctx->max_nr_regions / 3)
> +		nr_subregions = 3;
> +
> +	damon_for_each_task(t, ctx)
> +		damon_split_regions_of(ctx, t, nr_subregions);
> +
> +	if (!last_nr_regions)
> +		last_nr_regions = nr_regions;

So we are only setting last_nr_regions once when we first come along
here (when last_nr_regions == 0). Thus we are checking from now on if
nr_regions is the same as nr_regions was before the first ever split. So
we are doing the three-way split whenever nr_regions has come to the
initial number of regions. Is this actually what we want? The naming
suggests that we want to check against the number before the last split
to detect if we have moved into a spot where we are splitting and
merging back and forth between two states (this is the corner case we
are talking about?).

Or am I misunderstanding the intention here?

Leonard