Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   SeongJae Park <sj38.park@gmail.com>
To:     Shakeel Butt <shakeelb@google.com>
Cc:     SeongJae Park <sj38.park@gmail.com>,
        SeongJae Park <sjpark@amazon.de>, Jonathan.Cameron@huawei.com,
        acme@kernel.org, alexander.shishkin@linux.intel.com,
        amit@kernel.org, benh@kernel.crashing.org,
        Brendan Higgins <brendanhiggins@google.com>,
        Jonathan Corbet <corbet@lwn.net>,
        David Hildenbrand <david@redhat.com>, dwmw@amazon.com,
        Marco Elver <elver@google.com>, "Du, Fan" <fan.du@intel.com>,
        foersleo@amazon.de, greg@kroah.com,
        Greg Thelen <gthelen@google.com>, guoju.fgj@alibaba-inc.com,
        jgowans@amazon.com, Mel Gorman <mgorman@suse.de>, mheyne@amazon.de,
        Minchan Kim <minchan@kernel.org>,
        Ingo Molnar <mingo@redhat.com>, namhyung@kernel.org,
        "Peter Zijlstra (Intel)" <peterz@infradead.org>,
        Rik van Riel <riel@surriel.com>,
        David Rientjes <rientjes@google.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Mike Rapoport <rppt@kernel.org>, Shuah Khan <shuah@kernel.org>,
        sieberf@amazon.com, snu@zelle79.org,
        Vlastimil Babka <vbabka@suse.cz>,
        Vladimir Davydov <vdavydov.dev@gmail.com>,
        zgf574564920@gmail.com, linux-damon@amazon.com,
        Linux MM <linux-mm@kvack.org>, linux-doc@vger.kernel.org,
        LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v31 07/13] mm/damon: Implement a debugfs-based user space interface
Date:   Thu, 24 Jun 2021 10:26:22 +0000
Message-Id: <20210624102623.24563-4-sjpark@amazon.de>
In-Reply-To: <20210624102623.24563-1-sjpark@amazon.de>
References: <20210624102623.24563-1-sjpark@amazon.de>
In-Reply-To: <CALvZod61B66+Z4J1n6DG6UnvY9j8TE0diz=o1XXOBSZ5DaKSDQ@mail.gmail.com>
Precedence: bulk

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