by SeongJae Park

[permalink] [raw]

Subject: Re: [RFC v9 4/8] mm/damon/schemes: Implement a debugfs interface

On Tue, 26 May 2020 09:56:58 +0200 SeongJae Park <[email protected]> wrote:

> From: SeongJae Park <[email protected]>
>
> This commit implements a debugfs interface for the data access
> monitoring oriented memory management schemes. It is supposed to be
> used by administrators and/or privileged user space programs. Users can
> read and update the rules using ``<debugfs>/damon/schemes`` file. The
> format is::
>
> <min/max size> <min/max access frequency> <min/max age> <action>
>
> Signed-off-by: SeongJae Park <[email protected]>
> ---
> mm/damon.c | 176 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 174 insertions(+), 2 deletions(-)
>
> diff --git a/mm/damon.c b/mm/damon.c
> index 6a07649d1f5d..bcc732e8735c 100644
> --- a/mm/damon.c
> +++ b/mm/damon.c
> @@ -173,6 +173,29 @@ static void damon_destroy_task(struct damon_task *t)
> damon_free_task(t);
> }
> +
> +/*
> + * Converts a string into an array of struct damos pointers
> + *
> + * Returns an array of struct damos pointers that converted if the conversion
> + * success, or NULL otherwise.
> + */
> +static struct damos **str_to_schemes(const char *str, ssize_t len,
> + ssize_t *nr_schemes)
> +{
> + struct damos *scheme, **schemes;
> + const int max_nr_schemes = 256;
> + int pos = 0, parsed, ret;
> + unsigned int min_sz, max_sz, min_nr_a, max_nr_a, min_age, max_age;
> + unsigned int action;
> +
> + schemes = kmalloc_array(max_nr_schemes, sizeof(scheme),
> + GFP_KERNEL);
> + if (!schemes)
> + return NULL;
> +
> + *nr_schemes = 0;
> + while (pos < len && *nr_schemes < max_nr_schemes) {
> + ret = sscanf(&str[pos], "%u %u %u %u %u %u %u%n",
> + &min_sz, &max_sz, &min_nr_a, &max_nr_a,
> + &min_age, &max_age, &action, &parsed);
> + if (ret != 7)
> + break;
> + if (action >= DAMOS_ACTION_LEN) {
> + pr_err("wrong action %d\n", action);
> + goto fail;
> + }
> +
> + pos += parsed;
> + scheme = damon_new_scheme(min_sz, max_sz, min_nr_a, max_nr_a,
> + min_age, max_age, action);
> + if (!scheme)
> + goto fail;
> +
> + schemes[*nr_schemes] = scheme;
> + *nr_schemes += 1;
> + }
> + if (!*nr_schemes)
> + goto fail;
> + return schemes;
> +fail:
> + free_schemes_arr(schemes, *nr_schemes);
> + return NULL;
> +}
> +
> +static ssize_t debugfs_schemes_write(struct file *file, const char __user *buf,
> + size_t count, loff_t *ppos)
> +{
> + struct damon_ctx *ctx = &damon_user_ctx;
> + char *kbuf;
> + struct damos **schemes;
> + ssize_t nr_schemes = 0, ret;
> + int err;
> +
> + if (*ppos)
> + return -EINVAL;
> +
> + kbuf = kmalloc(count, GFP_KERNEL);
> + if (!kbuf)
> + return -ENOMEM;
> +
> + ret = simple_write_to_buffer(kbuf, count, ppos, buf, count);
> + if (ret < 0)
> + goto out;
> +
> + schemes = str_to_schemes(kbuf, ret, &nr_schemes);

In case of wrong input, 'str_to_schemes()' could return NULL with non-zero
nr_schemes, but the case is not handled here. I will properly handle the case
in the next spin.

Thanks,
SeongJae Park

> +
> + mutex_lock(&ctx->kdamond_lock);
> + if (ctx->kdamond) {
> + ret = -EBUSY;
> + goto unlock_out;
> + }
> +
> + err = damon_set_schemes(ctx, schemes, nr_schemes);
> + if (err)
> + ret = err;
> + else
> + nr_schemes = 0;
> +unlock_out:
> + mutex_unlock(&ctx->kdamond_lock);
> + free_schemes_arr(schemes, nr_schemes);
> +out:
> + kfree(kbuf);
> + return ret;
> +}
> +