Re: [PATCH v2] damon: adjust large arrays to dynamic allocation

[SeongJae Park <sj@kernel.org>]

Hi Peng,

On Fri, 12 Jul 2024 09:07:42 +0800 flyingpenghao@gmail.com wrote:

> From: Peng Hao <flyingpeng@tencent.com>
> 
> When KASAN is enabled and built with clang:
>     mm/damon/lru_sort.c:199:12: error: stack frame size (2328) exceeds limit (2048) in 'damon_lru_sort_apply_parameters' [-Werror,-Wframe-larger-than]
>     static int damon_lru_sort_apply_parameters(void)
>                ^
>     1 error generated.
> 
> This is because damon_lru_sort_quota contains a large array, and
> assigning this variable to a local variable causes a large amount of
> stack space to be occupied.

Is this a successor of a patch[1] that you sent yesterday?  If so, could you
please explain what's the change from the version and why such changes have
made?

[1] https://lore.kernel.org/20240711081051.66560-1-flyingpeng@tencent.com

> 
> Signed-off-by: Peng Hao <flyingpeng@tencent.com>
> ---
>  include/linux/damon.h | 2 +-
>  mm/damon/core.c       | 8 +++++++-
>  2 files changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/damon.h b/include/linux/damon.h
> index f7da65e1ac04..8ba0457ab538 100644
> --- a/include/linux/damon.h
> +++ b/include/linux/damon.h
> @@ -229,7 +229,7 @@ struct damos_quota {
>  	unsigned long charge_addr_from;
>  
>  	/* For prioritization */
> -	unsigned long histogram[DAMOS_MAX_SCORE + 1];
> +	unsigned long *histogram; //size:DAMOS_MAX_SCORE + 1;
>  	unsigned int min_score;
>  
>  	/* For feedback loop */
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 6392f1cc97a3..0614b49fbc35 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -358,6 +358,10 @@ struct damos *damon_new_scheme(struct damos_access_pattern *pattern,
>  {
>  	struct damos *scheme;
>  
> +	unsigned long *histogram = kmalloc(DAMOS_MAX_SCORE + 1, GFP_KERNEL);
> +	if (!histogram)
> +		return NULL;
> +
>  	scheme = kmalloc(sizeof(*scheme), GFP_KERNEL);
>  	if (!scheme)
>  		return NULL;

Some code allocate damos_quota objects without damon_new_scheme().  For
example, damon_lru_sort_quota.  I think such code should also be updated, to
get this change?  I think it is simpler to keep the histogram as an array as
is and change the stack-allocations as I suggested on the v1, though.

> @@ -375,6 +379,7 @@ struct damos *damon_new_scheme(struct damos_access_pattern *pattern,
>  	INIT_LIST_HEAD(&scheme->list);
>  
>  	scheme->quota = *(damos_quota_init(quota));
> +	cheme->quota.histogram = histogram;

I think this should s/cheme/scheme/?  Have you checked if this can be compiled?

>  	/* quota.goals should be separately set by caller */
>  	INIT_LIST_HEAD(&scheme->quota.goals);
>  
> @@ -408,6 +413,7 @@ static void damon_del_scheme(struct damos *s)
>  
>  static void damon_free_scheme(struct damos *s)
>  {
> +	kfree(s->quota.histogram);
>  	kfree(s);
>  }
>  
> @@ -1246,7 +1252,7 @@ static void damos_adjust_quota(struct damon_ctx *c, struct damos *s)
>  		return;
>  
>  	/* Fill up the score histogram */
> -	memset(quota->histogram, 0, sizeof(quota->histogram));
> +	memset(quota->histogram, 0, sizeof(*quota->histogram) * (DAMOS_MAX_SCORE + 1));
>  	damon_for_each_target(t, c) {
>  		damon_for_each_region(r, t) {
>  			if (!__damos_valid_target(r, s))
> -- 
> 2.27.0


Thanks,
SJ