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

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

[SeongJae Park]

Hi Peng,


For DAMON patches, please Cc linux-mm@.  I'd also suggest Cc-ing linux-kernel@
if you don't mind.

On Fri, 19 Jul 2024 17:11:00 +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.
> 
> v2: Change the histgram array in damon_quota to dynamic allocation.
> v1: Modify global variables directly.

Please put the version history under '---' line below, as suggested[1] by the
document.  Also, adding lore links to the previous versions would be helpful.

> 
> Signed-off-by: Peng Hao <flyingpeng@tencent.com>
> ---
>  mm/damon/core.c     |  1 +
>  mm/damon/lru_sort.c | 10 +++++++---
>  2 files changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 6392f1cc97a3..ac2b736c9115 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -375,6 +375,7 @@ struct damos *damon_new_scheme(struct damos_access_pattern *pattern,
>  	INIT_LIST_HEAD(&scheme->list);
>  
>  	scheme->quota = *(damos_quota_init(quota));
> +	kfree(quota);

We don't know if the caller has something to do further with it after this
function returns.  Let's keep it do be done by the caller.

>  	/* quota.goals should be separately set by caller */
>  	INIT_LIST_HEAD(&scheme->quota.goals);
>  
> diff --git a/mm/damon/lru_sort.c b/mm/damon/lru_sort.c
> index 3de2916a65c3..3b417e413bf2 100644
> --- a/mm/damon/lru_sort.c
> +++ b/mm/damon/lru_sort.c
> @@ -148,10 +148,14 @@ 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_quota *quota = kmemdup(&damon_lru_sort_quota,
> +					    sizeof(struct damos_quota), GFP_KERNEL);

Let's do sizeof(*damon_lru_sort_quota).  Also, please keep 80 columns line size
rule[2] for DAMON code.

[1] https://docs.kernel.org/process/submitting-patches.html#the-canonical-patch-format
[2] https://docs.kernel.org/process/coding-style.html#breaking-long-lines-and-strings

> +
> +	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(
>  			/* find the pattern, and */
> @@ -161,7 +165,7 @@ 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);
>  }
> -- 
> 2.27.0

Thanks,
SJ