mm/damon: Implement a debugfs-based user space interface
Subsystems (2)
Quoted 68 lines · On Mon, 1 Feb 2021 09:37:39 -0800 Shakeel Butt <[email protected]> wrote:
Quoted 9 lines · SeongJae Park
Quoted 5 lines
From: SeongJae Park <[email protected]>
DAMON is designed to be used by kernel space code such as the memory management subsystems, and therefore it provides only kernel space API.
Which kernel space APIs are being referred here?
The symbols in '
include/linux/damon.h'Quoted 9 lines
Quoted 9 lines
That said, letting the user space control DAMON could provide some benefits to them. For example, it will allow user space to analyze their specific workloads and make their own special optimizations.
For such cases, this commit implements a simple DAMON application kernel module, namely 'damon-dbgfs', which merely wraps the DAMON api and exports those to the user space via the debugfs.
[...]
Quoted 27 lines
Quoted 27 lines
(quoted diff) +27
-0+static ssize_t dbgfs_target_ids_write(struct file *file, + const char __user *buf, size_t count, loff_t *ppos) +{ + struct damon_ctx *ctx = file->private_data; + char *kbuf, *nrs; + unsigned long *targets; + ssize_t nr_targets; + ssize_t ret = count; + int i; + int err; + + kbuf = user_input_str(buf, count, ppos); + if (IS_ERR(kbuf)) + return PTR_ERR(kbuf); + + nrs = kbuf; + + targets = str_to_target_ids(nrs, ret, &nr_targets); + if (!targets) { + ret = -ENOMEM; + goto out; + } + + if (targetid_is_pid(ctx)) { + for (i = 0; i < nr_targets; i++) + targets[i] = (unsigned long)find_get_pid( + (int)targets[i]);Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ } + + mutex_lock(&ctx->kdamond_lock); + if (ctx->kdamond) { + ret = -EINVAL; + goto unlock_out;You need to
put_pidon the targets array.Good catch!
Quoted 7 lines
Quoted 6 lines
(quoted diff) +5
-0+ } + + err = damon_set_targets(ctx, targets, nr_targets); + if (err) + ret = err;
Quoted 3 lines
Quoted 1 line
You need to handle the partial failure from
damon_set_targets().My intention is to keep partial success as is.
But, we should put_pid() partial failures... I will simply make this to completely fail with no registered target.
Thanks,
SeongJae Park
[...]
Quoted 69 lines · SeongJae Park
Quoted 69 lines · On Mon, 1 Feb 2021 09:37:39 -0800 Shakeel Butt <[email protected]> wrote:
Quoted 9 lines · SeongJae Park
Quoted 5 lines
From: SeongJae Park <[email protected]>
DAMON is designed to be used by kernel space code such as the memory management subsystems, and therefore it provides only kernel space API.
Which kernel space APIs are being referred here?
The symbols in '
include/linux/damon.h'Quoted 9 lines
Quoted 9 lines
That said, letting the user space control DAMON could provide some benefits to them. For example, it will allow user space to analyze their specific workloads and make their own special optimizations.
For such cases, this commit implements a simple DAMON application kernel module, namely 'damon-dbgfs', which merely wraps the DAMON api and exports those to the user space via the debugfs.
[...]
Quoted 27 lines
Quoted 27 lines
(quoted diff) +27
-0+static ssize_t dbgfs_target_ids_write(struct file *file, + const char __user *buf, size_t count, loff_t *ppos) +{ + struct damon_ctx *ctx = file->private_data; + char *kbuf, *nrs; + unsigned long *targets; + ssize_t nr_targets; + ssize_t ret = count; + int i; + int err; + + kbuf = user_input_str(buf, count, ppos); + if (IS_ERR(kbuf)) + return PTR_ERR(kbuf); + + nrs = kbuf; + + targets = str_to_target_ids(nrs, ret, &nr_targets); + if (!targets) { + ret = -ENOMEM; + goto out; + } + + if (targetid_is_pid(ctx)) { + for (i = 0; i < nr_targets; i++) + targets[i] = (unsigned long)find_get_pid( + (int)targets[i]);Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ } + + mutex_lock(&ctx->kdamond_lock); + if (ctx->kdamond) { + ret = -EINVAL; + goto unlock_out;You need to
put_pidon the targets array.Good catch!
Quoted 7 lines
Quoted 6 lines
(quoted diff) +5
-0+ } + + err = damon_set_targets(ctx, targets, nr_targets); + if (err) + ret = err;
Quoted 7 lines
Quoted 3 lines
Quoted 1 line
You need to handle the partial failure from
damon_set_targets().My intention is to keep partial success as is.
But, we should
put_pid()partial failures... I will simply make this to completely fail with no registered target.
You can simplify by simply restricting to one pid/target per each write syscall.
Quoted 72 lines · On Tue, 2 Feb 2021 07:07:24 -0800 Shakeel Butt <[email protected]> wrote:
Quoted 71 lines · SeongJae Park
Quoted 71 lines · On Mon, 1 Feb 2021 09:37:39 -0800 Shakeel Butt <[email protected]> wrote:
Quoted 9 lines · SeongJae Park
Quoted 5 lines
From: SeongJae Park <[email protected]>
DAMON is designed to be used by kernel space code such as the memory management subsystems, and therefore it provides only kernel space API.
Which kernel space APIs are being referred here?
The symbols in '
include/linux/damon.h'Quoted 9 lines
Quoted 9 lines
That said, letting the user space control DAMON could provide some benefits to them. For example, it will allow user space to analyze their specific workloads and make their own special optimizations.
For such cases, this commit implements a simple DAMON application kernel module, namely 'damon-dbgfs', which merely wraps the DAMON api and exports those to the user space via the debugfs.
[...]
Quoted 27 lines
Quoted 27 lines
(quoted diff) +27
-0+static ssize_t dbgfs_target_ids_write(struct file *file, + const char __user *buf, size_t count, loff_t *ppos) +{ + struct damon_ctx *ctx = file->private_data; + char *kbuf, *nrs; + unsigned long *targets; + ssize_t nr_targets; + ssize_t ret = count; + int i; + int err; + + kbuf = user_input_str(buf, count, ppos); + if (IS_ERR(kbuf)) + return PTR_ERR(kbuf); + + nrs = kbuf; + + targets = str_to_target_ids(nrs, ret, &nr_targets); + if (!targets) { + ret = -ENOMEM; + goto out; + } + + if (targetid_is_pid(ctx)) { + for (i = 0; i < nr_targets; i++) + targets[i] = (unsigned long)find_get_pid( + (int)targets[i]);Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +6
-0+ } + + mutex_lock(&ctx->kdamond_lock); + if (ctx->kdamond) { + ret = -EINVAL; + goto unlock_out;You need to
put_pidon the targets array.Good catch!
Quoted 8 lines
Review
Quoted 6 lines
(quoted diff) +5
-0+ } + + err = damon_set_targets(ctx, targets, nr_targets); + if (err) + ret = err;You need to handle the partial failure from
damon_set_targets().
Quoted 7 lines
Quoted 5 lines
Quoted 1 line
My intention is to keep partial success as is.
But, we should
put_pid()partial failures... I will simply make this to completely fail with no registered target.You can simplify by simply restricting to one pid/target per each write syscall.
Right, thanks for the suggestion. However, I already almost finished writing the fix. If there is no other concern, I'd like to keep current interface.
Thanks,
SeongJae Park
Quoted 1 line · SeongJae Park
[snip]
Quoted 7 lines
Quoted 2 lines
You can simplify by simply restricting to one pid/target per each write syscall.
Right, thanks for the suggestion. However, I already almost finished writing the fix. If there is no other concern, I'd like to keep current interface.
Please go ahead with the current.