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

[SeongJae Park]

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