mm/damon/stat: add kdamond_pid parameter
Subsystems (4)
SeongJae Park
maintainercover mm/damon/stat: add kdamond_pid parameter
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
Signature · 1 line
2.47.3
SeongJae Park
maintainerRFC PATCH 1/2 mm/damon/stat: add a parameter for reading kdamond pid
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.
--- mm/damon/stat.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
mm/damon/stat.c +13 -0
@@ -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 ; }
Signature · 1 line
2.47.3
SeongJae Park
maintainerRe: (sashiko review) [RFC PATCH 1/2] mm/damon/stat: add a parameter for reading kdamond pid
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.
Quoted 27 lines
# review url: https://sashiko.dev/#/patchset/[email protected] # start of sashiko.dev inline review commit fc6c9ec12df0df57fcce5c90c7a0604cddb1cc78 Author: SeongJae Park <[email protected]>
mm/damon/stat: add a parameter for reading kdamond pidThis commit exposes the PID of the
DAMON_STATkdamond via a new read-only module parameter,kdamond_pid. It intends to set this to -1 whenDAMON_STATis disabled.Quoted 4 lines
mm/damon/stat.c +0
-0[ ... ]
mm/damon/stat.c static int damon_stat_start(void) · line 274
Quoted 7 lines
mm/damon/stat.c +3
-0@@ -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 globalkdamond_pidset to that negative error code instead of -1?
Quoted 3 lines
Also, if we take this early return path, does this code leak the
damon_stat_contextallocation? The error path just above it correctly callsdamon_destroy_ctx(damon_stat_context)and sets it to NULL.
Nice catch. I will fix this like below in the next revision.
'''
mm/damon/stat.c +6 -2
@@ -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;
'''
Quoted 10 lines
Quoted 6 lines
mm/damon/stat.c +0
-0damon_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 leavekdamond_pidholding a stale positive PID instead of being reset to -1?
Nice catch, but not this patch's fault. I will separately work on it.
Quoted 13 lines
mm/damon/stat.c static int damon_stat_start(void) · line 287
Quoted 9 lines
mm/damon/stat.c +1
-0@@ -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/[email protected]
Thanks,
SJ
# hkml [1] generated a draft of this mail. You can regenerate # this using below command: # # hkml patch sashiko_dev --for_forwarding \ # [email protected] # # [1] https://github.com/sjp38/hackermail
SeongJae Park
maintainerRe: (sashiko review) [RFC PATCH 1/2] mm/damon/stat: add a parameter for reading kdamond pid
Quoted 3 lines · On Mon, 13 Apr 2026 22:37:41 -0700 SeongJae Park <[email protected]> wrote:
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.
No, I was wrong.
Quoted 32 lines
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.
Quoted 27 lines
# review url: https://sashiko.dev/#/patchset/[email protected] # start of sashiko.dev inline review commit fc6c9ec12df0df57fcce5c90c7a0604cddb1cc78 Author: SeongJae Park <[email protected]>
mm/damon/stat: add a parameter for reading kdamond pidThis commit exposes the PID of the
DAMON_STATkdamond via a new read-only module parameter,kdamond_pid. It intends to set this to -1 whenDAMON_STATis disabled.Quoted 4 lines
mm/damon/stat.c +0
-0[ ... ]
mm/damon/stat.c static int damon_stat_start(void) · line 274
Quoted 7 lines
mm/damon/stat.c +3
-0@@ -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 globalkdamond_pidset to that negative error code instead of -1?
Quoted 5 lines
Quoted 3 lines
Also, if we take this early return path, does this code leak the
damon_stat_contextallocation? The error path just above it correctly callsdamon_destroy_ctx(damon_stat_context)and sets it to NULL.Nice catch. I will fix this like below in the next revision.
The kdamond_pid stale value question is indeed a nice catch. But the damon_stat_context allocation leak question is not. It will be cleaned up when damon_stat_start() is called again, by the cleanup logic at the beginning of this function. This is not a bug but an intended behavior.
[...]
Quoted 28 lines
'''
mm/damon/stat.c +6
-2@@ -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;'''
Quoted 7 lines
Quoted 6 lines
mm/damon/stat.c +0
-0damon_stat_last_refresh_jiffies = jiffies; call_control.data = damon_stat_context;return damon_call (damon_stat_context, &call_control);}
Quoted 5 lines
Quoted 3 lines
If
damon_call()fails here and returns an error, the caller will handle it by setting enabled to false. Does this leavekdamond_pidholding a stale positive PID instead of being reset to -1?Nice catch, but not this patch's fault. I will separately work on it.
I'm wrong here, too. This is this patch's fault. But, there is actually a bug that pre-existed before this patch. I was somehow confused. That is, the 'enabled' value can also be stale, and it can make DAMON_STAT cannot be restarted before the system is reboot.
I will hold this patch and fix the bug first.
Refer to my comment [1] on the next version of this patch, for more detail.
[1] https://lore.kernel.org/[email protected]
Thanks,
SJ
[...]
SeongJae Park
maintainerRFC PATCH 2/2 Docs/admin-guide/mm/damon/stat: document kdamond_pid parameter
In reply to SeongJae ParkUpdate DAMON_STAT usage document for newly added kdamond_pid parameter.
--- Documentation/admin-guide/mm/damon/stat.rst | 7 +++++++ 1 file changed, 7 insertions(+)
Documentation/admin-guide/mm/damon/stat.rst +7 -0
@@ -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.
Signature · 1 line
2.47.3