| Message ID | 20211012205711.29216-2-sj@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | DAMON: Support Physical Memory Address Space Monitoring | expand |
On Tue, 12 Oct 2021 20:57:05 +0000 SeongJae Park <sj@kernel.org> wrote: > Some 'damon-dbgfs' users would want to monitor only a part of the entire > virtual memory address space. The program interface users in the kernel > space could use '->before_start()' callback or set the regions inside > the context struct as they want, but 'damon-dbgfs' users cannot. > > For the reason, this commit introduces a new debugfs file called > 'init_region'. 'damon-dbgfs' 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 the below form: > > <pid> <start address> <end address> > > Note that the regions will be updated to cover entire memory mapped > regions after a 'regions update interval' is passed. 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. > > ... > > +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; > + unsigned long id; > + int rc = -EINVAL; > + > + if (ar->start >= ar->end) > + return -EINVAL; > + > + damon_for_each_target(t, c) { > + id = t->id; > + if (targetid_is_pid(c)) > + id = (unsigned long)pid_vnr((struct pid *)id); This is a bit ugly. Did you consider making damon_target.id a union of all the possible types it can contain? This will avoid typecasts, has documentation value and reflacts what is actually going on. > + if (id == target_id) { > + r = damon_new_region(ar->start, ar->end); > + if (!r) > + return -ENOMEM; > + damon_add_region(r, t); > + if (damon_nr_regions(t) > 1) { > + prev = damon_prev_region(r); > + if (prev->ar.end > r->ar.start) { > + damon_destroy_region(r, t); > + return -EINVAL; > + } > + } > + rc = 0; > + } > + } > + return rc; > +} > +
On Wed, 13 Oct 2021 15:45:35 -0700 Andrew Morton <akpm@linux-foundation.org> wrote: > On Tue, 12 Oct 2021 20:57:05 +0000 SeongJae Park <sj@kernel.org> wrote: > > > Some 'damon-dbgfs' users would want to monitor only a part of the entire > > virtual memory address space. The program interface users in the kernel > > space could use '->before_start()' callback or set the regions inside > > the context struct as they want, but 'damon-dbgfs' users cannot. > > > > For the reason, this commit introduces a new debugfs file called > > 'init_region'. 'damon-dbgfs' 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 the below form: > > > > <pid> <start address> <end address> > > > > Note that the regions will be updated to cover entire memory mapped > > regions after a 'regions update interval' is passed. 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. > > > > ... > > > > +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; > > + unsigned long id; > > + int rc = -EINVAL; > > + > > + if (ar->start >= ar->end) > > + return -EINVAL; > > + > > + damon_for_each_target(t, c) { > > + id = t->id; > > + if (targetid_is_pid(c)) > > + id = (unsigned long)pid_vnr((struct pid *)id); > > This is a bit ugly. Did you consider making damon_target.id a union of > all the possible types it can contain? This will avoid typecasts, has > documentation value and reflacts what is actually going on. Agreed, thank you for this great comment! I will make it at least before adding another type of monitoring target. Thanks, SJ > > > + if (id == target_id) { > > + r = damon_new_region(ar->start, ar->end); > > + if (!r) > > + return -ENOMEM; > > + damon_add_region(r, t); > > + if (damon_nr_regions(t) > 1) { > > + prev = damon_prev_region(r); > > + if (prev->ar.end > r->ar.start) { > > + damon_destroy_region(r, t); > > + return -EINVAL; > > + } > > + } > > + rc = 0; > > + } > > + } > > + return rc; > > +} > > + > >
diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c index 28d6abf27763..1cce53cd241d 100644 --- a/mm/damon/dbgfs.c +++ b/mm/damon/dbgfs.c @@ -394,6 +394,152 @@ static ssize_t dbgfs_target_ids_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 = scnprintf(&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 dbgfs_init_regions_read(struct file *file, char __user *buf, + size_t count, loff_t *ppos) +{ + struct damon_ctx *ctx = file->private_data; + 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); + len = -EBUSY; + goto out; + } + + 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; + unsigned long id; + int rc = -EINVAL; + + if (ar->start >= ar->end) + return -EINVAL; + + damon_for_each_target(t, c) { + id = t->id; + if (targetid_is_pid(c)) + id = (unsigned long)pid_vnr((struct pid *)id); + if (id == target_id) { + r = damon_new_region(ar->start, ar->end); + if (!r) + return -ENOMEM; + damon_add_region(r, t); + if (damon_nr_regions(t) > 1) { + prev = damon_prev_region(r); + if (prev->ar.end > r->ar.start) { + damon_destroy_region(r, t); + 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, t); + } + + 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, t); + } + return err; +} + +static ssize_t dbgfs_init_regions_write(struct file *file, + const char __user *buf, size_t count, + loff_t *ppos) +{ + struct damon_ctx *ctx = file->private_data; + char *kbuf; + ssize_t ret = count; + int err; + + kbuf = user_input_str(buf, count, ppos); + if (IS_ERR(kbuf)) + return PTR_ERR(kbuf); + + mutex_lock(&ctx->kdamond_lock); + if (ctx->kdamond) { + ret = -EBUSY; + goto unlock_out; + } + + err = set_init_regions(ctx, kbuf, ret); + if (err) + ret = err; + +unlock_out: + mutex_unlock(&ctx->kdamond_lock); + kfree(kbuf); + return ret; +} + static ssize_t dbgfs_kdamond_pid_read(struct file *file, char __user *buf, size_t count, loff_t *ppos) { @@ -445,6 +591,12 @@ static const struct file_operations target_ids_fops = { .write = dbgfs_target_ids_write, }; +static const struct file_operations init_regions_fops = { + .open = damon_dbgfs_open, + .read = dbgfs_init_regions_read, + .write = dbgfs_init_regions_write, +}; + static const struct file_operations kdamond_pid_fops = { .open = damon_dbgfs_open, .read = dbgfs_kdamond_pid_read, @@ -453,9 +605,9 @@ static const struct file_operations kdamond_pid_fops = { static void dbgfs_fill_ctx_dir(struct dentry *dir, struct damon_ctx *ctx) { const char * const file_names[] = {"attrs", "schemes", "target_ids", - "kdamond_pid"}; + "init_regions", "kdamond_pid"}; const struct file_operations *fops[] = {&attrs_fops, &schemes_fops, - &target_ids_fops, &kdamond_pid_fops}; + &target_ids_fops, &init_regions_fops, &kdamond_pid_fops}; int i; for (i = 0; i < ARRAY_SIZE(file_names); i++)
Some 'damon-dbgfs' users would want to monitor only a part of the entire virtual memory address space. The program interface users in the kernel space could use '->before_start()' callback or set the regions inside the context struct as they want, but 'damon-dbgfs' users cannot. For the reason, this commit introduces a new debugfs file called 'init_region'. 'damon-dbgfs' 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 the below form: <pid> <start address> <end address> Note that the regions will be updated to cover entire memory mapped regions after a 'regions update interval' is passed. 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 <sj@kernel.org> --- mm/damon/dbgfs.c | 156 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 154 insertions(+), 2 deletions(-)