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

From: Xin Hao
Date: Wed Oct 20 2021 - 04:15:44 EST



On 2021/10/20 下午4:12, SeongJae Park wrote:
On Wed, 20 Oct 2021 07:21:21 +0000 SeongJae Park <sj@xxxxxxxxxx> wrote:

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.
Sorry.

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/
Ok, i will correct it.


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;
Oops, I forgot that you should 'kfree(kbuf)' before returning here. Please do
that.
Okay,  i will correct it, thank you.

Thanks,
SJ

+ }
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

--
Best Regards!
Xin Hao