Hello Xin,
On Tue, 19 Oct 2021 22:26:04 +0800 Xin Hao <xhao@xxxxxxxxxxxxxxxxx> wrote:
> When the ctx->adaptive_targets list is empty,
> i did some test on monitor_on interface like this.
s/i/I/
>
> echo on > /sys/kernel/debug/damon/monitor_on
> [ 851.988307] damon: kdamond (5390) starts
Could you please add four spaces in the beginning of this example terminal
outputs? Also, I think confirming targets are empty like below before writing
'on' could make this example better.
# cat /sys/kernel/debug/damon/target_ids
#
or, like below.
# echo > /sys/kernel/debug/damon/target_ids
>
> 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>
> ---
> include/linux/damon.h | 1 +
> mm/damon/core.c | 5 +++++
> mm/damon/dbgfs.c | 10 ++++++++--
> 3 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/damon.h b/include/linux/damon.h
> index 715dadd21f7c..06acf73fe9db 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);
> +int damon_target_empty(struct damon_ctx *ctx);
Seems 'bool' would be better for the return type?
> 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..c651b49249e5 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);
> }
>
> +int damon_target_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..bac42dd358d1 100644
> --- a/mm/damon/dbgfs.c
> +++ b/mm/damon/dbgfs.c
> @@ -865,9 +865,15 @@ 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_target_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
> err = -EINVAL;
Please use braces for all above branches:
https://docs.kernel.org/process/coding-style.html#placing-braces-and-spaces
Thanks,
SJ
> --
> 2.31.0
