Message-Id: <20210624102623.24563-4-sjpark@amazon.de>
Date:   Thu, 24 Jun 2021 10:26:22 +0000
From:   SeongJae Park <sj38.park@...il.com>
To:     Shakeel Butt <shakeelb@...gle.com>
Cc:     SeongJae Park <sj38.park@...il.com>,
        SeongJae Park <sjpark@...zon.de>, Jonathan.Cameron@...wei.com,
        acme@...nel.org, alexander.shishkin@...ux.intel.com,
        amit@...nel.org, benh@...nel.crashing.org,
        Brendan Higgins <brendanhiggins@...gle.com>,
        Jonathan Corbet <corbet@....net>,
        David Hildenbrand <david@...hat.com>, dwmw@...zon.com,
        Marco Elver <elver@...gle.com>, "Du, Fan" <fan.du@...el.com>,
        foersleo@...zon.de, greg@...ah.com,
        Greg Thelen <gthelen@...gle.com>, guoju.fgj@...baba-inc.com,
        jgowans@...zon.com, Mel Gorman <mgorman@...e.de>, mheyne@...zon.de,
        Minchan Kim <minchan@...nel.org>,
        Ingo Molnar <mingo@...hat.com>, namhyung@...nel.org,
        "Peter Zijlstra (Intel)" <peterz@...radead.org>,
        Rik van Riel <riel@...riel.com>,
        David Rientjes <rientjes@...gle.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Mike Rapoport <rppt@...nel.org>, Shuah Khan <shuah@...nel.org>,
        sieberf@...zon.com, snu@...le79.org,
        Vlastimil Babka <vbabka@...e.cz>,
        Vladimir Davydov <vdavydov.dev@...il.com>,
        zgf574564920@...il.com, linux-damon@...zon.com,
        Linux MM <linux-mm@...ck.org>, linux-doc@...r.kernel.org,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v31 07/13] mm/damon: Implement a debugfs-based user space interface

From: SeongJae Park <sjpark@...zon.de>

On Tue, 22 Jun 2021 11:12:36 -0700 Shakeel Butt <shakeelb@...gle.com> wrote:

> On Mon, Jun 21, 2021 at 1:31 AM SeongJae Park <sj38.park@...il.com> wrote:
> >
> > From: SeongJae Park <sjpark@...zon.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@...zon.de>
> > Reviewed-by: Leonard Foerster <foersleo@...zon.de>
> > Reviewed-by: Fernand Sieber <sieberf@...zon.com>
> 
> 
> The high level comment I have for this patch is the layering of pid
> reference counting. The dbgfs should treat the targets as abstract
> objects and vaddr should handle the reference counting of pids. More
> specifically move find_get_pid from dbgfs to vaddr and to add an
> interface to the primitive for set_targets.
> 
> At the moment, the pid reference is taken in dbgfs and put in vaddr.
> This will be the source of bugs in future.

Good point, and agreed on the problem.  But, I'd like to move 'put_pid()' to
dbgfs, because I think that would let extending the dbgfs user interface to
pidfd a little bit simpler.  Also, I think that would be easier to use for
in-kernel programming interface usages.  If you disagree, please feel free to
let me know.


Thanks,
SeongJae Park

Powered by blists - more mailing lists