[RFC PATCH 0/2] mm/damon/stat: add kdamond_pid parameter

[RFC PATCH 0/2] mm/damon/stat: add kdamond_pid parameter

[SeongJae Park]

DAMON_STAT doesn't provide the pid of its kdamond, unlike DAMON_RECLAIM
and DAMON_LRU_SORT.  This makes user-space management of DAMON_STAT
unnecessarily complicated.  Provide the information via a new parameter,
namely kdamond_pid, and document it.

SeongJae Park (2):
  mm/damon/stat: add a parameter for reading kdamond pid
  Docs/admin-guide/mm/damon/stat: document kdamond_pid parameter

 Documentation/admin-guide/mm/damon/stat.rst |  7 +++++++
 mm/damon/stat.c                             | 13 +++++++++++++
 2 files changed, 20 insertions(+)


base-commit: 5262d0a487d50faeefbca2e4ebbdfbed3f69426e
-- 
2.47.3

[RFC PATCH 1/2] mm/damon/stat: add a parameter for reading kdamond pid

[SeongJae Park]

Knowing the pid of the kdamonds can help user-space management including
monitoring of DAMON's system resource consumption.  To make it easier,
DAMON_SYSFS, DAMON_RECLAIM and DAMON_LRU_SORT provide the pid
information.  DAMON_STAT is not providing it, though.  Expose the pid of
DAMON_STAT kdamond via a new read-only module parameter, namely
kdamond_pid.  This also makes DAMON modules usage more standardized,
because DAMON_RECLAIM and DAMON_LRU_SORT also provide the information
via their read-only parameters of the same name.

Signed-off-by: SeongJae Park <sj@kernel.org>
---
 mm/damon/stat.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/mm/damon/stat.c b/mm/damon/stat.c
index 99ba346f9e325..21fa04d95eeac 100644
--- a/mm/damon/stat.c
+++ b/mm/damon/stat.c
@@ -44,6 +44,15 @@ module_param(aggr_interval_us, ulong, 0400);
 MODULE_PARM_DESC(aggr_interval_us,
 		"Current tuned aggregation interval in microseconds");
 
+/*
+ * PID of the DAMON thread
+ *
+ * If DAMON_STAT is enabled, this becomes the PID of the worker thread.
+ * Else, -1.
+ */
+static int kdamond_pid __read_mostly = -1;
+module_param(kdamond_pid, int, 0400);
+
 static struct damon_ctx *damon_stat_context;
 
 static unsigned long damon_stat_last_refresh_jiffies;
@@ -260,6 +269,9 @@ static int damon_stat_start(void)
 		damon_stat_context = NULL;
 		return err;
 	}
+	kdamond_pid = damon_kdamond_pid(damon_stat_context);
+	if (kdamond_pid < 0)
+		return kdamond_pid;
 
 	damon_stat_last_refresh_jiffies = jiffies;
 	call_control.data = damon_stat_context;
@@ -269,6 +281,7 @@ static int damon_stat_start(void)
 static void damon_stat_stop(void)
 {
 	damon_stop(&damon_stat_context, 1);
+	kdamond_pid = -1;
 	damon_destroy_ctx(damon_stat_context);
 	damon_stat_context = NULL;
 }
-- 
2.47.3

Re: (sashiko review) [RFC PATCH 1/2] mm/damon/stat: add a parameter for reading kdamond pid

[SeongJae Park]

TL; DR: Sashiko found a bug of this patch; I will fix it in the next revision.
Sashiko also found one issue in the existing code; I will work on it
separately.

Forwarding full sashiko review in a reply format with my inline comments below,
for sharing details of my view and doing followup discussions via mails if
needed.

> # review url: https://sashiko.dev/#/patchset/20260414045002.88110-2-sj@kernel.org
> # start of sashiko.dev inline review
> commit fc6c9ec12df0df57fcce5c90c7a0604cddb1cc78
> Author: SeongJae Park <sj@kernel.org>
> 
> mm/damon/stat: add a parameter for reading kdamond pid
> 
> This commit exposes the PID of the DAMON_STAT kdamond via a new read-only
> module parameter, kdamond_pid. It intends to set this to -1 when DAMON_STAT
> is disabled.
> 
> > diff --git a/mm/damon/stat.c b/mm/damon/stat.c
> > index 99ba346f9e325..21fa04d95eeac 100644
> > --- a/mm/damon/stat.c
> > +++ b/mm/damon/stat.c
> [ ... ]
> > @@ -260,6 +269,9 @@ static int damon_stat_start(void)
> >  		damon_stat_context = NULL;
> >  		return err;
> >  	}
> > +	kdamond_pid = damon_kdamond_pid(damon_stat_context);
> > +	if (kdamond_pid < 0)
> > +		return kdamond_pid;
> 
> If damon_kdamond_pid() returns an error code, doesn't this leave the global
> kdamond_pid set to that negative error code instead of -1?
> 
> Also, if we take this early return path, does this code leak the
> damon_stat_context allocation? The error path just above it correctly
> calls damon_destroy_ctx(damon_stat_context) and sets it to NULL.

Nice catch.  I will fix this like below in the next revision.

'''
--- a/mm/damon/stat.c
+++ b/mm/damon/stat.c
@@ -223,8 +223,12 @@ static int damon_stat_start(void)
                return err;
        }
        kdamond_pid = damon_kdamond_pid(damon_stat_context);
-       if (kdamond_pid < 0)
-               return kdamond_pid;
+       if (kdamond_pid < 0) {
+               err = kdamond_pid;
+               kdamond_pid = -1;
+               damon_destroy_ctx(damon_stat_context);
+               damon_stat_context = NULL;
+               return err;

        damon_stat_last_refresh_jiffies = jiffies;
        call_control.data = damon_stat_context;
'''

> 
> >  
> >  	damon_stat_last_refresh_jiffies = jiffies;
> >  	call_control.data = damon_stat_context;
> >  	return damon_call(damon_stat_context, &call_control);
> >  }
> 
> If damon_call() fails here and returns an error, the caller will handle it
> by setting enabled to false. Does this leave kdamond_pid holding a stale
> positive PID instead of being reset to -1?

Nice catch, but not this patch's fault.  I will separately work on it.

> 
> > @@ -269,6 +281,7 @@ static int damon_stat_start(void)
> >  static void damon_stat_stop(void)
> >  {
> >  	damon_stop(&damon_stat_context, 1);
> > +	kdamond_pid = -1;
> >  	damon_destroy_ctx(damon_stat_context);
> >  	damon_stat_context = NULL;
> >  }
> 
> 
> # end of sashiko.dev inline review
> # review url: https://sashiko.dev/#/patchset/20260414045002.88110-2-sj@kernel.org


Thanks,
SJ

# hkml [1] generated a draft of this mail.  You can regenerate
# this using below command:
#
#     hkml patch sashiko_dev --for_forwarding \
#             20260414045002.88110-2-sj@kernel.org
#
# [1] https://github.com/sjp38/hackermail

[RFC PATCH 2/2] Docs/admin-guide/mm/damon/stat: document kdamond_pid parameter

[SeongJae Park]

Update DAMON_STAT usage document for newly added kdamond_pid parameter.

Signed-off-by: SeongJae Park <sj@kernel.org>
---
 Documentation/admin-guide/mm/damon/stat.rst | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/Documentation/admin-guide/mm/damon/stat.rst b/Documentation/admin-guide/mm/damon/stat.rst
index c4b14daeb2dd6..46c5dd96aa2ed 100644
--- a/Documentation/admin-guide/mm/damon/stat.rst
+++ b/Documentation/admin-guide/mm/damon/stat.rst
@@ -89,3 +89,10 @@ percentiles of the idle time values via this read-only parameter.  Reading the
 parameter returns 101 idle time values in milliseconds, separated by comma.
 Each value represents 0-th, 1st, 2nd, 3rd, ..., 99th and 100th percentile idle
 times.
+
+kdamond_pid
+-----------
+
+PID of the DAMON thread.
+
+If DAMON_STAT is enabled, this becomes the PID of the worker thread.  Else, -1.
-- 
2.47.3