Re: [RFC v8 01/10] mm/damon/debugfs: Allow users to set initial monitoring target regions

2020-08-31 Thread SeongJae Park
On Mon, 31 Aug 2020 20:08:44 +0200 Marco Elver  wrote:

> On Mon, Aug 31, 2020 at 12:47PM +0200, SeongJae Park wrote:
> [...]
> > diff --git a/mm/damon.c b/mm/damon.c
> > index 7e3c8c82a010..9815d22fc4de 100644
> > --- a/mm/damon.c
> > +++ b/mm/damon.c
> > @@ -2001,6 +2001,147 @@ static ssize_t debugfs_record_write(struct file 
> > *file,
> > return ret;
> >  }
> >  
> > +static ssize_t sprint_init_regions(struct damon_ctx *c, char *buf, ssize_t 
> > len)
> > +{
> > +   struct damon_target *t;
> > +   struct damon_region *r;
> > +   int written = 0;
> > +   int rc;
> > +
> > +   damon_for_each_target(t, c) {
> > +   damon_for_each_region(r, t) {
> > +   rc = snprintf(&buf[written], len - written,
> > +   "%lu %lu %lu\n",
> > +   t->id, r->ar.start, r->ar.end);
> 
> This most likely will not work as intended, because snprintf() returns
> "[...] the number of characters which would be generated for the given
> input, excluding the trailing null [...]". Would scnprintf() -- which
> returns "[...] the number of characters written into @buf not including
> the trailing '\0' [...]" -- do what you intended?

Ah, you're right!  I will use 'scnprintf' instead, not only here but in other
relevant parts.


Thanks,
SeongJae Park

> 
> > +   if (!rc)
> > +   return -ENOMEM;
> > +   written += rc;
> > +   }
> > +   }
> > +   return written;
> > +}
> [...]


Re: [RFC v8 01/10] mm/damon/debugfs: Allow users to set initial monitoring target regions

2020-08-31 Thread Marco Elver
On Mon, Aug 31, 2020 at 12:47PM +0200, SeongJae Park wrote:
[...]
> diff --git a/mm/damon.c b/mm/damon.c
> index 7e3c8c82a010..9815d22fc4de 100644
> --- a/mm/damon.c
> +++ b/mm/damon.c
> @@ -2001,6 +2001,147 @@ static ssize_t debugfs_record_write(struct file *file,
>   return ret;
>  }
>  
> +static ssize_t sprint_init_regions(struct damon_ctx *c, char *buf, ssize_t 
> len)
> +{
> + struct damon_target *t;
> + struct damon_region *r;
> + int written = 0;
> + int rc;
> +
> + damon_for_each_target(t, c) {
> + damon_for_each_region(r, t) {
> + rc = snprintf(&buf[written], len - written,
> + "%lu %lu %lu\n",
> + t->id, r->ar.start, r->ar.end);

This most likely will not work as intended, because snprintf() returns
"[...] the number of characters which would be generated for the given
input, excluding the trailing null [...]". Would scnprintf() -- which
returns "[...] the number of characters written into @buf not including
the trailing '\0' [...]" -- do what you intended?

> + if (!rc)
> + return -ENOMEM;
> + written += rc;
> + }
> + }
> + return written;
> +}
[...]


[RFC v8 01/10] mm/damon/debugfs: Allow users to set initial monitoring target regions

2020-08-31 Thread SeongJae Park
From: SeongJae Park 

Some users would want to monitor only a part of the entire virtual
memory address space.  The '->init_target_regions' callback is therefore
provided, but only programming interface can use it.

For the reason, this commit introduces a new debugfs file,
'init_region'.  Users can specify which initial monitoring target
address regions they want by writing special input to the file.  The
input should describe each region in each line in below form:

  

This commit also makes the default '->init_target_regions' callback,
'kdamon_init_vm_regions()' to do nothing if the user has set the initial
target regions already.

Note that the regions will be updated to cover entire memory mapped
regions after 'regions update interval'.  If you want the regions to not
be updated after the initial setting, you could set the interval as a
very long time, say, a few decades.

Signed-off-by: SeongJae Park 
---
 mm/damon.c | 156 +++--
 1 file changed, 152 insertions(+), 4 deletions(-)

diff --git a/mm/damon.c b/mm/damon.c
index 7e3c8c82a010..9815d22fc4de 100644
--- a/mm/damon.c
+++ b/mm/damon.c
@@ -2001,6 +2001,147 @@ static ssize_t debugfs_record_write(struct file *file,
return ret;
 }
 
+static ssize_t sprint_init_regions(struct damon_ctx *c, char *buf, ssize_t len)
+{
+   struct damon_target *t;
+   struct damon_region *r;
+   int written = 0;
+   int rc;
+
+   damon_for_each_target(t, c) {
+   damon_for_each_region(r, t) {
+   rc = snprintf(&buf[written], len - written,
+   "%lu %lu %lu\n",
+   t->id, r->ar.start, r->ar.end);
+   if (!rc)
+   return -ENOMEM;
+   written += rc;
+   }
+   }
+   return written;
+}
+
+static ssize_t debugfs_init_regions_read(struct file *file, char __user *buf,
+   size_t count, loff_t *ppos)
+{
+   struct damon_ctx *ctx = &damon_user_ctx;
+   char *kbuf;
+   ssize_t len;
+
+   kbuf = kmalloc(count, GFP_KERNEL);
+   if (!kbuf)
+   return -ENOMEM;
+
+   mutex_lock(&ctx->kdamond_lock);
+   if (ctx->kdamond) {
+   mutex_unlock(&ctx->kdamond_lock);
+   return -EBUSY;
+   }
+
+   len = sprint_init_regions(ctx, kbuf, count);
+   mutex_unlock(&ctx->kdamond_lock);
+   if (len < 0)
+   goto out;
+   len = simple_read_from_buffer(buf, count, ppos, kbuf, len);
+
+out:
+   kfree(kbuf);
+   return len;
+}
+
+static int add_init_region(struct damon_ctx *c,
+unsigned long target_id, struct damon_addr_range *ar)
+{
+   struct damon_target *t;
+   struct damon_region *r, *prev;
+   int rc = -EINVAL;
+
+   if (ar->start >= ar->end)
+   return -EINVAL;
+
+   damon_for_each_target(t, c) {
+   if (t->id == target_id) {
+   r = damon_new_region(ar->start, ar->end);
+   if (!r)
+   return -ENOMEM;
+   damon_add_region(r, t);
+   if (nr_damon_regions(t) > 1) {
+   prev = damon_prev_region(r);
+   if (prev->ar.end > r->ar.start) {
+   damon_destroy_region(r);
+   return -EINVAL;
+   }
+   }
+   rc = 0;
+   }
+   }
+   return rc;
+}
+
+static int set_init_regions(struct damon_ctx *c, const char *str, ssize_t len)
+{
+   struct damon_target *t;
+   struct damon_region *r, *next;
+   int pos = 0, parsed, ret;
+   unsigned long target_id;
+   struct damon_addr_range ar;
+   int err;
+
+   damon_for_each_target(t, c) {
+   damon_for_each_region_safe(r, next, t)
+   damon_destroy_region(r);
+   }
+
+   while (pos < len) {
+   ret = sscanf(&str[pos], "%lu %lu %lu%n",
+   &target_id, &ar.start, &ar.end, &parsed);
+   if (ret != 3)
+   break;
+   err = add_init_region(c, target_id, &ar);
+   if (err)
+   goto fail;
+   pos += parsed;
+   }
+
+   return 0;
+
+fail:
+   damon_for_each_target(t, c) {
+   damon_for_each_region_safe(r, next, t)
+   damon_destroy_region(r);
+   }
+   return err;
+}
+
+static ssize_t debugfs_init_regions_write(struct file *file,
+ const char __user *buf, size_t count,
+ loff_t *ppos)
+{
+   struct damon_ctx *ctx = &damon_user_ctx;
+   char *kbuf;
+   ssize_t ret = count;
+   i