Re: [PATCH v4] mm/damon/lru_sort: adjust local variable to dynamic allocation

Re: [PATCH v4] mm/damon/lru_sort: adjust local variable to dynamic allocation

[SeongJae Park]

Hi Peng,


On Mon, 22 Jul 2024 13:30:33 +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.
> 
> So adjust local variable to dynamic allocation.
> 
> Signed-off-by: Peng Hao <flyingpeng@tencent.com>
> ---
>  v3 -> v4: adjust release point, tweak format, etc.
>  v2 -> v3: don't change histgram array in damon_quota, dynamically 
>            allocating local variables 
>  v1 -> v2: don't modify global variables and change the histgram array in
>            damon_quota to dynamic allocation

To help people finding the previous versions easy, I'd suggest adding
lore.kernel.org links for the versions here together.  For an example, you
could refer to https://lore.kernel.org/20240621163626.74815-1-sj@kernel.org

> 
>  mm/damon/lru_sort.c | 16 ++++++++++++----
>  1 file changed, 12 insertions(+), 4 deletions(-)

This patch cannot cleanly applied on latest mm-unstable tree.  Could you please
rebase this to it and send again?

> 
> diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
> index 3de2916a65c3..57bcb3b531ff 100644
> --- a/mm/damon/lru_sort.c
> +++ b/mm/damon/lru_sort.c
> @@ -148,12 +148,18 @@ static struct damon_target *target;
>  static struct damos *damon_lru_sort_new_scheme(
>  		struct damos_access_pattern *pattern, enum damos_action action)
>  {
> -	struct damos_quota quota = damon_lru_sort_quota;
> +	struct damos *damos;
> +
> +	struct damos_quota *quota = kmemdup(&damon_lru_sort_quota,
> +				    sizeof(damon_lru_sort_quota), GFP_KERNEL);

I don't think we need the above empty space between two variables definitions.

> +
> +	if (!quota)
> +		return NULL;
>  
>  	/* Use half of total quota for hot/cold pages sorting */
> -	quota.ms = quota.ms / 2;
> +	quota->ms = quota->ms / 2;
>  
> -	return damon_new_scheme(
> +	damos = damon_new_scheme(
>  			/* find the pattern, and */
>  			pattern,
>  			/* (de)prioritize on LRU-lists */
> @@ -161,9 +167,11 @@ static struct damos *damon_lru_sort_new_scheme(
>  			/* for each aggregation interval */
>  			0,
>  			/* under the quota. */
> -			&quota,
> +			quota,
>  			/* (De)activate this according to the watermarks. */
>  			&damon_lru_sort_wmarks);
> +	kfree(quota);
> +	return damos;
>  }
>  
>  /* Create a DAMON-based operation scheme for hot memory regions */
> -- 
> 2.27.0

Other than above comments, looks good to me overall.


Thanks,
SJ