Re: [PATCH v3 2/2] mm/damon/dbgfs: Add adaptive_targets list check before enable monitor_on

[SeongJae Park]

On Wed, 20 Oct 2021 09:42:33 +0800 Xin Hao <xhao@xxxxxxxxxxxxxxxxx> wrote:

> When the ctx->adaptive_targets list is empty,
> I did some test on monitor_on interface like this.
> 
>     # echo > /sys/kernel/debug/damon/target_ids

Thanks for the change, but you missed writing 'on' to 'monitor_on' in the above
example.

> 
> Though the ctx->adaptive_targets list is empty, but the
> kthread_run still be called, and the kdamond.x thread still
> be created, this is meaningless.
> 
> So there adds a judgment in 'dbgfs_monitor_on_write',
> if the ctx->adaptive_targets list is empty, return -EINVAL.
> 
> Signed-off-by: Xin Hao <xhao@xxxxxxxxxxxxxxxxx>

Other parts looks good to me.  However, seems this commit conflicts with a
patch[1] which already merged in -mm tree.  Could you please rebase this on it?
FYI, all DAMON patches that merged in -mm tree are also applied on DAMON
development tree.  So, you could get the patch applied tree by:

    $ git remote add sj git://git.kernel.org/pub/scm/linux/kernel/git/sj/linux.git
    $ git fetch sj
    $ git checkout 0fa378d04b12

[1] https://lore.kernel.org/linux-mm/20211014073014.35754-1-sj@xxxxxxxxxx/


Thanks,
SJ

> ---
>  include/linux/damon.h |  1 +
>  mm/damon/core.c       |  5 +++++
>  mm/damon/dbgfs.c      | 13 ++++++++++---
>  3 files changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/damon.h b/include/linux/damon.h
> index 715dadd21f7c..4fce5f1f6dad 100644
> --- a/include/linux/damon.h
> +++ b/include/linux/damon.h
> @@ -316,6 +316,7 @@ void damon_destroy_scheme(struct damos *s);
> 
>  struct damon_target *damon_new_target(unsigned long id);
>  void damon_add_target(struct damon_ctx *ctx, struct damon_target *t);
> +bool damon_targets_empty(struct damon_ctx *ctx);
>  void damon_free_target(struct damon_target *t);
>  void damon_destroy_target(struct damon_target *t);
>  unsigned int damon_nr_regions(struct damon_target *t);
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 2f6785737902..c3a1374dbe0b 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -156,6 +156,11 @@ void damon_add_target(struct damon_ctx *ctx, struct damon_target *t)
>  	list_add_tail(&t->list, &ctx->adaptive_targets);
>  }
> 
> +bool damon_targets_empty(struct damon_ctx *ctx)
> +{
> +	return list_empty(&ctx->adaptive_targets);
> +}
> +
>  static void damon_del_target(struct damon_target *t)
>  {
>  	list_del(&t->list);
> diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
> index 38188347d8ab..9dee29f7d103 100644
> --- a/mm/damon/dbgfs.c
> +++ b/mm/damon/dbgfs.c
> @@ -865,12 +865,19 @@ static ssize_t dbgfs_monitor_on_write(struct file *file,
>  		return -EINVAL;
>  	}
> 
> -	if (!strncmp(kbuf, "on", count))
> +	if (!strncmp(kbuf, "on", count)) {
> +		int i;
> +
> +		for (i = 0; i < dbgfs_nr_ctxs; i++) {
> +			if (damon_targets_empty(dbgfs_ctxs[i]))
> +				return -EINVAL;
> +		}
>  		err = damon_start(dbgfs_ctxs, dbgfs_nr_ctxs);
> -	else if (!strncmp(kbuf, "off", count))
> +	} else if (!strncmp(kbuf, "off", count)) {
>  		err = damon_stop(dbgfs_ctxs, dbgfs_nr_ctxs);
> -	else
> +	} else {
>  		err = -EINVAL;
> +	}
> 
>  	if (err)
>  		ret = err;
> --
> 2.31.0