* Re: [PATCH v23 07/15] mm/damon: Implement a debugfs-based user space interface
@ 2021-02-02 10:29 ` SeongJae Park
0 siblings, 0 replies; 11+ messages in thread
From: SeongJae Park @ 2021-02-02 10:29 UTC (permalink / raw)
Cc: Shakeel Butt, Jonathan.Cameron, Andrea Arcangeli, acme,
alexander.shishkin, amit, benh, brendan.d.gregg, Brendan Higgins,
Qian Cai, Colin Ian King, Jonathan Corbet, David Hildenbrand,
dwmw, Marco Elver, Du, Fan, foersleo, Greg Thelen, Ian Rogers,
jolsa, Kirill A. Shutemov, Mark Rutland, Mel Gorman, Minchan Kim,
Ingo Molnar, namhyung, Peter Zijlstra (Intel),
Randy Dunlap, Rik van Riel, David Rientjes, Steven Rostedt,
Mike Rapoport, sblbir, Shuah Khan, sj38.park, snu,
Vlastimil Babka, Vladimir Davydov, Yang Shi, Huang Ying,
zgf574564920, linux-damon, Linux MM, linux-doc, LKML
> On Mon, 1 Feb 2021 09:37:39 -0800 Shakeel Butt <shakeelb@google.com> wrote:
>
> > On Tue, Dec 15, 2020 at 3:59 AM SeongJae Park <sjpark@amazon.com> wrote:
> > >
> > > From: SeongJae Park <sjpark@amazon.de>
> > >
> > > 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'
>
> >
> > > 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.
> > >
> [...]
> > > +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]);
> > > + }
> > > +
> > > + mutex_lock(&ctx->kdamond_lock);
> > > + if (ctx->kdamond) {
> > > + ret = -EINVAL;
> > > + goto unlock_out;
> >
> > You need to put_pid on the targets array.
>
> Good catch!
>
> >
> > > + }
> > > +
> > > + err = damon_set_targets(ctx, targets, nr_targets);
> > > + if (err)
> > > + ret = err;
> >
> > 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
[...]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v23 07/15] mm/damon: Implement a debugfs-based user space interface
@ 2021-02-02 10:29 ` SeongJae Park
0 siblings, 0 replies; 11+ messages in thread
From: SeongJae Park @ 2021-02-02 10:29 UTC (permalink / raw)
Cc: Shakeel Butt, Jonathan.Cameron, Andrea Arcangeli, acme,
alexander.shishkin, amit, benh, brendan.d.gregg, Brendan Higgins,
Qian Cai, Colin Ian King, Jonathan Corbet, David Hildenbrand,
dwmw, Marco Elver, Du, Fan, foersleo, Greg Thelen, Ian Rogers,
jolsa, Kirill A. Shutemov, Mark Rutland, Mel Gorman, Minchan Kim,
Ingo Molnar, namhyung, Peter Zijlstra (Intel),
Randy Dunlap, Rik van Riel, David Rientjes, Steven Rostedt,
Mike Rapoport, sblbir, Shuah Khan, sj38.park, snu,
Vlastimil Babka, Vladimir Davydov, Yang Shi, Huang Ying,
zgf574564920, linux-damon, Linux MM, linux-doc, LKML
> On Mon, 1 Feb 2021 09:37:39 -0800 Shakeel Butt <shakeelb@google.com> wrote:
>
> > On Tue, Dec 15, 2020 at 3:59 AM SeongJae Park <sjpark@amazon.com> wrote:
> > >
> > > From: SeongJae Park <sjpark@amazon.de>
> > >
> > > 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'
>
> >
> > > 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.
> > >
> [...]
> > > +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]);
> > > + }
> > > +
> > > + mutex_lock(&ctx->kdamond_lock);
> > > + if (ctx->kdamond) {
> > > + ret = -EINVAL;
> > > + goto unlock_out;
> >
> > You need to put_pid on the targets array.
>
> Good catch!
>
> >
> > > + }
> > > +
> > > + err = damon_set_targets(ctx, targets, nr_targets);
> > > + if (err)
> > > + ret = err;
> >
> > 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
[...]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v23 07/15] mm/damon: Implement a debugfs-based user space interface
2021-02-02 10:29 ` SeongJae Park
@ 2021-02-02 15:07 ` Shakeel Butt
-1 siblings, 0 replies; 11+ messages in thread
From: Shakeel Butt @ 2021-02-02 15:07 UTC (permalink / raw)
To: SeongJae Park
Cc: Jonathan.Cameron, Andrea Arcangeli, acme, alexander.shishkin,
amit, benh, brendan.d.gregg, Brendan Higgins, Qian Cai,
Colin Ian King, Jonathan Corbet, David Hildenbrand, dwmw,
Marco Elver, Du, Fan, foersleo, Greg Thelen, Ian Rogers, jolsa,
Kirill A. Shutemov, Mark Rutland, Mel Gorman, Minchan Kim,
Ingo Molnar, namhyung, Peter Zijlstra (Intel),
Randy Dunlap, Rik van Riel, David Rientjes, Steven Rostedt,
Mike Rapoport, sblbir, Shuah Khan, sj38.park, snu,
Vlastimil Babka, Vladimir Davydov, Yang Shi, Huang Ying,
zgf574564920, linux-damon, Linux MM, linux-doc, LKML
On Tue, Feb 2, 2021 at 2:30 AM SeongJae Park <sjpark@amazon.com> wrote:
>
> > On Mon, 1 Feb 2021 09:37:39 -0800 Shakeel Butt <shakeelb@google.com> wrote:
> >
> > > On Tue, Dec 15, 2020 at 3:59 AM SeongJae Park <sjpark@amazon.com> wrote:
> > > >
> > > > From: SeongJae Park <sjpark@amazon.de>
> > > >
> > > > 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'
> >
> > >
> > > > 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.
> > > >
> > [...]
> > > > +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]);
> > > > + }
> > > > +
> > > > + mutex_lock(&ctx->kdamond_lock);
> > > > + if (ctx->kdamond) {
> > > > + ret = -EINVAL;
> > > > + goto unlock_out;
> > >
> > > You need to put_pid on the targets array.
> >
> > Good catch!
> >
> > >
> > > > + }
> > > > +
> > > > + err = damon_set_targets(ctx, targets, nr_targets);
> > > > + if (err)
> > > > + ret = err;
> > >
> > > 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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v23 07/15] mm/damon: Implement a debugfs-based user space interface
@ 2021-02-02 15:07 ` Shakeel Butt
0 siblings, 0 replies; 11+ messages in thread
From: Shakeel Butt @ 2021-02-02 15:07 UTC (permalink / raw)
To: SeongJae Park
Cc: Jonathan.Cameron, Andrea Arcangeli, acme, alexander.shishkin,
amit, benh, brendan.d.gregg, Brendan Higgins, Qian Cai,
Colin Ian King, Jonathan Corbet, David Hildenbrand, dwmw,
Marco Elver, Du, Fan, foersleo, Greg Thelen, Ian Rogers, jolsa,
Kirill A. Shutemov, Mark Rutland, Mel Gorman, Minchan Kim,
Ingo Molnar, namhyung, Peter Zijlstra (Intel),
Randy Dunlap, Rik van Riel, David Rientjes, Steven Rostedt,
Mike Rapoport, sblbir, Shuah Khan, sj38.park, snu,
Vlastimil Babka, Vladimir Davydov, Yang Shi, Huang Ying,
zgf574564920, linux-damon, Linux MM, linux-doc, LKML
On Tue, Feb 2, 2021 at 2:30 AM SeongJae Park <sjpark@amazon.com> wrote:
>
> > On Mon, 1 Feb 2021 09:37:39 -0800 Shakeel Butt <shakeelb@google.com> wrote:
> >
> > > On Tue, Dec 15, 2020 at 3:59 AM SeongJae Park <sjpark@amazon.com> wrote:
> > > >
> > > > From: SeongJae Park <sjpark@amazon.de>
> > > >
> > > > 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'
> >
> > >
> > > > 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.
> > > >
> > [...]
> > > > +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]);
> > > > + }
> > > > +
> > > > + mutex_lock(&ctx->kdamond_lock);
> > > > + if (ctx->kdamond) {
> > > > + ret = -EINVAL;
> > > > + goto unlock_out;
> > >
> > > You need to put_pid on the targets array.
> >
> > Good catch!
> >
> > >
> > > > + }
> > > > +
> > > > + err = damon_set_targets(ctx, targets, nr_targets);
> > > > + if (err)
> > > > + ret = err;
> > >
> > > 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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH v23 07/15] mm/damon: Implement a debugfs-based user space interface
2021-02-02 15:07 ` Shakeel Butt
(?)
@ 2021-02-02 15:45 ` SeongJae Park
2021-02-02 16:08 ` Shakeel Butt
-1 siblings, 1 reply; 11+ messages in thread
From: SeongJae Park @ 2021-02-02 15:45 UTC (permalink / raw)
To: Shakeel Butt
Cc: SeongJae Park, Andrea Arcangeli, acme, alexander.shishkin, amit,
benh, brendan.d.gregg, Brendan Higgins, Qian Cai, Colin Ian King,
Jonathan Corbet, David Hildenbrand, dwmw, Marco Elver, Du, Fan,
foersleo, Greg Thelen, Ian Rogers, jolsa, Kirill A. Shutemov,
Mark Rutland, Mel Gorman, Minchan Kim, Ingo Molnar, namhyung,
Peter Zijlstra (Intel),
Randy Dunlap, Rik van Riel, David Rientjes, Steven Rostedt,
Mike Rapoport, sblbir, Shuah Khan, sj38.park, snu,
Vlastimil Babka, Vladimir Davydov, Yang Shi, Huang Ying,
zgf574564920, linux-damon, Linux MM, linux-doc, LKML
On Tue, 2 Feb 2021 07:07:24 -0800 Shakeel Butt <shakeelb@google.com> wrote:
> On Tue, Feb 2, 2021 at 2:30 AM SeongJae Park <sjpark@amazon.com> wrote:
> >
> >> On Mon, 1 Feb 2021 09:37:39 -0800 Shakeel Butt <shakeelb@google.com> wrote:
> >>
> >>> On Tue, Dec 15, 2020 at 3:59 AM SeongJae Park <sjpark@amazon.com> wrote:
> >>>>
> >>>> From: SeongJae Park <sjpark@amazon.de>
> >>>>
> >>>> 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'
> >>
> >>>
> >>>> 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.
> >>>>
> >> [...]
> >>>> +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]);
> >>>> + }
> >>>> +
> >>>> + mutex_lock(&ctx->kdamond_lock);
> >>>> + if (ctx->kdamond) {
> >>>> + ret = -EINVAL;
> >>>> + goto unlock_out;
> >>>
> >>> You need to put_pid on the targets array.
> >>
> >> Good catch!
> >>
> >>>
> >>>> + }
> >>>> +
> >>>> + err = damon_set_targets(ctx, targets, nr_targets);
> >>>> + if (err)
> >>>> + ret = err;
> >>>
> >>> 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.
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v23 07/15] mm/damon: Implement a debugfs-based user space interface
2021-02-02 15:45 ` SeongJae Park
@ 2021-02-02 16:08 ` Shakeel Butt
0 siblings, 0 replies; 11+ messages in thread
From: Shakeel Butt @ 2021-02-02 16:08 UTC (permalink / raw)
To: SeongJae Park
Cc: Andrea Arcangeli, acme, alexander.shishkin, amit, benh,
brendan.d.gregg, Brendan Higgins, Qian Cai, Colin Ian King,
Jonathan Corbet, David Hildenbrand, dwmw, Marco Elver, Du, Fan,
foersleo, Greg Thelen, Ian Rogers, jolsa, Kirill A. Shutemov,
Mark Rutland, Mel Gorman, Minchan Kim, Ingo Molnar, namhyung,
Peter Zijlstra (Intel),
Randy Dunlap, Rik van Riel, David Rientjes, Steven Rostedt,
Mike Rapoport, sblbir, Shuah Khan, sj38.park, snu,
Vlastimil Babka, Vladimir Davydov, Yang Shi, Huang Ying,
zgf574564920, linux-damon, Linux MM, linux-doc, LKML
On Tue, Feb 2, 2021 at 7:46 AM SeongJae Park <sjpark@amazon.com> wrote:
>
[snip]
> >
> > 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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v23 07/15] mm/damon: Implement a debugfs-based user space interface
@ 2021-02-02 16:08 ` Shakeel Butt
0 siblings, 0 replies; 11+ messages in thread
From: Shakeel Butt @ 2021-02-02 16:08 UTC (permalink / raw)
To: SeongJae Park
Cc: Andrea Arcangeli, acme, alexander.shishkin, amit, benh,
brendan.d.gregg, Brendan Higgins, Qian Cai, Colin Ian King,
Jonathan Corbet, David Hildenbrand, dwmw, Marco Elver, Du, Fan,
foersleo, Greg Thelen, Ian Rogers, jolsa, Kirill A. Shutemov,
Mark Rutland, Mel Gorman, Minchan Kim, Ingo Molnar, namhyung,
Peter Zijlstra (Intel),
Randy Dunlap, Rik van Riel, David Rientjes, Steven Rostedt,
Mike Rapoport, sblbir, Shuah Khan, sj38.park, snu,
Vlastimil Babka, Vladimir Davydov, Yang Shi, Huang Ying,
zgf574564920, linux-damon, Linux MM, linux-doc, LKML
On Tue, Feb 2, 2021 at 7:46 AM SeongJae Park <sjpark@amazon.com> wrote:
>
[snip]
> >
> > 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.
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v23 00/15] Introduce Data Access MONitor (DAMON)
@ 2020-12-15 11:54 SeongJae Park
2020-12-15 11:54 ` [PATCH v23 07/15] mm/damon: Implement a debugfs-based user space interface SeongJae Park
0 siblings, 1 reply; 11+ messages in thread
From: SeongJae Park @ 2020-12-15 11:54 UTC (permalink / raw)
To: akpm
Cc: SeongJae Park, Jonathan.Cameron, aarcange, acme,
alexander.shishkin, amit, benh, brendan.d.gregg, brendanhiggins,
cai, colin.king, corbet, david, dwmw, elver, fan.du, foersleo,
gthelen, irogers, jolsa, kirill, mark.rutland, mgorman, minchan,
mingo, namhyung, peterz, rdunlap, riel, rientjes, rostedt, rppt,
sblbir, shakeelb, shuah, sj38.park, snu, vbabka, vdavydov.dev,
yang.shi, ying.huang, zgf574564920, linux-damon, linux-mm,
linux-doc, linux-kernel
From: SeongJae Park <sjpark@amazon.de>
Changes from Previous Version (v22)
===================================
- Support arbitrary targets; now DAMON incurs only zero space overhead for page
granularity idleness monitoring
- Reorder patches for easier review (Shakeel Butt)
- Introduce arbitrary targets with sampling first, then the overhead-accuracy
control logic
- Introduce data structure manipulation functions when it really used.
- Call callbacks explicitly, without macro (Shakeel Butt)
- Rename DAMON_PRIMITIVES to DAMON_VADDR (Shakeel Butt)
- Remove 'page_idle_lock' patch (Shakeel Butt)
- Drop pidfd support in debugfs (Shakeel Butt)
Introduction
============
DAMON is a data access monitoring framework for the Linux kernel. The core
mechanisms of DAMON called 'region based sampling' and 'adaptive regions
adjustment' (refer to 'mechanisms.rst' in the 11th patch of this patchset for
the detail) make it
- accurate (The monitored information is useful for DRAM level memory
management. It might not appropriate for Cache-level accuracy, though.),
- light-weight (The monitoring overhead is low enough to be applied online
while making no impact on the performance of the target workloads.), and
- scalable (the upper-bound of the instrumentation overhead is controllable
regardless of the size of target workloads.).
Using this framework, therefore, several memory management mechanisms such as
reclamation and THP can be optimized to aware real data access patterns.
Experimental access pattern aware memory management optimization works that
incurring high instrumentation overhead will be able to have another try.
Though DAMON is for kernel subsystems, it can be easily exposed to the user
space by writing a DAMON-wrapper kernel subsystem. Then, user space users who
have some special workloads will be able to write personalized tools or
applications for deeper understanding and specialized optimizations of their
systems.
Long-term Plan
--------------
DAMON is a part of a project called Data Access-aware Operating System (DAOS).
As the name implies, I want to improve the performance and efficiency of
systems using fine-grained data access patterns. The optimizations are for
both kernel and user spaces. I will therefore modify or create kernel
subsystems, export some of those to user space and implement user space library
/ tools. Below shows the layers and components for the project.
---------------------------------------------------------------------------
Primitives: PTE Accessed bit, PG_idle, rmap, (Intel CMT), ...
Framework: DAMON
Features: DAMOS, virtual addr, physical addr, ...
Applications: DAMON-debugfs, (DARC), ...
^^^^^^^^^^^^^^^^^^^^^^^ KERNEL SPACE ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Raw Interface: debugfs, (sysfs), (damonfs), tracepoints, (sys_damon), ...
vvvvvvvvvvvvvvvvvvvvvvv USER SPACE vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
Library: (libdamon), ...
Tools: DAMO, (perf), ...
---------------------------------------------------------------------------
The components in parentheses or marked as '...' are not implemented yet but in
the future plan. IOW, those are the TODO tasks of DAOS project. For more
detail, please refer to the plans:
https://lore.kernel.org/linux-mm/20201202082731.24828-1-sjpark@amazon.com/
Evaluations
===========
We evaluated DAMON's overhead, monitoring quality and usefulness using 24
realistic workloads on my QEMU/KVM based virtual machine running a kernel that
v23 DAMON patchset is applied.
DAMON is lightweight. It increases system memory usage by 0.42% and slows
target workloads down by 0.39%.
DAMON is accurate and useful for memory management optimizations. An
experimental DAMON-based operation scheme for THP, 'ethp', removes 81.45% of
THP memory overheads while preserving 50.09% of THP speedup. Another
experimental DAMON-based 'proactive reclamation' implementation, 'prcl',
reduces 91.45% of residential sets and 22.91% of system memory footprint while
incurring only 2.43% runtime overhead in the best case (parsec3/freqmine).
NOTE that the experimental THP optimization and proactive reclamation are not
for production but only for proof of concepts.
Please refer to the official document[1] or "Documentation/admin-guide/mm: Add
a document for DAMON" patch in this patchset for detailed evaluation setup and
results.
[1] https://damonitor.github.io/doc/html/latest-damon/admin-guide/mm/damon/eval.html
Real-world User Story
=====================
In summary, DAMON has used on production systems and proved its usefulness.
DAMON as a profiler
-------------------
We analyzed characteristics of a large scale production systems of our
customers using DAMON. The systems utilize 70GB DRAM and 36 CPUs. From this,
we were able to find interesting things below.
There were obviously different access pattern under idle workload and active
workload. Under the idle workload, it accessed large memory regions with low
frequency, while the active workload accessed small memory regions with high
freuqnecy.
DAMON found a 7GB memory region that showing obviously high access frequency
under the active workload. We believe this is the performance-effective
working set and need to be protected.
There was a 4KB memory region that showing highest access frequency under not
only active but also idle workloads. We think this must be a hottest code
section like thing that should never be paged out.
For this analysis, DAMON used only 0.3-1% of single CPU time. Because we used
recording-based analysis, it consumed about 3-12 MB of disk space per 20
minutes. This is only small amount of disk space, but we can further reduce
the disk usage by using non-recording-based DAMON features. I'd like to argue
that only DAMON can do such detailed analysis (finding 4KB highest region in
70GB memory) with the light overhead.
DAMON as a system optimization tool
-----------------------------------
We also found below potential performance problems on the systems and made
DAMON-based solutions.
The system doesn't want to make the workload suffer from the page reclamation
and thus it utilizes enough DRAM but no swap device. However, we found the
system is actively reclaiming file-backed pages, because the system has
intensive file IO. The file IO turned out to be not performance critical for
the workload, but the customer wanted to ensure performance critical
file-backed pages like code section to not mistakenly be evicted.
Using direct IO should or `mlock()` would be a straightforward solution, but
modifying the user space code is not easy for the customer. Alternatively, we
could use DAMON-based operation scheme[1]. By using it, we can ask DAMON to
track access frequency of each region and make
'process_madvise(MADV_WILLNEED)[2]' call for regions having specific size and
access frequency for a time interval.
We also found the system is having high number of TLB misses. We tried
'always' THP enabled policy and it greatly reduced TLB misses, but the page
reclamation also been more frequent due to the THP internal fragmentation
caused memory bloat. We could try another DAMON-based operation scheme that
applies 'MADV_HUGEPAGE' to memory regions having >=2MB size and high access
frequency, while applying 'MADV_NOHUGEPAGE' to regions having <2MB size and low
access frequency.
We do not own the systems so we only reported the analysis results and possible
optimization solutions to the customers. The customers satisfied about the
analysis results and promised to try the optimization guides.
[1] https://lore.kernel.org/linux-mm/20201006123931.5847-1-sjpark@amazon.com/
[2] https://lore.kernel.org/linux-api/20200622192900.22757-4-minchan@kernel.org/
Comparison with Idle Page Tracking
==================================
Idle Page Tracking allows users to set and read idleness of pages using a
bitmap file which represents each page with each bit of the file. One
recommended usage of it is working set size detection. Users can do that by
1. find PFN of each page for workloads in interest,
2. set all the pages as idle by doing writes to the bitmap file,
3. wait until the workload accesses its working set, and
4. read the idleness of the pages again and count pages became not idle.
NOTE: While Idle Page Tracking is for user space users, DAMON is primarily
designed for kernel subsystems though it can easily exposed to the user space.
Hence, this section only assumes such user space use of DAMON.
For what use cases Idle Page Tracking would be better?
------------------------------------------------------
1. Flexible usecases other than hotness monitoring.
Because Idle Page Tracking allows users to control the primitive (Page
idleness) by themselves, Idle Page Tracking users can do anything they want.
Meanwhile, DAMON is primarily designed to monitor the hotness of each memory
region. For this, DAMON asks users to provide sampling interval and
aggregation interval. For the reason, there could be some use case that using
Idle Page Tracking is simpler.
2. Physical memory monitoring.
Idle Page Tracking receives PFN range as input, so natively supports physical
memory monitoring.
DAMON is designed to be extensible for multiple address spaces and use cases by
implementing and using primitives for the given use case. Therefore, by
theory, DAMON has no limitation in the type of target address space as long as
primitives for the given address space exists. However, the default primitives
introduced by this patchset supports only virtual address spaces.
Therefore, for physical memory monitoring, you should implement your own
primitives and use it, or simply use Idle Page Tracking.
Nonetheless, RFC patchsets[1] for the physical memory address space primitives
is already available. It also supports user memory same to Idle Page Tracking.
[1] https://lore.kernel.org/linux-mm/20200831104730.28970-1-sjpark@amazon.com/
For what use cases DAMON is better?
-----------------------------------
1. Hotness Monitoring.
Idle Page Tracking let users know only if a page frame is accessed or not. For
hotness check, the user should write more code and use more memory. DAMON do
that by itself.
2. Low Monitoring Overhead
DAMON receives user's monitoring request with one step and then provide the
results. So, roughly speaking, DAMON require only O(1) user/kernel context
switches.
In case of Idle Page Tracking, however, because the interface receives
contiguous page frames, the number of user/kernel context switches increases as
the monitoring target becomes complex and huge. As a result, the context
switch overhead could be not negligible.
Moreover, DAMON is born to handle with the monitoring overhead. Because the
core mechanism is pure logical, Idle Page Tracking users might be able to
implement the mechanism on thier own, but it would be time consuming and the
user/kernel context switching will still more frequent than that of DAMON.
Also, the kernel subsystems cannot use the logic in this case.
3. Page granularity working set size detection.
Until v22 of this patchset, this was categorized as the thing Idle Page
Tracking could do better, because DAMON basically maintains additional metadata
for each of the monitoring target regions. So, in the page granularity working
set size detection use case, DAMON would incur (number of monitoring target
pages * size of metadata) memory overhead. Size of the single metadata item is
about 54 bytes, so assuming 4KB pages, about 1.3% of monitoring target pages
will be additionally used.
All essential metadata for Idle Page Tracking are embedded in 'struct page' and
page table entries. Therefore, in this use case, only one counter variable for
working set size accounting is required if Idle Page Tracking is used.
There are more details to consider, but roughly speaking, this is true in most
cases.
However, the situation changed from v23. Now DAMON supports arbitrary types of
monitoring targets, which don't use the metadata. Using that, DAMON can do the
working set size detection with no additional space overhead but less
user-kernel context switch. A first draft for the implementation of monitoring
primitives for this usage is available in a DAMON development tree[1]. An RFC
patchset for it based on this patchset will also be available soon.
[1] https://github.com/sjp38/linux/tree/damon/pgidle_hack
4. More future usecases
While Idle Page Tracking has tight coupling with base primitives (PG_Idle and
page table Accessed bits), DAMON is designed to be extensible for many use
cases and address spaces. If you need some special address type or want to use
special h/w access check primitives, you can write your own primitives for that
and configure DAMON to use those. Therefore, if your use case could be changed
a lot in future, using DAMON could be better.
Can I use both Idle Page Tracking and DAMON?
--------------------------------------------
Yes, though using them concurrently for overlapping memory regions could result
in interference to each other. Nevertheless, such use case would be rare or
makes no sense at all. Even in the case, the noise would bot be really
significant. So, you can choose whatever you want depending on the
characteristics of your use cases.
More Information
================
We prepared a showcase web site[1] that you can get more information. There
are
- the official documentations[2],
- the heatmap format dynamic access pattern of various realistic workloads for
heap area[3], mmap()-ed area[4], and stack[5] area,
- the dynamic working set size distribution[6] and chronological working set
size changes[7], and
- the latest performance test results[8].
[1] https://damonitor.github.io/_index
[2] https://damonitor.github.io/doc/html/latest-damon
[3] https://damonitor.github.io/test/result/visual/latest/rec.heatmap.0.png.html
[4] https://damonitor.github.io/test/result/visual/latest/rec.heatmap.1.png.html
[5] https://damonitor.github.io/test/result/visual/latest/rec.heatmap.2.png.html
[6] https://damonitor.github.io/test/result/visual/latest/rec.wss_sz.png.html
[7] https://damonitor.github.io/test/result/visual/latest/rec.wss_time.png.html
[8] https://damonitor.github.io/test/result/perf/latest/html/index.html
Baseline and Complete Git Trees
===============================
The patches are based on the v5.10. You can also clone the complete git
tree:
$ git clone git://github.com/sjp38/linux -b damon/patches/v23
The web is also available:
https://github.com/sjp38/linux/releases/tag/damon/patches/v23
There are a couple of trees for entire DAMON patchset series. It includes
future features. The first one[1] contains the changes for latest release,
while the other one[2] contains the changes for next release.
[1] https://github.com/sjp38/linux/tree/damon/master
[2] https://github.com/sjp38/linux/tree/damon/next
Sequence Of Patches
===================
First three patches implement the core logics of DAMON. The 1st patch
introduces basic sampling based hotness monitoring for arbitrary types of
targets. Following two patches implement the core mechanisms for control of
overhead and accuracy, namely regions based sampling (patch 2) and adaptive
regions adjustment (patch 3).
Now the essential parts of DAMON is complete, but it cannot work unless someone
provides monitoring primitives for a specific use case. The following two
patches make it just work for virtual address spaces monitoring. The 4th patch
makes 'PG_idle' can be used by DAMON and the 5th patch implements the virtual
memory address space specific monitoring primitives using page table Accessed
bits and the 'PG_idle' page flag.
Now DAMON just works for virtual address space monitoring via the kernel space
api. To let the user space users can use DAMON, following six patches add
interfaces for them. The 6th patch adds a tracepoint for monitoring results.
The 7th patch implements a DAMON application kernel module, namely damon-dbgfs,
that simply wraps DAMON and exposes DAMON interface to the user space via the
debugfs interface. To let the user space get the monitoring results more
easily, the 8th patch implements a simple recording feature in 'damon-dbgfs'.
The 9th patch further exports pid of monitoring thread (kdamond) to user space
for easier cpu usage accounting, and the 10th patch makes the debugfs interface
to support multiple contexts. Then, the 11th patch implements a user space
tool to provide a minimal reference to the debugfs interface and for high level
use/tests of the DAMON.
Three patches for maintainability follows. The 12th patch adds documentations
for both the user space and the kernel space. The 13th patch provides unit
tests (based on the kunit) while the 14th patch adds user space tests (based on
the kselftest).
Finally, the last patch (15th) updates the MAINTAINERS file.
Patch History
=============
Changes from v22
(https://lore.kernel.org/linux-mm/20201020085940.13875-1-sjpark@amazon.com/)
- Support arbitrary targets; now DAMON incurs only zero space overhead for page
granularity idleness monitoring
- Reorder patches for easier review (Shakeel Butt)
- Introduce arbitrary targets with sampling first, then the overhead-accuracy
control logic
- Introduce data structure manipulation functions when it really used.
- Call callbacks explicitly, without macro (Shakeel Butt)
- Rename DAMON_PRIMITIVES to DAMON_VADDR (Shakeel Butt)
- Remove 'page_idle_lock' patch (Shakeel Butt)
- Drop pidfd support in debugfs (Shakeel Butt)
Changes from v21
(https://lore.kernel.org/linux-doc/20201005105522.23841-1-sjpark@amazon.com/)
- Fix build warnings and errors (kernel test robot)
- Fix a memory leak (kmemleak)
- Respect KUNIT_ALL_TESTS
- Rebase on v5.9
- Update the evaluation results
Changes from v20
(https://lore.kernel.org/linux-mm/20200817105137.19296-1-sjpark@amazon.com/)
- s/snprintf()/scnprintf() (Marco Elver)
- Support multiple contexts for user space users (Shakeel Butt)
- Export pid of monitoring thread to user space (Shakeel Butt)
- Let coexistable with Idle Page Tracking
- Place three parts of DAMON (core, primitives, and dbgfs) in different files
Changes from v19
(https://lore.kernel.org/linux-mm/20200804091416.31039-1-sjpark@amazon.com/)
- Place 'CREATE_TRACE_POINTS' after '#include' statements (Steven Rostedt)
- Support large record file (Alkaid)
- Place 'put_pid()' of virtual monitoring targets in 'cleanup' callback
- Avoid conflict between concurrent DAMON users
- Update evaluation result document
Changes from v18
(https://lore.kernel.org/linux-mm/20200713084144.4430-1-sjpark@amazon.com/)
- Drop loadable module support (Mike Rapoport)
- Select PAGE_EXTENSION if !64BIT for 'set_page_young()'
- Take care of the MMU notification subscribers (Shakeel Butt)
- Substitute 'struct damon_task' with 'struct damon_target' for better abstract
- Use 'struct pid' instead of 'pid_t' as the target (Shakeel Butt)
- Support pidfd from the debugfs interface (Shakeel Butt)
- Fix typos (Greg Thelen)
- Properly isolate DAMON from other pmd/pte Accessed bit users (Greg Thelen)
- Rebase on v5.8
Changes from v17
(https://lore.kernel.org/linux-mm/20200706115322.29598-1-sjpark@amazon.com/)
- Reorganize the doc and remove png blobs (Mike Rapoport)
- Wordsmith mechnisms doc and commit messages
- tools/wss: Set default working set access frequency threshold
- Avoid race in damon deamon start
Please refer to the v17 patchset to get older history.
SeongJae Park (15):
mm: Introduce Data Access MONitor (DAMON)
mm/damon/core: Implement region-based sampling
mm/damon: Adaptively adjust regions
mm/idle_page_tracking: Make PG_idle reusable
mm/damon: Implement primitives for the virtual memory address spaces
mm/damon: Add a tracepoint
mm/damon: Implement a debugfs-based user space interface
mm/damon/dbgfs: Implement recording feature
mm/damon/dbgfs: Export kdamond pid to the user space
mm/damon/dbgfs: Support multiple contexts
tools: Introduce a minimal user-space tool for DAMON
Documentation: Add documents for DAMON
mm/damon: Add kunit tests
mm/damon: Add user space selftests
MAINTAINERS: Update for DAMON
Documentation/admin-guide/mm/damon/guide.rst | 157 ++++
Documentation/admin-guide/mm/damon/index.rst | 15 +
Documentation/admin-guide/mm/damon/plans.rst | 29 +
Documentation/admin-guide/mm/damon/start.rst | 96 ++
Documentation/admin-guide/mm/damon/usage.rst | 302 ++++++
Documentation/admin-guide/mm/index.rst | 1 +
Documentation/vm/damon/api.rst | 20 +
Documentation/vm/damon/design.rst | 166 ++++
Documentation/vm/damon/eval.rst | 227 +++++
Documentation/vm/damon/faq.rst | 58 ++
Documentation/vm/damon/index.rst | 31 +
Documentation/vm/index.rst | 1 +
MAINTAINERS | 12 +
include/linux/damon.h | 293 ++++++
include/linux/page-flags.h | 4 +-
include/linux/page_ext.h | 2 +-
include/linux/page_idle.h | 6 +-
include/trace/events/damon.h | 43 +
include/trace/events/mmflags.h | 2 +-
mm/Kconfig | 10 +
mm/Makefile | 1 +
mm/damon/Kconfig | 69 ++
mm/damon/Makefile | 5 +
mm/damon/core-test.h | 253 +++++
mm/damon/core.c | 717 +++++++++++++++
mm/damon/dbgfs-test.h | 209 +++++
mm/damon/dbgfs.c | 867 ++++++++++++++++++
mm/damon/vaddr-test.h | 328 +++++++
mm/damon/vaddr.c | 586 ++++++++++++
mm/page_ext.c | 12 +-
mm/page_idle.c | 10 -
tools/damon/.gitignore | 1 +
tools/damon/_damon.py | 130 +++
tools/damon/_dist.py | 35 +
tools/damon/_recfile.py | 23 +
tools/damon/bin2txt.py | 67 ++
tools/damon/damo | 37 +
tools/damon/heats.py | 362 ++++++++
tools/damon/nr_regions.py | 91 ++
tools/damon/record.py | 135 +++
tools/damon/report.py | 45 +
tools/damon/wss.py | 100 ++
tools/testing/selftests/damon/Makefile | 7 +
.../selftests/damon/_chk_dependency.sh | 28 +
tools/testing/selftests/damon/_chk_record.py | 109 +++
.../testing/selftests/damon/debugfs_attrs.sh | 161 ++++
.../testing/selftests/damon/debugfs_record.sh | 50 +
47 files changed, 5895 insertions(+), 18 deletions(-)
create mode 100644 Documentation/admin-guide/mm/damon/guide.rst
create mode 100644 Documentation/admin-guide/mm/damon/index.rst
create mode 100644 Documentation/admin-guide/mm/damon/plans.rst
create mode 100644 Documentation/admin-guide/mm/damon/start.rst
create mode 100644 Documentation/admin-guide/mm/damon/usage.rst
create mode 100644 Documentation/vm/damon/api.rst
create mode 100644 Documentation/vm/damon/design.rst
create mode 100644 Documentation/vm/damon/eval.rst
create mode 100644 Documentation/vm/damon/faq.rst
create mode 100644 Documentation/vm/damon/index.rst
create mode 100644 include/linux/damon.h
create mode 100644 include/trace/events/damon.h
create mode 100644 mm/damon/Kconfig
create mode 100644 mm/damon/Makefile
create mode 100644 mm/damon/core-test.h
create mode 100644 mm/damon/core.c
create mode 100644 mm/damon/dbgfs-test.h
create mode 100644 mm/damon/dbgfs.c
create mode 100644 mm/damon/vaddr-test.h
create mode 100644 mm/damon/vaddr.c
create mode 100644 tools/damon/.gitignore
create mode 100644 tools/damon/_damon.py
create mode 100644 tools/damon/_dist.py
create mode 100644 tools/damon/_recfile.py
create mode 100644 tools/damon/bin2txt.py
create mode 100755 tools/damon/damo
create mode 100644 tools/damon/heats.py
create mode 100644 tools/damon/nr_regions.py
create mode 100644 tools/damon/record.py
create mode 100644 tools/damon/report.py
create mode 100644 tools/damon/wss.py
create mode 100644 tools/testing/selftests/damon/Makefile
create mode 100644 tools/testing/selftests/damon/_chk_dependency.sh
create mode 100644 tools/testing/selftests/damon/_chk_record.py
create mode 100755 tools/testing/selftests/damon/debugfs_attrs.sh
create mode 100755 tools/testing/selftests/damon/debugfs_record.sh
--
2.17.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH v23 07/15] mm/damon: Implement a debugfs-based user space interface
2020-12-15 11:54 [PATCH v23 00/15] Introduce Data Access MONitor (DAMON) SeongJae Park
@ 2020-12-15 11:54 ` SeongJae Park
2021-02-01 17:37 ` Shakeel Butt
0 siblings, 1 reply; 11+ messages in thread
From: SeongJae Park @ 2020-12-15 11:54 UTC (permalink / raw)
To: akpm
Cc: SeongJae Park, Jonathan.Cameron, aarcange, acme,
alexander.shishkin, amit, benh, brendan.d.gregg, brendanhiggins,
cai, colin.king, corbet, david, dwmw, elver, fan.du, foersleo,
gthelen, irogers, jolsa, kirill, mark.rutland, mgorman, minchan,
mingo, namhyung, peterz, rdunlap, riel, rientjes, rostedt, rppt,
sblbir, shakeelb, shuah, sj38.park, snu, vbabka, vdavydov.dev,
yang.shi, ying.huang, zgf574564920, linux-damon, linux-mm,
linux-doc, linux-kernel
From: SeongJae Park <sjpark@amazon.de>
DAMON is designed to be used by kernel space code such as the memory
management subsystems, and therefore it provides only kernel space API.
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.
'damon-dbgfs' exports three files, ``attrs``, ``target_ids``, and
``monitor_on`` under its debugfs directory, ``<debugfs>/damon/``.
Attributes
----------
Users can read and write the ``sampling interval``, ``aggregation
interval``, ``regions update interval``, and min/max number of
monitoring target regions by reading from and writing to the ``attrs``
file. For example, below commands set those values to 5 ms, 100 ms,
1,000 ms, 10, 1000 and check it again::
# cd <debugfs>/damon
# echo 5000 100000 1000000 10 1000 > attrs
# cat attrs
5000 100000 1000000 10 1000
Target IDs
----------
Some types of address spaces supports multiple monitoring target. For
example, the virtual memory address spaces monitoring can have multiple
processes as the monitoring targets. Users can set the targets by
writing relevant id values of the targets to, and get the ids of the
current targets by reading from the ``target_ids`` file. In case of the
virtual address spaces monitoring, the values should be pids of the
monitoring target processes. For example, below commands set processes
having pids 42 and 4242 as the monitoring targets and check it again::
# cd <debugfs>/damon
# echo 42 4242 > target_ids
# cat target_ids
42 4242
Note that setting the target ids doesn't start the monitoring.
Turning On/Off
--------------
Setting the files as described above doesn't incur effect unless you
explicitly start the monitoring. You can start, stop, and check the
current status of the monitoring by writing to and reading from the
``monitor_on`` file. Writing ``on`` to the file starts the monitoring
of the targets with the attributes. Writing ``off`` to the file stops
those. DAMON also stops if every targets are invalidated (in case of
the virtual memory monitoring, target processes are invalidated when
terminated). Below example commands turn on, off, and check the status
of DAMON::
# cd <debugfs>/damon
# echo on > monitor_on
# echo off > monitor_on
# cat monitor_on
off
Please note that you cannot write to the above-mentioned debugfs files
while the monitoring is turned on. If you write to the files while
DAMON is running, an error code such as ``-EBUSY`` will be returned.
Signed-off-by: SeongJae Park <sjpark@amazon.de>
Reviewed-by: Leonard Foerster <foersleo@amazon.de>
---
include/linux/damon.h | 3 +
mm/damon/Kconfig | 9 ++
mm/damon/Makefile | 1 +
mm/damon/core.c | 45 ++++++
mm/damon/dbgfs.c | 366 ++++++++++++++++++++++++++++++++++++++++++
5 files changed, 424 insertions(+)
create mode 100644 mm/damon/dbgfs.c
diff --git a/include/linux/damon.h b/include/linux/damon.h
index 39b4d6d3ddee..f9e0d4349352 100644
--- a/include/linux/damon.h
+++ b/include/linux/damon.h
@@ -265,9 +265,12 @@ unsigned int damon_nr_regions(struct damon_target *t);
struct damon_ctx *damon_new_ctx(enum damon_target_type type);
void damon_destroy_ctx(struct damon_ctx *ctx);
+int damon_set_targets(struct damon_ctx *ctx,
+ unsigned long *ids, ssize_t nr_ids);
int damon_set_attrs(struct damon_ctx *ctx, unsigned long sample_int,
unsigned long aggr_int, unsigned long regions_update_int,
unsigned long min_nr_reg, unsigned long max_nr_reg);
+int damon_nr_running_ctxs(void);
int damon_start(struct damon_ctx **ctxs, int nr_ctxs);
int damon_stop(struct damon_ctx **ctxs, int nr_ctxs);
diff --git a/mm/damon/Kconfig b/mm/damon/Kconfig
index 8ae080c52950..72f1683ba0ee 100644
--- a/mm/damon/Kconfig
+++ b/mm/damon/Kconfig
@@ -21,4 +21,13 @@ config DAMON_VADDR
This builds the default data access monitoring primitives for DAMON
that works for virtual address spaces.
+config DAMON_DBGFS
+ bool "DAMON debugfs interface"
+ depends on DAMON_VADDR && DEBUG_FS
+ help
+ This builds the debugfs interface for DAMON. The user space admins
+ can use the interface for arbitrary data access monitoring.
+
+ If unsure, say N.
+
endmenu
diff --git a/mm/damon/Makefile b/mm/damon/Makefile
index 6ebbd08aed67..fed4be3bace3 100644
--- a/mm/damon/Makefile
+++ b/mm/damon/Makefile
@@ -2,3 +2,4 @@
obj-$(CONFIG_DAMON) := core.o
obj-$(CONFIG_DAMON_VADDR) += vaddr.o
+obj-$(CONFIG_DAMON_DBGFS) += dbgfs.o
diff --git a/mm/damon/core.c b/mm/damon/core.c
index 5ca9f79ccbb6..b9575a6bebff 100644
--- a/mm/damon/core.c
+++ b/mm/damon/core.c
@@ -166,6 +166,37 @@ void damon_destroy_ctx(struct damon_ctx *ctx)
kfree(ctx);
}
+/**
+ * damon_set_targets() - Set monitoring targets.
+ * @ctx: monitoring context
+ * @ids: array of target ids
+ * @nr_ids: number of entries in @ids
+ *
+ * This function should not be called while the kdamond is running.
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+int damon_set_targets(struct damon_ctx *ctx,
+ unsigned long *ids, ssize_t nr_ids)
+{
+ ssize_t i;
+ struct damon_target *t, *next;
+
+ damon_for_each_target_safe(t, next, ctx)
+ damon_destroy_target(t);
+
+ for (i = 0; i < nr_ids; i++) {
+ t = damon_new_target(ids[i]);
+ if (!t) {
+ pr_err("Failed to alloc damon_target\n");
+ return -ENOMEM;
+ }
+ damon_add_target(ctx, t);
+ }
+
+ return 0;
+}
+
/**
* damon_set_attrs() - Set attributes for the monitoring.
* @ctx: monitoring context
@@ -206,6 +237,20 @@ int damon_set_attrs(struct damon_ctx *ctx, unsigned long sample_int,
return 0;
}
+/**
+ * damon_nr_running_ctxs() - Return number of currently running contexts.
+ */
+int damon_nr_running_ctxs(void)
+{
+ int nr_ctxs;
+
+ mutex_lock(&damon_lock);
+ nr_ctxs = nr_running_ctxs;
+ mutex_unlock(&damon_lock);
+
+ return nr_ctxs;
+}
+
/* Returns the size upper limit for each monitoring region */
static unsigned long damon_region_sz_limit(struct damon_ctx *ctx)
{
diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
new file mode 100644
index 000000000000..fd1665a183c2
--- /dev/null
+++ b/mm/damon/dbgfs.c
@@ -0,0 +1,366 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * DAMON Debugfs Interface
+ *
+ * Author: SeongJae Park <sjpark@amazon.de>
+ */
+
+#define pr_fmt(fmt) "damon-dbgfs: " fmt
+
+#include <linux/damon.h>
+#include <linux/debugfs.h>
+#include <linux/file.h>
+#include <linux/mm.h>
+#include <linux/module.h>
+#include <linux/page_idle.h>
+#include <linux/slab.h>
+
+static struct damon_ctx **dbgfs_ctxs;
+static int dbgfs_nr_ctxs;
+static struct dentry **dbgfs_dirs;
+
+/*
+ * Returns non-empty string on success, negarive error code otherwise.
+ */
+static char *user_input_str(const char __user *buf, size_t count, loff_t *ppos)
+{
+ char *kbuf;
+ ssize_t ret;
+
+ /* We do not accept continuous write */
+ if (*ppos)
+ return ERR_PTR(-EINVAL);
+
+ kbuf = kmalloc(count + 1, GFP_KERNEL);
+ if (!kbuf)
+ return ERR_PTR(-ENOMEM);
+
+ ret = simple_write_to_buffer(kbuf, count + 1, ppos, buf, count);
+ if (ret != count) {
+ kfree(kbuf);
+ return ERR_PTR(-EIO);
+ }
+ kbuf[ret] = '\0';
+
+ return kbuf;
+}
+
+static ssize_t dbgfs_attrs_read(struct file *file,
+ char __user *buf, size_t count, loff_t *ppos)
+{
+ struct damon_ctx *ctx = file->private_data;
+ char kbuf[128];
+ int ret;
+
+ mutex_lock(&ctx->kdamond_lock);
+ ret = scnprintf(kbuf, ARRAY_SIZE(kbuf), "%lu %lu %lu %lu %lu\n",
+ ctx->sample_interval, ctx->aggr_interval,
+ ctx->regions_update_interval, ctx->min_nr_regions,
+ ctx->max_nr_regions);
+ mutex_unlock(&ctx->kdamond_lock);
+
+ return simple_read_from_buffer(buf, count, ppos, kbuf, ret);
+}
+
+static ssize_t dbgfs_attrs_write(struct file *file,
+ const char __user *buf, size_t count, loff_t *ppos)
+{
+ struct damon_ctx *ctx = file->private_data;
+ unsigned long s, a, r, minr, maxr;
+ char *kbuf;
+ ssize_t ret = count;
+ int err;
+
+ kbuf = user_input_str(buf, count, ppos);
+ if (IS_ERR(kbuf))
+ return PTR_ERR(kbuf);
+
+ if (sscanf(kbuf, "%lu %lu %lu %lu %lu",
+ &s, &a, &r, &minr, &maxr) != 5) {
+ ret = -EINVAL;
+ goto out;
+ }
+
+ mutex_lock(&ctx->kdamond_lock);
+ if (ctx->kdamond) {
+ ret = -EBUSY;
+ goto unlock_out;
+ }
+
+ err = damon_set_attrs(ctx, s, a, r, minr, maxr);
+ if (err)
+ ret = err;
+unlock_out:
+ mutex_unlock(&ctx->kdamond_lock);
+out:
+ kfree(kbuf);
+ return ret;
+}
+
+#define targetid_is_pid(ctx) \
+ (ctx->primitive.target_valid == damon_va_target_valid)
+
+static ssize_t sprint_target_ids(struct damon_ctx *ctx, char *buf, ssize_t len)
+{
+ struct damon_target *t;
+ unsigned long id;
+ int written = 0;
+ int rc;
+
+ damon_for_each_target(t, ctx) {
+ id = t->id;
+ if (targetid_is_pid(ctx))
+ /* Show pid numbers to debugfs users */
+ id = (unsigned long)pid_vnr((struct pid *)id);
+
+ rc = scnprintf(&buf[written], len - written, "%lu ", id);
+ if (!rc)
+ return -ENOMEM;
+ written += rc;
+ }
+ if (written)
+ written -= 1;
+ written += scnprintf(&buf[written], len - written, "\n");
+ return written;
+}
+
+static ssize_t dbgfs_target_ids_read(struct file *file,
+ char __user *buf, size_t count, loff_t *ppos)
+{
+ struct damon_ctx *ctx = file->private_data;
+ ssize_t len;
+ char ids_buf[320];
+
+ mutex_lock(&ctx->kdamond_lock);
+ len = sprint_target_ids(ctx, ids_buf, 320);
+ mutex_unlock(&ctx->kdamond_lock);
+ if (len < 0)
+ return len;
+
+ return simple_read_from_buffer(buf, count, ppos, ids_buf, len);
+}
+
+/*
+ * Converts a string into an array of unsigned long integers
+ *
+ * Returns an array of unsigned long integers if the conversion success, or
+ * NULL otherwise.
+ */
+static unsigned long *str_to_target_ids(const char *str, ssize_t len,
+ ssize_t *nr_ids)
+{
+ unsigned long *ids;
+ const int max_nr_ids = 32;
+ unsigned long id;
+ int pos = 0, parsed, ret;
+
+ *nr_ids = 0;
+ ids = kmalloc_array(max_nr_ids, sizeof(id), GFP_KERNEL);
+ if (!ids)
+ return NULL;
+ while (*nr_ids < max_nr_ids && pos < len) {
+ ret = sscanf(&str[pos], "%lu%n", &id, &parsed);
+ pos += parsed;
+ if (ret != 1)
+ break;
+ ids[*nr_ids] = id;
+ *nr_ids += 1;
+ }
+
+ return ids;
+}
+
+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]);
+ }
+
+ mutex_lock(&ctx->kdamond_lock);
+ if (ctx->kdamond) {
+ ret = -EINVAL;
+ goto unlock_out;
+ }
+
+ err = damon_set_targets(ctx, targets, nr_targets);
+ if (err)
+ ret = err;
+unlock_out:
+ mutex_unlock(&ctx->kdamond_lock);
+ kfree(targets);
+out:
+ kfree(kbuf);
+ return ret;
+}
+
+static int damon_dbgfs_open(struct inode *inode, struct file *file)
+{
+ file->private_data = inode->i_private;
+
+ return nonseekable_open(inode, file);
+}
+
+static const struct file_operations attrs_fops = {
+ .owner = THIS_MODULE,
+ .open = damon_dbgfs_open,
+ .read = dbgfs_attrs_read,
+ .write = dbgfs_attrs_write,
+};
+
+static const struct file_operations target_ids_fops = {
+ .owner = THIS_MODULE,
+ .open = damon_dbgfs_open,
+ .read = dbgfs_target_ids_read,
+ .write = dbgfs_target_ids_write,
+};
+
+static int dbgfs_fill_ctx_dir(struct dentry *dir, struct damon_ctx *ctx)
+{
+ const char * const file_names[] = {"attrs", "target_ids"};
+ const struct file_operations *fops[] = {&attrs_fops, &target_ids_fops};
+ int i;
+
+ for (i = 0; i < ARRAY_SIZE(file_names); i++) {
+ if (!debugfs_create_file(file_names[i], 0600, dir,
+ ctx, fops[i])) {
+ pr_err("failed to create %s file\n", file_names[i]);
+ return -ENOMEM;
+ }
+ }
+
+ return 0;
+}
+
+static struct damon_ctx *dbgfs_new_ctx(void)
+{
+ struct damon_ctx *ctx;
+
+ ctx = damon_new_ctx(DAMON_ADAPTIVE_TARGET);
+ if (!ctx)
+ return NULL;
+
+ damon_va_set_primitives(ctx);
+ return ctx;
+}
+
+static ssize_t dbgfs_monitor_on_read(struct file *file,
+ char __user *buf, size_t count, loff_t *ppos)
+{
+ char monitor_on_buf[5];
+ bool monitor_on = damon_nr_running_ctxs() != 0;
+ int len;
+
+ len = scnprintf(monitor_on_buf, 5, monitor_on ? "on\n" : "off\n");
+
+ return simple_read_from_buffer(buf, count, ppos, monitor_on_buf, len);
+}
+
+static ssize_t dbgfs_monitor_on_write(struct file *file,
+ const char __user *buf, size_t count, loff_t *ppos)
+{
+ ssize_t ret = count;
+ char *kbuf;
+ int err;
+
+ kbuf = user_input_str(buf, count, ppos);
+ if (IS_ERR(kbuf))
+ return PTR_ERR(kbuf);
+
+ /* Remove white space */
+ if (sscanf(kbuf, "%s", kbuf) != 1) {
+ kfree(kbuf);
+ return -EINVAL;
+ }
+
+ if (!strncmp(kbuf, "on", count))
+ err = damon_start(dbgfs_ctxs, dbgfs_nr_ctxs);
+ else if (!strncmp(kbuf, "off", count))
+ err = damon_stop(dbgfs_ctxs, dbgfs_nr_ctxs);
+ else
+ err = -EINVAL;
+
+ if (err)
+ ret = err;
+ kfree(kbuf);
+ return ret;
+}
+
+static const struct file_operations monitor_on_fops = {
+ .owner = THIS_MODULE,
+ .read = dbgfs_monitor_on_read,
+ .write = dbgfs_monitor_on_write,
+};
+
+static int __init __damon_dbgfs_init(void)
+{
+ struct dentry *dbgfs_root;
+ const char * const file_names[] = {"monitor_on"};
+ const struct file_operations *fops[] = {&monitor_on_fops};
+ int i;
+
+ dbgfs_root = debugfs_create_dir("damon", NULL);
+ if (IS_ERR(dbgfs_root)) {
+ pr_err("failed to create the dbgfs dir\n");
+ return PTR_ERR(dbgfs_root);
+ }
+
+ for (i = 0; i < ARRAY_SIZE(file_names); i++) {
+ if (!debugfs_create_file(file_names[i], 0600, dbgfs_root,
+ NULL, fops[i])) {
+ pr_err("failed to create %s file\n", file_names[i]);
+ return -ENOMEM;
+ }
+ }
+ dbgfs_fill_ctx_dir(dbgfs_root, dbgfs_ctxs[0]);
+
+ dbgfs_dirs = kmalloc_array(1, sizeof(dbgfs_root), GFP_KERNEL);
+ dbgfs_dirs[0] = dbgfs_root;
+
+ return 0;
+}
+
+/*
+ * Functions for the initialization
+ */
+
+static int __init damon_dbgfs_init(void)
+{
+ int rc;
+
+ dbgfs_ctxs = kmalloc(sizeof(*dbgfs_ctxs), GFP_KERNEL);
+ dbgfs_ctxs[0] = dbgfs_new_ctx();
+ if (!dbgfs_ctxs[0])
+ return -ENOMEM;
+ dbgfs_nr_ctxs = 1;
+
+ rc = __damon_dbgfs_init();
+ if (rc)
+ pr_err("%s: dbgfs init failed\n", __func__);
+
+ return rc;
+}
+
+module_init(damon_dbgfs_init);
--
2.17.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v23 07/15] mm/damon: Implement a debugfs-based user space interface
2020-12-15 11:54 ` [PATCH v23 07/15] mm/damon: Implement a debugfs-based user space interface SeongJae Park
@ 2021-02-01 17:37 ` Shakeel Butt
0 siblings, 0 replies; 11+ messages in thread
From: Shakeel Butt @ 2021-02-01 17:37 UTC (permalink / raw)
To: SeongJae Park
Cc: Andrew Morton, SeongJae Park, Jonathan.Cameron, Andrea Arcangeli,
acme, alexander.shishkin, amit, benh, brendan.d.gregg,
Brendan Higgins, Qian Cai, Colin Ian King, Jonathan Corbet,
David Hildenbrand, dwmw, Marco Elver, Du, Fan, foersleo,
Greg Thelen, Ian Rogers, jolsa, Kirill A. Shutemov, Mark Rutland,
Mel Gorman, Minchan Kim, Ingo Molnar, namhyung,
Peter Zijlstra (Intel),
Randy Dunlap, Rik van Riel, David Rientjes, Steven Rostedt,
Mike Rapoport, sblbir, Shuah Khan, sj38.park, snu,
Vlastimil Babka, Vladimir Davydov, Yang Shi, Huang Ying,
zgf574564920, linux-damon, Linux MM, linux-doc, LKML
On Tue, Dec 15, 2020 at 3:59 AM SeongJae Park <sjpark@amazon.com> wrote:
>
> From: SeongJae Park <sjpark@amazon.de>
>
> 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?
> 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.
>
> 'damon-dbgfs' exports three files, ``attrs``, ``target_ids``, and
> ``monitor_on`` under its debugfs directory, ``<debugfs>/damon/``.
>
> Attributes
> ----------
>
> Users can read and write the ``sampling interval``, ``aggregation
> interval``, ``regions update interval``, and min/max number of
> monitoring target regions by reading from and writing to the ``attrs``
> file. For example, below commands set those values to 5 ms, 100 ms,
> 1,000 ms, 10, 1000 and check it again::
>
> # cd <debugfs>/damon
> # echo 5000 100000 1000000 10 1000 > attrs
> # cat attrs
> 5000 100000 1000000 10 1000
>
> Target IDs
> ----------
>
> Some types of address spaces supports multiple monitoring target. For
> example, the virtual memory address spaces monitoring can have multiple
> processes as the monitoring targets. Users can set the targets by
> writing relevant id values of the targets to, and get the ids of the
> current targets by reading from the ``target_ids`` file. In case of the
> virtual address spaces monitoring, the values should be pids of the
> monitoring target processes. For example, below commands set processes
> having pids 42 and 4242 as the monitoring targets and check it again::
>
> # cd <debugfs>/damon
> # echo 42 4242 > target_ids
> # cat target_ids
> 42 4242
>
> Note that setting the target ids doesn't start the monitoring.
>
> Turning On/Off
> --------------
>
> Setting the files as described above doesn't incur effect unless you
> explicitly start the monitoring. You can start, stop, and check the
> current status of the monitoring by writing to and reading from the
> ``monitor_on`` file. Writing ``on`` to the file starts the monitoring
> of the targets with the attributes. Writing ``off`` to the file stops
> those. DAMON also stops if every targets are invalidated (in case of
> the virtual memory monitoring, target processes are invalidated when
> terminated). Below example commands turn on, off, and check the status
> of DAMON::
>
> # cd <debugfs>/damon
> # echo on > monitor_on
> # echo off > monitor_on
> # cat monitor_on
> off
>
> Please note that you cannot write to the above-mentioned debugfs files
> while the monitoring is turned on. If you write to the files while
> DAMON is running, an error code such as ``-EBUSY`` will be returned.
>
> Signed-off-by: SeongJae Park <sjpark@amazon.de>
> Reviewed-by: Leonard Foerster <foersleo@amazon.de>
> ---
> include/linux/damon.h | 3 +
> mm/damon/Kconfig | 9 ++
> mm/damon/Makefile | 1 +
> mm/damon/core.c | 45 ++++++
> mm/damon/dbgfs.c | 366 ++++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 424 insertions(+)
> create mode 100644 mm/damon/dbgfs.c
>
> diff --git a/include/linux/damon.h b/include/linux/damon.h
> index 39b4d6d3ddee..f9e0d4349352 100644
> --- a/include/linux/damon.h
> +++ b/include/linux/damon.h
> @@ -265,9 +265,12 @@ unsigned int damon_nr_regions(struct damon_target *t);
>
> struct damon_ctx *damon_new_ctx(enum damon_target_type type);
> void damon_destroy_ctx(struct damon_ctx *ctx);
> +int damon_set_targets(struct damon_ctx *ctx,
> + unsigned long *ids, ssize_t nr_ids);
> int damon_set_attrs(struct damon_ctx *ctx, unsigned long sample_int,
> unsigned long aggr_int, unsigned long regions_update_int,
> unsigned long min_nr_reg, unsigned long max_nr_reg);
> +int damon_nr_running_ctxs(void);
>
> int damon_start(struct damon_ctx **ctxs, int nr_ctxs);
> int damon_stop(struct damon_ctx **ctxs, int nr_ctxs);
> diff --git a/mm/damon/Kconfig b/mm/damon/Kconfig
> index 8ae080c52950..72f1683ba0ee 100644
> --- a/mm/damon/Kconfig
> +++ b/mm/damon/Kconfig
> @@ -21,4 +21,13 @@ config DAMON_VADDR
> This builds the default data access monitoring primitives for DAMON
> that works for virtual address spaces.
>
> +config DAMON_DBGFS
> + bool "DAMON debugfs interface"
> + depends on DAMON_VADDR && DEBUG_FS
> + help
> + This builds the debugfs interface for DAMON. The user space admins
> + can use the interface for arbitrary data access monitoring.
> +
> + If unsure, say N.
> +
> endmenu
> diff --git a/mm/damon/Makefile b/mm/damon/Makefile
> index 6ebbd08aed67..fed4be3bace3 100644
> --- a/mm/damon/Makefile
> +++ b/mm/damon/Makefile
> @@ -2,3 +2,4 @@
>
> obj-$(CONFIG_DAMON) := core.o
> obj-$(CONFIG_DAMON_VADDR) += vaddr.o
> +obj-$(CONFIG_DAMON_DBGFS) += dbgfs.o
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 5ca9f79ccbb6..b9575a6bebff 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -166,6 +166,37 @@ void damon_destroy_ctx(struct damon_ctx *ctx)
> kfree(ctx);
> }
>
> +/**
> + * damon_set_targets() - Set monitoring targets.
> + * @ctx: monitoring context
> + * @ids: array of target ids
> + * @nr_ids: number of entries in @ids
> + *
> + * This function should not be called while the kdamond is running.
> + *
> + * Return: 0 on success, negative error code otherwise.
> + */
> +int damon_set_targets(struct damon_ctx *ctx,
> + unsigned long *ids, ssize_t nr_ids)
> +{
> + ssize_t i;
> + struct damon_target *t, *next;
> +
> + damon_for_each_target_safe(t, next, ctx)
> + damon_destroy_target(t);
You need to put the reference on the target before destroying.
> +
> + for (i = 0; i < nr_ids; i++) {
> + t = damon_new_target(ids[i]);
> + if (!t) {
> + pr_err("Failed to alloc damon_target\n");
> + return -ENOMEM;
> + }
> + damon_add_target(ctx, t);
> + }
> +
> + return 0;
> +}
> +
> /**
> * damon_set_attrs() - Set attributes for the monitoring.
> * @ctx: monitoring context
> @@ -206,6 +237,20 @@ int damon_set_attrs(struct damon_ctx *ctx, unsigned long sample_int,
> return 0;
> }
>
> +/**
> + * damon_nr_running_ctxs() - Return number of currently running contexts.
> + */
> +int damon_nr_running_ctxs(void)
> +{
> + int nr_ctxs;
> +
> + mutex_lock(&damon_lock);
> + nr_ctxs = nr_running_ctxs;
> + mutex_unlock(&damon_lock);
READ_ONCE(nr_running_ctxs) ?
> +
> + return nr_ctxs;
> +}
> +
> /* Returns the size upper limit for each monitoring region */
> static unsigned long damon_region_sz_limit(struct damon_ctx *ctx)
> {
> diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
> new file mode 100644
> index 000000000000..fd1665a183c2
> --- /dev/null
> +++ b/mm/damon/dbgfs.c
> @@ -0,0 +1,366 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DAMON Debugfs Interface
> + *
> + * Author: SeongJae Park <sjpark@amazon.de>
> + */
> +
> +#define pr_fmt(fmt) "damon-dbgfs: " fmt
> +
> +#include <linux/damon.h>
> +#include <linux/debugfs.h>
> +#include <linux/file.h>
> +#include <linux/mm.h>
> +#include <linux/module.h>
> +#include <linux/page_idle.h>
> +#include <linux/slab.h>
> +
> +static struct damon_ctx **dbgfs_ctxs;
> +static int dbgfs_nr_ctxs;
> +static struct dentry **dbgfs_dirs;
> +
> +/*
> + * Returns non-empty string on success, negarive error code otherwise.
> + */
> +static char *user_input_str(const char __user *buf, size_t count, loff_t *ppos)
> +{
> + char *kbuf;
> + ssize_t ret;
> +
> + /* We do not accept continuous write */
> + if (*ppos)
> + return ERR_PTR(-EINVAL);
> +
> + kbuf = kmalloc(count + 1, GFP_KERNEL);
> + if (!kbuf)
> + return ERR_PTR(-ENOMEM);
> +
> + ret = simple_write_to_buffer(kbuf, count + 1, ppos, buf, count);
> + if (ret != count) {
> + kfree(kbuf);
> + return ERR_PTR(-EIO);
> + }
> + kbuf[ret] = '\0';
> +
> + return kbuf;
> +}
> +
> +static ssize_t dbgfs_attrs_read(struct file *file,
> + char __user *buf, size_t count, loff_t *ppos)
> +{
> + struct damon_ctx *ctx = file->private_data;
> + char kbuf[128];
> + int ret;
> +
> + mutex_lock(&ctx->kdamond_lock);
> + ret = scnprintf(kbuf, ARRAY_SIZE(kbuf), "%lu %lu %lu %lu %lu\n",
> + ctx->sample_interval, ctx->aggr_interval,
> + ctx->regions_update_interval, ctx->min_nr_regions,
> + ctx->max_nr_regions);
> + mutex_unlock(&ctx->kdamond_lock);
> +
> + return simple_read_from_buffer(buf, count, ppos, kbuf, ret);
> +}
> +
> +static ssize_t dbgfs_attrs_write(struct file *file,
> + const char __user *buf, size_t count, loff_t *ppos)
> +{
> + struct damon_ctx *ctx = file->private_data;
> + unsigned long s, a, r, minr, maxr;
> + char *kbuf;
> + ssize_t ret = count;
> + int err;
> +
> + kbuf = user_input_str(buf, count, ppos);
> + if (IS_ERR(kbuf))
> + return PTR_ERR(kbuf);
> +
> + if (sscanf(kbuf, "%lu %lu %lu %lu %lu",
> + &s, &a, &r, &minr, &maxr) != 5) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + mutex_lock(&ctx->kdamond_lock);
> + if (ctx->kdamond) {
> + ret = -EBUSY;
> + goto unlock_out;
> + }
> +
> + err = damon_set_attrs(ctx, s, a, r, minr, maxr);
> + if (err)
> + ret = err;
> +unlock_out:
> + mutex_unlock(&ctx->kdamond_lock);
> +out:
> + kfree(kbuf);
> + return ret;
> +}
> +
> +#define targetid_is_pid(ctx) \
> + (ctx->primitive.target_valid == damon_va_target_valid)
> +
> +static ssize_t sprint_target_ids(struct damon_ctx *ctx, char *buf, ssize_t len)
> +{
> + struct damon_target *t;
> + unsigned long id;
> + int written = 0;
> + int rc;
> +
> + damon_for_each_target(t, ctx) {
> + id = t->id;
> + if (targetid_is_pid(ctx))
> + /* Show pid numbers to debugfs users */
> + id = (unsigned long)pid_vnr((struct pid *)id);
> +
> + rc = scnprintf(&buf[written], len - written, "%lu ", id);
> + if (!rc)
> + return -ENOMEM;
> + written += rc;
> + }
> + if (written)
> + written -= 1;
> + written += scnprintf(&buf[written], len - written, "\n");
> + return written;
> +}
> +
> +static ssize_t dbgfs_target_ids_read(struct file *file,
> + char __user *buf, size_t count, loff_t *ppos)
> +{
> + struct damon_ctx *ctx = file->private_data;
> + ssize_t len;
> + char ids_buf[320];
> +
> + mutex_lock(&ctx->kdamond_lock);
> + len = sprint_target_ids(ctx, ids_buf, 320);
> + mutex_unlock(&ctx->kdamond_lock);
> + if (len < 0)
> + return len;
> +
> + return simple_read_from_buffer(buf, count, ppos, ids_buf, len);
> +}
> +
> +/*
> + * Converts a string into an array of unsigned long integers
> + *
> + * Returns an array of unsigned long integers if the conversion success, or
> + * NULL otherwise.
> + */
> +static unsigned long *str_to_target_ids(const char *str, ssize_t len,
> + ssize_t *nr_ids)
> +{
> + unsigned long *ids;
> + const int max_nr_ids = 32;
> + unsigned long id;
> + int pos = 0, parsed, ret;
> +
> + *nr_ids = 0;
> + ids = kmalloc_array(max_nr_ids, sizeof(id), GFP_KERNEL);
> + if (!ids)
> + return NULL;
> + while (*nr_ids < max_nr_ids && pos < len) {
> + ret = sscanf(&str[pos], "%lu%n", &id, &parsed);
> + pos += parsed;
> + if (ret != 1)
> + break;
> + ids[*nr_ids] = id;
> + *nr_ids += 1;
> + }
> +
> + return ids;
> +}
> +
> +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]);
> + }
> +
> + mutex_lock(&ctx->kdamond_lock);
> + if (ctx->kdamond) {
> + ret = -EINVAL;
> + goto unlock_out;
You need to put_pid on the targets array.
> + }
> +
> + err = damon_set_targets(ctx, targets, nr_targets);
> + if (err)
> + ret = err;
You need to handle the partial failure from damon_set_targets().
> +unlock_out:
> + mutex_unlock(&ctx->kdamond_lock);
> + kfree(targets);
> +out:
> + kfree(kbuf);
> + return ret;
> +}
> +
> +static int damon_dbgfs_open(struct inode *inode, struct file *file)
> +{
> + file->private_data = inode->i_private;
> +
> + return nonseekable_open(inode, file);
> +}
> +
> +static const struct file_operations attrs_fops = {
> + .owner = THIS_MODULE,
> + .open = damon_dbgfs_open,
> + .read = dbgfs_attrs_read,
> + .write = dbgfs_attrs_write,
> +};
> +
> +static const struct file_operations target_ids_fops = {
> + .owner = THIS_MODULE,
> + .open = damon_dbgfs_open,
> + .read = dbgfs_target_ids_read,
> + .write = dbgfs_target_ids_write,
> +};
> +
> +static int dbgfs_fill_ctx_dir(struct dentry *dir, struct damon_ctx *ctx)
> +{
> + const char * const file_names[] = {"attrs", "target_ids"};
> + const struct file_operations *fops[] = {&attrs_fops, &target_ids_fops};
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(file_names); i++) {
> + if (!debugfs_create_file(file_names[i], 0600, dir,
> + ctx, fops[i])) {
> + pr_err("failed to create %s file\n", file_names[i]);
> + return -ENOMEM;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static struct damon_ctx *dbgfs_new_ctx(void)
> +{
> + struct damon_ctx *ctx;
> +
> + ctx = damon_new_ctx(DAMON_ADAPTIVE_TARGET);
> + if (!ctx)
> + return NULL;
> +
> + damon_va_set_primitives(ctx);
> + return ctx;
> +}
> +
> +static ssize_t dbgfs_monitor_on_read(struct file *file,
> + char __user *buf, size_t count, loff_t *ppos)
> +{
> + char monitor_on_buf[5];
> + bool monitor_on = damon_nr_running_ctxs() != 0;
> + int len;
> +
> + len = scnprintf(monitor_on_buf, 5, monitor_on ? "on\n" : "off\n");
> +
> + return simple_read_from_buffer(buf, count, ppos, monitor_on_buf, len);
> +}
> +
> +static ssize_t dbgfs_monitor_on_write(struct file *file,
> + const char __user *buf, size_t count, loff_t *ppos)
> +{
> + ssize_t ret = count;
> + char *kbuf;
> + int err;
> +
> + kbuf = user_input_str(buf, count, ppos);
> + if (IS_ERR(kbuf))
> + return PTR_ERR(kbuf);
> +
> + /* Remove white space */
> + if (sscanf(kbuf, "%s", kbuf) != 1) {
> + kfree(kbuf);
> + return -EINVAL;
> + }
> +
> + if (!strncmp(kbuf, "on", count))
> + err = damon_start(dbgfs_ctxs, dbgfs_nr_ctxs);
> + else if (!strncmp(kbuf, "off", count))
> + err = damon_stop(dbgfs_ctxs, dbgfs_nr_ctxs);
> + else
> + err = -EINVAL;
> +
> + if (err)
> + ret = err;
> + kfree(kbuf);
> + return ret;
> +}
> +
> +static const struct file_operations monitor_on_fops = {
> + .owner = THIS_MODULE,
> + .read = dbgfs_monitor_on_read,
> + .write = dbgfs_monitor_on_write,
> +};
> +
> +static int __init __damon_dbgfs_init(void)
> +{
> + struct dentry *dbgfs_root;
> + const char * const file_names[] = {"monitor_on"};
> + const struct file_operations *fops[] = {&monitor_on_fops};
> + int i;
> +
> + dbgfs_root = debugfs_create_dir("damon", NULL);
> + if (IS_ERR(dbgfs_root)) {
> + pr_err("failed to create the dbgfs dir\n");
> + return PTR_ERR(dbgfs_root);
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(file_names); i++) {
> + if (!debugfs_create_file(file_names[i], 0600, dbgfs_root,
> + NULL, fops[i])) {
> + pr_err("failed to create %s file\n", file_names[i]);
> + return -ENOMEM;
> + }
> + }
> + dbgfs_fill_ctx_dir(dbgfs_root, dbgfs_ctxs[0]);
> +
> + dbgfs_dirs = kmalloc_array(1, sizeof(dbgfs_root), GFP_KERNEL);
> + dbgfs_dirs[0] = dbgfs_root;
> +
> + return 0;
> +}
> +
> +/*
> + * Functions for the initialization
> + */
> +
> +static int __init damon_dbgfs_init(void)
> +{
> + int rc;
> +
> + dbgfs_ctxs = kmalloc(sizeof(*dbgfs_ctxs), GFP_KERNEL);
> + dbgfs_ctxs[0] = dbgfs_new_ctx();
> + if (!dbgfs_ctxs[0])
> + return -ENOMEM;
> + dbgfs_nr_ctxs = 1;
> +
> + rc = __damon_dbgfs_init();
> + if (rc)
> + pr_err("%s: dbgfs init failed\n", __func__);
> +
> + return rc;
> +}
> +
> +module_init(damon_dbgfs_init);
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v23 07/15] mm/damon: Implement a debugfs-based user space interface
@ 2021-02-01 17:37 ` Shakeel Butt
0 siblings, 0 replies; 11+ messages in thread
From: Shakeel Butt @ 2021-02-01 17:37 UTC (permalink / raw)
To: SeongJae Park
Cc: Andrew Morton, SeongJae Park, Jonathan.Cameron, Andrea Arcangeli,
acme, alexander.shishkin, amit, benh, brendan.d.gregg,
Brendan Higgins, Qian Cai, Colin Ian King, Jonathan Corbet,
David Hildenbrand, dwmw, Marco Elver, Du, Fan, foersleo,
Greg Thelen, Ian Rogers, jolsa, Kirill A. Shutemov, Mark Rutland,
Mel Gorman, Minchan Kim, Ingo Molnar, namhyung,
Peter Zijlstra (Intel),
Randy Dunlap, Rik van Riel, David Rientjes, Steven Rostedt,
Mike Rapoport, sblbir, Shuah Khan, sj38.park, snu,
Vlastimil Babka, Vladimir Davydov, Yang Shi, Huang Ying,
zgf574564920, linux-damon, Linux MM, linux-doc, LKML
On Tue, Dec 15, 2020 at 3:59 AM SeongJae Park <sjpark@amazon.com> wrote:
>
> From: SeongJae Park <sjpark@amazon.de>
>
> 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?
> 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.
>
> 'damon-dbgfs' exports three files, ``attrs``, ``target_ids``, and
> ``monitor_on`` under its debugfs directory, ``<debugfs>/damon/``.
>
> Attributes
> ----------
>
> Users can read and write the ``sampling interval``, ``aggregation
> interval``, ``regions update interval``, and min/max number of
> monitoring target regions by reading from and writing to the ``attrs``
> file. For example, below commands set those values to 5 ms, 100 ms,
> 1,000 ms, 10, 1000 and check it again::
>
> # cd <debugfs>/damon
> # echo 5000 100000 1000000 10 1000 > attrs
> # cat attrs
> 5000 100000 1000000 10 1000
>
> Target IDs
> ----------
>
> Some types of address spaces supports multiple monitoring target. For
> example, the virtual memory address spaces monitoring can have multiple
> processes as the monitoring targets. Users can set the targets by
> writing relevant id values of the targets to, and get the ids of the
> current targets by reading from the ``target_ids`` file. In case of the
> virtual address spaces monitoring, the values should be pids of the
> monitoring target processes. For example, below commands set processes
> having pids 42 and 4242 as the monitoring targets and check it again::
>
> # cd <debugfs>/damon
> # echo 42 4242 > target_ids
> # cat target_ids
> 42 4242
>
> Note that setting the target ids doesn't start the monitoring.
>
> Turning On/Off
> --------------
>
> Setting the files as described above doesn't incur effect unless you
> explicitly start the monitoring. You can start, stop, and check the
> current status of the monitoring by writing to and reading from the
> ``monitor_on`` file. Writing ``on`` to the file starts the monitoring
> of the targets with the attributes. Writing ``off`` to the file stops
> those. DAMON also stops if every targets are invalidated (in case of
> the virtual memory monitoring, target processes are invalidated when
> terminated). Below example commands turn on, off, and check the status
> of DAMON::
>
> # cd <debugfs>/damon
> # echo on > monitor_on
> # echo off > monitor_on
> # cat monitor_on
> off
>
> Please note that you cannot write to the above-mentioned debugfs files
> while the monitoring is turned on. If you write to the files while
> DAMON is running, an error code such as ``-EBUSY`` will be returned.
>
> Signed-off-by: SeongJae Park <sjpark@amazon.de>
> Reviewed-by: Leonard Foerster <foersleo@amazon.de>
> ---
> include/linux/damon.h | 3 +
> mm/damon/Kconfig | 9 ++
> mm/damon/Makefile | 1 +
> mm/damon/core.c | 45 ++++++
> mm/damon/dbgfs.c | 366 ++++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 424 insertions(+)
> create mode 100644 mm/damon/dbgfs.c
>
> diff --git a/include/linux/damon.h b/include/linux/damon.h
> index 39b4d6d3ddee..f9e0d4349352 100644
> --- a/include/linux/damon.h
> +++ b/include/linux/damon.h
> @@ -265,9 +265,12 @@ unsigned int damon_nr_regions(struct damon_target *t);
>
> struct damon_ctx *damon_new_ctx(enum damon_target_type type);
> void damon_destroy_ctx(struct damon_ctx *ctx);
> +int damon_set_targets(struct damon_ctx *ctx,
> + unsigned long *ids, ssize_t nr_ids);
> int damon_set_attrs(struct damon_ctx *ctx, unsigned long sample_int,
> unsigned long aggr_int, unsigned long regions_update_int,
> unsigned long min_nr_reg, unsigned long max_nr_reg);
> +int damon_nr_running_ctxs(void);
>
> int damon_start(struct damon_ctx **ctxs, int nr_ctxs);
> int damon_stop(struct damon_ctx **ctxs, int nr_ctxs);
> diff --git a/mm/damon/Kconfig b/mm/damon/Kconfig
> index 8ae080c52950..72f1683ba0ee 100644
> --- a/mm/damon/Kconfig
> +++ b/mm/damon/Kconfig
> @@ -21,4 +21,13 @@ config DAMON_VADDR
> This builds the default data access monitoring primitives for DAMON
> that works for virtual address spaces.
>
> +config DAMON_DBGFS
> + bool "DAMON debugfs interface"
> + depends on DAMON_VADDR && DEBUG_FS
> + help
> + This builds the debugfs interface for DAMON. The user space admins
> + can use the interface for arbitrary data access monitoring.
> +
> + If unsure, say N.
> +
> endmenu
> diff --git a/mm/damon/Makefile b/mm/damon/Makefile
> index 6ebbd08aed67..fed4be3bace3 100644
> --- a/mm/damon/Makefile
> +++ b/mm/damon/Makefile
> @@ -2,3 +2,4 @@
>
> obj-$(CONFIG_DAMON) := core.o
> obj-$(CONFIG_DAMON_VADDR) += vaddr.o
> +obj-$(CONFIG_DAMON_DBGFS) += dbgfs.o
> diff --git a/mm/damon/core.c b/mm/damon/core.c
> index 5ca9f79ccbb6..b9575a6bebff 100644
> --- a/mm/damon/core.c
> +++ b/mm/damon/core.c
> @@ -166,6 +166,37 @@ void damon_destroy_ctx(struct damon_ctx *ctx)
> kfree(ctx);
> }
>
> +/**
> + * damon_set_targets() - Set monitoring targets.
> + * @ctx: monitoring context
> + * @ids: array of target ids
> + * @nr_ids: number of entries in @ids
> + *
> + * This function should not be called while the kdamond is running.
> + *
> + * Return: 0 on success, negative error code otherwise.
> + */
> +int damon_set_targets(struct damon_ctx *ctx,
> + unsigned long *ids, ssize_t nr_ids)
> +{
> + ssize_t i;
> + struct damon_target *t, *next;
> +
> + damon_for_each_target_safe(t, next, ctx)
> + damon_destroy_target(t);
You need to put the reference on the target before destroying.
> +
> + for (i = 0; i < nr_ids; i++) {
> + t = damon_new_target(ids[i]);
> + if (!t) {
> + pr_err("Failed to alloc damon_target\n");
> + return -ENOMEM;
> + }
> + damon_add_target(ctx, t);
> + }
> +
> + return 0;
> +}
> +
> /**
> * damon_set_attrs() - Set attributes for the monitoring.
> * @ctx: monitoring context
> @@ -206,6 +237,20 @@ int damon_set_attrs(struct damon_ctx *ctx, unsigned long sample_int,
> return 0;
> }
>
> +/**
> + * damon_nr_running_ctxs() - Return number of currently running contexts.
> + */
> +int damon_nr_running_ctxs(void)
> +{
> + int nr_ctxs;
> +
> + mutex_lock(&damon_lock);
> + nr_ctxs = nr_running_ctxs;
> + mutex_unlock(&damon_lock);
READ_ONCE(nr_running_ctxs) ?
> +
> + return nr_ctxs;
> +}
> +
> /* Returns the size upper limit for each monitoring region */
> static unsigned long damon_region_sz_limit(struct damon_ctx *ctx)
> {
> diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
> new file mode 100644
> index 000000000000..fd1665a183c2
> --- /dev/null
> +++ b/mm/damon/dbgfs.c
> @@ -0,0 +1,366 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * DAMON Debugfs Interface
> + *
> + * Author: SeongJae Park <sjpark@amazon.de>
> + */
> +
> +#define pr_fmt(fmt) "damon-dbgfs: " fmt
> +
> +#include <linux/damon.h>
> +#include <linux/debugfs.h>
> +#include <linux/file.h>
> +#include <linux/mm.h>
> +#include <linux/module.h>
> +#include <linux/page_idle.h>
> +#include <linux/slab.h>
> +
> +static struct damon_ctx **dbgfs_ctxs;
> +static int dbgfs_nr_ctxs;
> +static struct dentry **dbgfs_dirs;
> +
> +/*
> + * Returns non-empty string on success, negarive error code otherwise.
> + */
> +static char *user_input_str(const char __user *buf, size_t count, loff_t *ppos)
> +{
> + char *kbuf;
> + ssize_t ret;
> +
> + /* We do not accept continuous write */
> + if (*ppos)
> + return ERR_PTR(-EINVAL);
> +
> + kbuf = kmalloc(count + 1, GFP_KERNEL);
> + if (!kbuf)
> + return ERR_PTR(-ENOMEM);
> +
> + ret = simple_write_to_buffer(kbuf, count + 1, ppos, buf, count);
> + if (ret != count) {
> + kfree(kbuf);
> + return ERR_PTR(-EIO);
> + }
> + kbuf[ret] = '\0';
> +
> + return kbuf;
> +}
> +
> +static ssize_t dbgfs_attrs_read(struct file *file,
> + char __user *buf, size_t count, loff_t *ppos)
> +{
> + struct damon_ctx *ctx = file->private_data;
> + char kbuf[128];
> + int ret;
> +
> + mutex_lock(&ctx->kdamond_lock);
> + ret = scnprintf(kbuf, ARRAY_SIZE(kbuf), "%lu %lu %lu %lu %lu\n",
> + ctx->sample_interval, ctx->aggr_interval,
> + ctx->regions_update_interval, ctx->min_nr_regions,
> + ctx->max_nr_regions);
> + mutex_unlock(&ctx->kdamond_lock);
> +
> + return simple_read_from_buffer(buf, count, ppos, kbuf, ret);
> +}
> +
> +static ssize_t dbgfs_attrs_write(struct file *file,
> + const char __user *buf, size_t count, loff_t *ppos)
> +{
> + struct damon_ctx *ctx = file->private_data;
> + unsigned long s, a, r, minr, maxr;
> + char *kbuf;
> + ssize_t ret = count;
> + int err;
> +
> + kbuf = user_input_str(buf, count, ppos);
> + if (IS_ERR(kbuf))
> + return PTR_ERR(kbuf);
> +
> + if (sscanf(kbuf, "%lu %lu %lu %lu %lu",
> + &s, &a, &r, &minr, &maxr) != 5) {
> + ret = -EINVAL;
> + goto out;
> + }
> +
> + mutex_lock(&ctx->kdamond_lock);
> + if (ctx->kdamond) {
> + ret = -EBUSY;
> + goto unlock_out;
> + }
> +
> + err = damon_set_attrs(ctx, s, a, r, minr, maxr);
> + if (err)
> + ret = err;
> +unlock_out:
> + mutex_unlock(&ctx->kdamond_lock);
> +out:
> + kfree(kbuf);
> + return ret;
> +}
> +
> +#define targetid_is_pid(ctx) \
> + (ctx->primitive.target_valid == damon_va_target_valid)
> +
> +static ssize_t sprint_target_ids(struct damon_ctx *ctx, char *buf, ssize_t len)
> +{
> + struct damon_target *t;
> + unsigned long id;
> + int written = 0;
> + int rc;
> +
> + damon_for_each_target(t, ctx) {
> + id = t->id;
> + if (targetid_is_pid(ctx))
> + /* Show pid numbers to debugfs users */
> + id = (unsigned long)pid_vnr((struct pid *)id);
> +
> + rc = scnprintf(&buf[written], len - written, "%lu ", id);
> + if (!rc)
> + return -ENOMEM;
> + written += rc;
> + }
> + if (written)
> + written -= 1;
> + written += scnprintf(&buf[written], len - written, "\n");
> + return written;
> +}
> +
> +static ssize_t dbgfs_target_ids_read(struct file *file,
> + char __user *buf, size_t count, loff_t *ppos)
> +{
> + struct damon_ctx *ctx = file->private_data;
> + ssize_t len;
> + char ids_buf[320];
> +
> + mutex_lock(&ctx->kdamond_lock);
> + len = sprint_target_ids(ctx, ids_buf, 320);
> + mutex_unlock(&ctx->kdamond_lock);
> + if (len < 0)
> + return len;
> +
> + return simple_read_from_buffer(buf, count, ppos, ids_buf, len);
> +}
> +
> +/*
> + * Converts a string into an array of unsigned long integers
> + *
> + * Returns an array of unsigned long integers if the conversion success, or
> + * NULL otherwise.
> + */
> +static unsigned long *str_to_target_ids(const char *str, ssize_t len,
> + ssize_t *nr_ids)
> +{
> + unsigned long *ids;
> + const int max_nr_ids = 32;
> + unsigned long id;
> + int pos = 0, parsed, ret;
> +
> + *nr_ids = 0;
> + ids = kmalloc_array(max_nr_ids, sizeof(id), GFP_KERNEL);
> + if (!ids)
> + return NULL;
> + while (*nr_ids < max_nr_ids && pos < len) {
> + ret = sscanf(&str[pos], "%lu%n", &id, &parsed);
> + pos += parsed;
> + if (ret != 1)
> + break;
> + ids[*nr_ids] = id;
> + *nr_ids += 1;
> + }
> +
> + return ids;
> +}
> +
> +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]);
> + }
> +
> + mutex_lock(&ctx->kdamond_lock);
> + if (ctx->kdamond) {
> + ret = -EINVAL;
> + goto unlock_out;
You need to put_pid on the targets array.
> + }
> +
> + err = damon_set_targets(ctx, targets, nr_targets);
> + if (err)
> + ret = err;
You need to handle the partial failure from damon_set_targets().
> +unlock_out:
> + mutex_unlock(&ctx->kdamond_lock);
> + kfree(targets);
> +out:
> + kfree(kbuf);
> + return ret;
> +}
> +
> +static int damon_dbgfs_open(struct inode *inode, struct file *file)
> +{
> + file->private_data = inode->i_private;
> +
> + return nonseekable_open(inode, file);
> +}
> +
> +static const struct file_operations attrs_fops = {
> + .owner = THIS_MODULE,
> + .open = damon_dbgfs_open,
> + .read = dbgfs_attrs_read,
> + .write = dbgfs_attrs_write,
> +};
> +
> +static const struct file_operations target_ids_fops = {
> + .owner = THIS_MODULE,
> + .open = damon_dbgfs_open,
> + .read = dbgfs_target_ids_read,
> + .write = dbgfs_target_ids_write,
> +};
> +
> +static int dbgfs_fill_ctx_dir(struct dentry *dir, struct damon_ctx *ctx)
> +{
> + const char * const file_names[] = {"attrs", "target_ids"};
> + const struct file_operations *fops[] = {&attrs_fops, &target_ids_fops};
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(file_names); i++) {
> + if (!debugfs_create_file(file_names[i], 0600, dir,
> + ctx, fops[i])) {
> + pr_err("failed to create %s file\n", file_names[i]);
> + return -ENOMEM;
> + }
> + }
> +
> + return 0;
> +}
> +
> +static struct damon_ctx *dbgfs_new_ctx(void)
> +{
> + struct damon_ctx *ctx;
> +
> + ctx = damon_new_ctx(DAMON_ADAPTIVE_TARGET);
> + if (!ctx)
> + return NULL;
> +
> + damon_va_set_primitives(ctx);
> + return ctx;
> +}
> +
> +static ssize_t dbgfs_monitor_on_read(struct file *file,
> + char __user *buf, size_t count, loff_t *ppos)
> +{
> + char monitor_on_buf[5];
> + bool monitor_on = damon_nr_running_ctxs() != 0;
> + int len;
> +
> + len = scnprintf(monitor_on_buf, 5, monitor_on ? "on\n" : "off\n");
> +
> + return simple_read_from_buffer(buf, count, ppos, monitor_on_buf, len);
> +}
> +
> +static ssize_t dbgfs_monitor_on_write(struct file *file,
> + const char __user *buf, size_t count, loff_t *ppos)
> +{
> + ssize_t ret = count;
> + char *kbuf;
> + int err;
> +
> + kbuf = user_input_str(buf, count, ppos);
> + if (IS_ERR(kbuf))
> + return PTR_ERR(kbuf);
> +
> + /* Remove white space */
> + if (sscanf(kbuf, "%s", kbuf) != 1) {
> + kfree(kbuf);
> + return -EINVAL;
> + }
> +
> + if (!strncmp(kbuf, "on", count))
> + err = damon_start(dbgfs_ctxs, dbgfs_nr_ctxs);
> + else if (!strncmp(kbuf, "off", count))
> + err = damon_stop(dbgfs_ctxs, dbgfs_nr_ctxs);
> + else
> + err = -EINVAL;
> +
> + if (err)
> + ret = err;
> + kfree(kbuf);
> + return ret;
> +}
> +
> +static const struct file_operations monitor_on_fops = {
> + .owner = THIS_MODULE,
> + .read = dbgfs_monitor_on_read,
> + .write = dbgfs_monitor_on_write,
> +};
> +
> +static int __init __damon_dbgfs_init(void)
> +{
> + struct dentry *dbgfs_root;
> + const char * const file_names[] = {"monitor_on"};
> + const struct file_operations *fops[] = {&monitor_on_fops};
> + int i;
> +
> + dbgfs_root = debugfs_create_dir("damon", NULL);
> + if (IS_ERR(dbgfs_root)) {
> + pr_err("failed to create the dbgfs dir\n");
> + return PTR_ERR(dbgfs_root);
> + }
> +
> + for (i = 0; i < ARRAY_SIZE(file_names); i++) {
> + if (!debugfs_create_file(file_names[i], 0600, dbgfs_root,
> + NULL, fops[i])) {
> + pr_err("failed to create %s file\n", file_names[i]);
> + return -ENOMEM;
> + }
> + }
> + dbgfs_fill_ctx_dir(dbgfs_root, dbgfs_ctxs[0]);
> +
> + dbgfs_dirs = kmalloc_array(1, sizeof(dbgfs_root), GFP_KERNEL);
> + dbgfs_dirs[0] = dbgfs_root;
> +
> + return 0;
> +}
> +
> +/*
> + * Functions for the initialization
> + */
> +
> +static int __init damon_dbgfs_init(void)
> +{
> + int rc;
> +
> + dbgfs_ctxs = kmalloc(sizeof(*dbgfs_ctxs), GFP_KERNEL);
> + dbgfs_ctxs[0] = dbgfs_new_ctx();
> + if (!dbgfs_ctxs[0])
> + return -ENOMEM;
> + dbgfs_nr_ctxs = 1;
> +
> + rc = __damon_dbgfs_init();
> + if (rc)
> + pr_err("%s: dbgfs init failed\n", __func__);
> +
> + return rc;
> +}
> +
> +module_init(damon_dbgfs_init);
> --
> 2.17.1
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v23 07/15] mm/damon: Implement a debugfs-based user space interface
2021-02-01 17:37 ` Shakeel Butt
(?)
@ 2021-02-02 10:00 ` SeongJae Park
-1 siblings, 0 replies; 11+ messages in thread
From: SeongJae Park @ 2021-02-02 10:00 UTC (permalink / raw)
To: Shakeel Butt
Cc: SeongJae Park, SeongJae Park, Jonathan.Cameron, Andrea Arcangeli,
acme, alexander.shishkin, amit, benh, brendan.d.gregg,
Brendan Higgins, Qian Cai, Colin Ian King, Jonathan Corbet,
David Hildenbrand, dwmw, Marco Elver, Du, Fan, foersleo,
Greg Thelen, Ian Rogers, jolsa, Kirill A. Shutemov, Mark Rutland,
Mel Gorman, Minchan Kim, Ingo Molnar, namhyung,
Peter Zijlstra (Intel),
Randy Dunlap, Rik van Riel, David Rientjes, Steven Rostedt,
Mike Rapoport, sblbir, Shuah Khan, sj38.park, snu,
Vlastimil Babka, Vladimir Davydov, Yang Shi, Huang Ying,
zgf574564920, linux-damon, Linux MM, linux-doc, LKML
On Mon, 1 Feb 2021 09:37:39 -0800 Shakeel Butt <shakeelb@google.com> wrote:
> On Tue, Dec 15, 2020 at 3:59 AM SeongJae Park <sjpark@amazon.com> wrote:
> >
> > From: SeongJae Park <sjpark@amazon.de>
> >
> > 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'
>
> > 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.
> >
[...]
> >
> > Signed-off-by: SeongJae Park <sjpark@amazon.de>
> > Reviewed-by: Leonard Foerster <foersleo@amazon.de>
> > ---
> > include/linux/damon.h | 3 +
> > mm/damon/Kconfig | 9 ++
> > mm/damon/Makefile | 1 +
> > mm/damon/core.c | 45 ++++++
> > mm/damon/dbgfs.c | 366 ++++++++++++++++++++++++++++++++++++++++++
> > 5 files changed, 424 insertions(+)
> > create mode 100644 mm/damon/dbgfs.c
> >
> > diff --git a/include/linux/damon.h b/include/linux/damon.h
> > index 39b4d6d3ddee..f9e0d4349352 100644
> > --- a/include/linux/damon.h
> > +++ b/include/linux/damon.h
> > @@ -265,9 +265,12 @@ unsigned int damon_nr_regions(struct damon_target *t);
> >
> > struct damon_ctx *damon_new_ctx(enum damon_target_type type);
> > void damon_destroy_ctx(struct damon_ctx *ctx);
> > +int damon_set_targets(struct damon_ctx *ctx,
> > + unsigned long *ids, ssize_t nr_ids);
> > int damon_set_attrs(struct damon_ctx *ctx, unsigned long sample_int,
> > unsigned long aggr_int, unsigned long regions_update_int,
> > unsigned long min_nr_reg, unsigned long max_nr_reg);
> > +int damon_nr_running_ctxs(void);
> >
> > int damon_start(struct damon_ctx **ctxs, int nr_ctxs);
> > int damon_stop(struct damon_ctx **ctxs, int nr_ctxs);
> > diff --git a/mm/damon/Kconfig b/mm/damon/Kconfig
> > index 8ae080c52950..72f1683ba0ee 100644
> > --- a/mm/damon/Kconfig
> > +++ b/mm/damon/Kconfig
> > @@ -21,4 +21,13 @@ config DAMON_VADDR
> > This builds the default data access monitoring primitives for DAMON
> > that works for virtual address spaces.
> >
> > +config DAMON_DBGFS
> > + bool "DAMON debugfs interface"
> > + depends on DAMON_VADDR && DEBUG_FS
> > + help
> > + This builds the debugfs interface for DAMON. The user space admins
> > + can use the interface for arbitrary data access monitoring.
> > +
> > + If unsure, say N.
> > +
> > endmenu
> > diff --git a/mm/damon/Makefile b/mm/damon/Makefile
> > index 6ebbd08aed67..fed4be3bace3 100644
> > --- a/mm/damon/Makefile
> > +++ b/mm/damon/Makefile
> > @@ -2,3 +2,4 @@
> >
> > obj-$(CONFIG_DAMON) := core.o
> > obj-$(CONFIG_DAMON_VADDR) += vaddr.o
> > +obj-$(CONFIG_DAMON_DBGFS) += dbgfs.o
> > diff --git a/mm/damon/core.c b/mm/damon/core.c
> > index 5ca9f79ccbb6..b9575a6bebff 100644
> > --- a/mm/damon/core.c
> > +++ b/mm/damon/core.c
> > @@ -166,6 +166,37 @@ void damon_destroy_ctx(struct damon_ctx *ctx)
> > kfree(ctx);
> > }
> >
> > +/**
> > + * damon_set_targets() - Set monitoring targets.
> > + * @ctx: monitoring context
> > + * @ids: array of target ids
> > + * @nr_ids: number of entries in @ids
> > + *
> > + * This function should not be called while the kdamond is running.
> > + *
> > + * Return: 0 on success, negative error code otherwise.
> > + */
> > +int damon_set_targets(struct damon_ctx *ctx,
> > + unsigned long *ids, ssize_t nr_ids)
> > +{
> > + ssize_t i;
> > + struct damon_target *t, *next;
> > +
> > + damon_for_each_target_safe(t, next, ctx)
> > + damon_destroy_target(t);
>
> You need to put the reference on the target before destroying.
Oops, you're right. I will fix this in the next version.
>
> > +
> > + for (i = 0; i < nr_ids; i++) {
> > + t = damon_new_target(ids[i]);
> > + if (!t) {
> > + pr_err("Failed to alloc damon_target\n");
> > + return -ENOMEM;
> > + }
> > + damon_add_target(ctx, t);
> > + }
> > +
> > + return 0;
> > +}
> > +
> > /**
> > * damon_set_attrs() - Set attributes for the monitoring.
> > * @ctx: monitoring context
> > @@ -206,6 +237,20 @@ int damon_set_attrs(struct damon_ctx *ctx, unsigned long sample_int,
> > return 0;
> > }
> >
> > +/**
> > + * damon_nr_running_ctxs() - Return number of currently running contexts.
> > + */
> > +int damon_nr_running_ctxs(void)
> > +{
> > + int nr_ctxs;
> > +
> > + mutex_lock(&damon_lock);
> > + nr_ctxs = nr_running_ctxs;
> > + mutex_unlock(&damon_lock);
>
> READ_ONCE(nr_running_ctxs) ?
I'd like to keep the code simpler to read, unless this turns out to be a real
performance bottleneck.
>
> > +
> > + return nr_ctxs;
> > +}
> > +
[...]
> > +
> > +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]);
> > + }
> > +
> > + mutex_lock(&ctx->kdamond_lock);
> > + if (ctx->kdamond) {
> > + ret = -EINVAL;
> > + goto unlock_out;
>
> You need to put_pid on the targets array.
Good catch!
>
> > + }
> > +
> > + err = damon_set_targets(ctx, targets, nr_targets);
> > + if (err)
> > + ret = err;
>
> You need to handle the partial failure from damon_set_targets().
My intention is to keep partial success as is.
>
>
> > +unlock_out:
> > + mutex_unlock(&ctx->kdamond_lock);
> > + kfree(targets);
> > +out:
> > + kfree(kbuf);
> > + return ret;
> > +}
[...]
Thanks,
SeongJae Park
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2021-02-02 16:11 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-02 10:29 [PATCH v23 07/15] mm/damon: Implement a debugfs-based user space interface SeongJae Park
2021-02-02 10:29 ` SeongJae Park
2021-02-02 15:07 ` Shakeel Butt
2021-02-02 15:07 ` Shakeel Butt
2021-02-02 15:45 ` SeongJae Park
2021-02-02 16:08 ` Shakeel Butt
2021-02-02 16:08 ` Shakeel Butt
-- strict thread matches above, loose matches on Subject: below --
2020-12-15 11:54 [PATCH v23 00/15] Introduce Data Access MONitor (DAMON) SeongJae Park
2020-12-15 11:54 ` [PATCH v23 07/15] mm/damon: Implement a debugfs-based user space interface SeongJae Park
2021-02-01 17:37 ` Shakeel Butt
2021-02-01 17:37 ` Shakeel Butt
2021-02-02 10:00 ` SeongJae Park
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.