Re: [PATCH v31 07/13] mm/damon: Implement a debugfs-based user space interface

[SeongJae Park <sj38.park@gmail.com>]

From: SeongJae Park <sjpark@amazon.de>

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

> On Mon, Jun 21, 2021 at 1:31 AM SeongJae Park <sj38.park@gmail.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.
> > 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>
> > Reviewed-by: Fernand Sieber <sieberf@amazon.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