Skip to content

Commit c3fb156

Browse files
sjp38gregkh
authored andcommitted
mm/damon/lru_sort: detect and use fresh enabled and kdamond_pid values
commit b98b7ff upstream. DAMON_LRU_SORT updates 'enabled' and 'kdamond_pid' parameter values, which represents the running status of its kdamond, when the user explicitly requests start/stop of the kdamond. The kdamond can, however, be stopped in events other than the explicit user request in the following three events. 1. ctx->regions_score_histogram allocation failure at beginning of the execution, 2. damon_commit_ctx() failure due to invalid user input, and 3. damon_commit_ctx() failure due to its internal allocation failures. Hence, if the kdamond is stopped by the above three events, the values of the status parameters can be stale. Users could show the stale values and be confused. This is already bad, but the real consequence is worse. DAMON_LRU_SORT avoids unnecessary damon_start() and damon_stop() calls based on the 'enabled' parameter value. And the update of 'enabled' parameter value depends on the damon_start() and damon_stop() call results. Hence, once the kdamond has stopped by the unintentional events, the user cannot restart the kdamond before the system reboot. For example, the issue can be reproduced via below steps. # cd /sys/module/damon_lru_sort/parameters # # # start DAMON_LRU_SORT # echo Y > enabled # ps -ef | grep kdamond root 806 2 0 17:53 ? 00:00:00 [kdamond.0] root 808 803 0 17:53 pts/4 00:00:00 grep kdamond # # # commit wrong input to stop kdamond withou explicit stop request # echo 3 > addr_unit # echo Y > commit_inputs bash: echo: write error: Invalid argument # # # confirm kdamond is stopped # ps -ef | grep kdamond root 811 803 0 17:53 pts/4 00:00:00 grep kdamond # # # users casn now show stable status # cat enabled Y # cat kdamond_pid 806 # # # even after fixing the wrong parameter, # # kdamond cannot be restarted. # echo 1 > addr_unit # echo Y > enabled # ps -ef | grep kdamond root 815 803 0 17:54 pts/4 00:00:00 grep kdamond The problem will only rarely happen in real and common setups for the following reasons. The allocation failures are unlikely in such setups since those allocations are arguably too small to fail. Also sane users on real production environments may not commit wrong input parameters. But once it happens, the consequence is quite bad. And the bug is a bug. The issue stems from the fact that there are multiple events that can change the status, and following all the events is challenging. Dynamically detect and use the fresh status for the parameters when those are requested. Link: https://lore.kernel.org/20260419161003.79176-3-sj@kernel.org Fixes: 40e983c ("mm/damon: introduce DAMON-based LRU-lists Sorting") Co-developed-by: Liew Rui Yan <aethernet65535@gmail.com> Signed-off-by: Liew Rui Yan <aethernet65535@gmail.com> Signed-off-by: SeongJae Park <sj@kernel.org> Cc: <stable@vger.kernel.org> # 6.0.x Signed-off-by: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
1 parent 4b6b06a commit c3fb156

1 file changed

Lines changed: 55 additions & 30 deletions

File tree

mm/damon/lru_sort.c

Lines changed: 55 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -161,15 +161,6 @@ module_param(monitor_region_end, ulong, 0600);
161161
*/
162162
static unsigned long addr_unit __read_mostly = 1;
163163

164-
/*
165-
* PID of the DAMON thread
166-
*
167-
* If DAMON_LRU_SORT is enabled, this becomes the PID of the worker thread.
168-
* Else, -1.
169-
*/
170-
static int kdamond_pid __read_mostly = -1;
171-
module_param(kdamond_pid, int, 0400);
172-
173164
static struct damos_stat damon_lru_sort_hot_stat;
174165
DEFINE_DAMON_MODULES_DAMOS_STATS_PARAMS(damon_lru_sort_hot_stat,
175166
lru_sort_tried_hot_regions, lru_sorted_hot_regions,
@@ -391,12 +382,8 @@ static int damon_lru_sort_turn(bool on)
391382
{
392383
int err;
393384

394-
if (!on) {
395-
err = damon_stop(&ctx, 1);
396-
if (!err)
397-
kdamond_pid = -1;
398-
return err;
399-
}
385+
if (!on)
386+
return damon_stop(&ctx, 1);
400387

401388
err = damon_lru_sort_apply_parameters();
402389
if (err)
@@ -405,9 +392,6 @@ static int damon_lru_sort_turn(bool on)
405392
err = damon_start(&ctx, 1, true);
406393
if (err)
407394
return err;
408-
kdamond_pid = damon_kdamond_pid(ctx);
409-
if (kdamond_pid < 0)
410-
return kdamond_pid;
411395
return damon_call(ctx, &call_control);
412396
}
413397

@@ -435,42 +419,83 @@ module_param_cb(addr_unit, &addr_unit_param_ops, &addr_unit, 0600);
435419
MODULE_PARM_DESC(addr_unit,
436420
"Scale factor for DAMON_LRU_SORT to ops address conversion (default: 1)");
437421

422+
static bool damon_lru_sort_enabled(void)
423+
{
424+
if (!ctx)
425+
return false;
426+
return damon_is_running(ctx);
427+
}
428+
438429
static int damon_lru_sort_enabled_store(const char *val,
439430
const struct kernel_param *kp)
440431
{
441-
bool is_enabled = enabled;
442-
bool enable;
443432
int err;
444433

445-
err = kstrtobool(val, &enable);
434+
err = kstrtobool(val, &enabled);
446435
if (err)
447436
return err;
448437

449-
if (is_enabled == enable)
438+
if (damon_lru_sort_enabled() == enabled)
450439
return 0;
451440

452441
/* Called before init function. The function will handle this. */
453442
if (!damon_initialized())
454-
goto set_param_out;
443+
return 0;
455444

456-
err = damon_lru_sort_turn(enable);
457-
if (err)
458-
return err;
445+
return damon_lru_sort_turn(enabled);
446+
}
459447

460-
set_param_out:
461-
enabled = enable;
462-
return err;
448+
static int damon_lru_sort_enabled_load(char *buffer,
449+
const struct kernel_param *kp)
450+
{
451+
return sprintf(buffer, "%c\n", damon_lru_sort_enabled() ? 'Y' : 'N');
463452
}
464453

465454
static const struct kernel_param_ops enabled_param_ops = {
466455
.set = damon_lru_sort_enabled_store,
467-
.get = param_get_bool,
456+
.get = damon_lru_sort_enabled_load,
468457
};
469458

470459
module_param_cb(enabled, &enabled_param_ops, &enabled, 0600);
471460
MODULE_PARM_DESC(enabled,
472461
"Enable or disable DAMON_LRU_SORT (default: disabled)");
473462

463+
static int damon_lru_sort_kdamond_pid_store(const char *val,
464+
const struct kernel_param *kp)
465+
{
466+
/*
467+
* kdamond_pid is read-only, but kernel command line could write it.
468+
* Do nothing here.
469+
*/
470+
return 0;
471+
}
472+
473+
static int damon_lru_sort_kdamond_pid_load(char *buffer,
474+
const struct kernel_param *kp)
475+
{
476+
int kdamond_pid = -1;
477+
478+
if (ctx) {
479+
kdamond_pid = damon_kdamond_pid(ctx);
480+
if (kdamond_pid < 0)
481+
kdamond_pid = -1;
482+
}
483+
return sprintf(buffer, "%d\n", kdamond_pid);
484+
}
485+
486+
static const struct kernel_param_ops kdamond_pid_param_ops = {
487+
.set = damon_lru_sort_kdamond_pid_store,
488+
.get = damon_lru_sort_kdamond_pid_load,
489+
};
490+
491+
/*
492+
* PID of the DAMON thread
493+
*
494+
* If DAMON_LRU_SORT is enabled, this becomes the PID of the worker thread.
495+
* Else, -1.
496+
*/
497+
module_param_cb(kdamond_pid, &kdamond_pid_param_ops, NULL, 0400);
498+
474499
static int __init damon_lru_sort_init(void)
475500
{
476501
int err;

0 commit comments

Comments
 (0)