On Tue, 19 Oct 2021 15:42:43 +0000 SeongJae Park <sj@xxxxxxxxxx> wrote:
> 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?
Also, I think 'damon_targets_empty()' (s/target/targets/) might be a better
naming?
Thanks,
SJ
>
> > 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
