mm/damon: refactor damon_{start,stop,commit}() for simple error handling
Subsystems (2)
SJ Park
maintainercover mm/damon: refactor damon_{start,stop,commit}() for simple error handling
damon_start() and damon_stop() could leave a few of requested DAMON contexts running when they return an error. damon_commit() failure stops the DAMON context, but in an asynchronous way. Callers should stop the left-over DAMON contexts. It is easy to make mistakes, and indeed a few bugs from such mistakes were found and fixed.
Refactor the core API functions to guarantee contexts are completely stopped under failures. Remove the caller side error handlers that are no longer needed due to the refactoring.
Patches Sequence
================
Patch 1 refactors damon_start() to ensure all contexts are stopped for failures. Patch 2 updates unnecessary damon_start() error handling from mtier sample module.
Patch 3 refactors damon_stop() to always success. Patches 4-6 updates callers (damon_{sysfs,reclaim lru_sort}) to ignore the return value. Patch 7 update damon_stop() return value to void. Patch 8 simplifies damon_stop() error handling in mtier sample module.
Patch 9 refactors damon_call() to return errors only after the context is completely stopped. Patches 10 and 11 remove unnecessary error handlings from callers (wsse and prcl sample modules).
Changelog
=========
Changes from RFC v1
- RFC v1: https://lore.kernel.org/[email protected]
- Fix ABBA deadlock in damon_start().
- Wait instead of doing racy-stop in damon_call().
SJ Park (11):
mm/damon/core: stop ctxs in damon_start() before returning an error samples/damon/mtier: do not stop first context for damon_start()
failure
mm/damon/core: make damon_stop() never fails mm/damon/sysfs: ignore damon_stop() return value mm/damon/reclaaim: ignore damon_stop() return value mm/damon/lru_sort: ignore damon_stop() return value mm/damon/core: change damon_stop() return type to void samples/damon/mtier: stop all contexts with single damon_stop() call mm/damon/core: wait ctx stop in damon_call() before reruning an error samples/damon/wsse: do not stop ctx for damon_call() failure samples/damon/prcl: do not stop DAMON for damon_call() failure
include/linux/damon.h | 2 +- mm/damon/core.c | 37 +++++++++++++++++++------------------ mm/damon/lru_sort.c | 6 ++++-- mm/damon/reclaim.c | 6 ++++-- mm/damon/sysfs.c | 3 ++- samples/damon/mtier.c | 5 +---- samples/damon/prcl.c | 4 +--- samples/damon/wsse.c | 4 +--- 8 files changed, 33 insertions(+), 34 deletions(-)
base-commit: abe5b65a9c676375c3f118d453037f2f9ebbb7ac
Signature · 1 line
2.47.3
SJ Park
maintainerRFC PATCH 1/11 mm/damon/core: stop ctxs in damon_start() before returning an error
When multiple contexts are passed to damon_start(), the function starts the contexts one by one. If any of the operations fails, it immediately returns an error. Contexts that successfully started before the failure keep running. The caller should catch this and stop the contexts. It is complicated and easy to make mistakes. Stop all contexts in damon_start() under the failures.
--- mm/damon/core.c | 3 +++ 1 file changed, 3 insertions(+)
mm/damon/core.c +3 -0
@@ -1917,6 +1917,9 @@ int damon_start(struct damon_ctx **ctxs, int nr_ctxs, bool exclusive) running_exclusive_ctxs = true;mutex_unlock (&damon_lock); +if (i != nr_ctxs) +damon_stop (ctxs, i); +return err; }
Signature · 1 line
2.47.3
SJ Park
maintainerRFC PATCH 2/11 samples/damon/mtier: do not stop first context for damon_start() failure
In reply to SJ Parkdamon_start() was able to fail while keeping the first context running. mtier hence stops the first context in the case. damon_start() is refactored to avoid that. The error handling is no longer necessary. Remove it.
--- samples/damon/mtier.c | 2 -- 1 file changed, 2 deletions(-)
samples/damon/mtier.c +0 -2
@@ -193,8 +193,6 @@ static int damon_sample_mtier_start(void)if (!err)return 0 ; -if (damon_is_running (ctxs[0 ])) -damon_stop (ctxs,1 );damon_destroy_ctx (ctxs[0 ]);damon_destroy_ctx (ctxs[1 ]);return err;
Signature · 1 line
2.47.3
SJ Park
maintainerRFC PATCH 3/11 mm/damon/core: make damon_stop() never fails
In reply to SJ Parkdamon_stop() has no reason to fail. It returns an error code only for possible future changes that can make it fail. Such a change has not been made yet, and this only makes the error handling complicated and confusing. Ensure it returns no error.
--- mm/damon/core.c | 21 ++++++--------------- 1 file changed, 6 insertions(+), 15 deletions(-)
mm/damon/core.c +6 -15
@@ -1926,10 +1926,8 @@ int damon_start(struct damon_ctx **ctxs, int nr_ctxs, bool exclusive)/* * __damon_stop() - Stops monitoring of a given context. * @ctx: monitoring context -* -* Return: 0 on success, negative error code otherwise. */ -static int __damon_stop (struct damon_ctx *ctx) +static void __damon_stop (struct damon_ctx *ctx) {struct task_struct *tsk;
@@ -1939,31 +1937,24 @@ static int __damon_stop(struct damon_ctx *ctx)get_task_struct (tsk);mutex_unlock (&ctx->kdamond_lock);kthread_stop_put (tsk); -return 0 ; +return ; }mutex_unlock (&ctx->kdamond_lock); - -return -EPERM ; }/** * damon_stop() - Stops the monitorings for a given group of contexts. * @ctxs: an array of the pointers for contexts to stop monitoring * @nr_ctxs: size of @ctxs -* -* Return: 0 on success, negative error code otherwise. */ int damon_stop (struct damon_ctx **ctxs,int nr_ctxs) { -int i, err =0 ; +int i; -for (i =0 ; i < nr_ctxs; i++) { +for (i =0 ; i < nr_ctxs; i++)/* nr_running_ctxs is decremented in kdamond_fn */ - err =__damon_stop (ctxs[i]); -if (err) -break ; - } -return err; +__damon_stop (ctxs[i]); +return 0 ; }/**
Signature · 1 line
2.47.3
SJ Park
maintainerRFC PATCH 4/11 mm/damon/sysfs: ignore damon_stop() return value
In reply to SJ Parkdamon_stop() return value is guaranteed to be 0. Ignore it.
--- mm/damon/sysfs.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
mm/damon/sysfs.c +2 -1
@@ -2262,12 +2262,13 @@ static int damon_sysfs_turn_damon_off(struct damon_sysfs_kdamond *kdamond) {if (!kdamond->damon_ctx)return -EINVAL ; -return damon_stop (&kdamond->damon_ctx,1 ); +damon_stop (&kdamond->damon_ctx,1 );/* * To allow users show final monitoring results of already turned-off * DAMON, we free kdamond->damon_ctx in next * damon_sysfs_turn_damon_on(), or kdamonds_nr_store() */ +return 0 ; } static int damon_sysfs_damon_call(int (*fn)(void *data),
Signature · 1 line
2.47.3
SJ Park
maintainerRFC PATCH 5/11 mm/damon/reclaaim: ignore damon_stop() return value
In reply to SJ Parkdamon_stop() return value is guaranteed to be 0. Ignore it.
--- mm/damon/reclaim.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
mm/damon/reclaim.c +4 -2
@@ -332,8 +332,10 @@ static int damon_reclaim_turn(bool on) {int err; -if (!on) -return damon_stop (&ctx,1 ); +if (!on) { +damon_stop (&ctx,1 ); +return 0 ; + } err =damon_reclaim_apply_parameters ();if (err)
Signature · 1 line
2.47.3
SJ Park
maintainerRFC PATCH 6/11 mm/damon/lru_sort: ignore damon_stop() return value
In reply to SJ Parkdamon_stop() return value is guaranteed to be 0. Ignore it.
--- mm/damon/lru_sort.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
mm/damon/lru_sort.c +4 -2
@@ -409,8 +409,10 @@ static int damon_lru_sort_turn(bool on) {int err; -if (!on) -return damon_stop (&ctx,1 ); +if (!on) { +damon_stop (&ctx,1 ); +return 0 ; + } err =damon_lru_sort_apply_parameters ();if (err)
Signature · 1 line
2.47.3
SJ Park
maintainerRFC PATCH 7/11 mm/damon/core: change damon_stop() return type to void
In reply to SJ Parkdamon_stop() always returns 0, and nobody cares. Change the return type to void.
--- include/linux/damon.h | 2 +- mm/damon/core.c | 3 +-- 2 files changed, 2 insertions(+), 3 deletions(-)
include/linux/damon.h +1 -1
@@ -1070,7 +1070,7 @@ static inline unsigned int damon_max_nr_accesses(const struct damon_attrs *attrsbool damon_initialized (void );int damon_start (struct damon_ctx **ctxs,int nr_ctxs,bool exclusive); -int damon_stop (struct damon_ctx **ctxs,int nr_ctxs); +void damon_stop (struct damon_ctx **ctxs,int nr_ctxs);bool damon_is_running (struct damon_ctx *ctx);int damon_kdamond_pid (struct damon_ctx *ctx);
mm/damon/core.c +1 -2
@@ -1947,14 +1947,13 @@ static void __damon_stop(struct damon_ctx *ctx)* @ctxs: an array of the pointers for contexts to stop monitoring * @nr_ctxs: size of @ctxs */ -int damon_stop (struct damon_ctx **ctxs,int nr_ctxs) +void damon_stop (struct damon_ctx **ctxs,int nr_ctxs) {int i;for (i =0 ; i < nr_ctxs; i++)/* nr_running_ctxs is decremented in kdamond_fn */ __damon_stop (ctxs[i]); -return 0 ; }/**
Signature · 1 line
2.47.3
SJ Park
maintainerRFC PATCH 8/11 samples/damon/mtier: stop all contexts with single damon_stop() call
In reply to SJ Parkdamon_stop() was theoretically able to return failure while keeping the second context for mtier running. mtier stops its contexts one by one with two damon_stop() call for the reason. damon_stop() is refactored to always successfully stop all requested DAMON contexts. The two calls are unnecessary. Use only single damon_stop() call for all contexts.
--- samples/damon/mtier.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)
samples/damon/mtier.c +1 -2
@@ -200,8 +200,7 @@ static int damon_sample_mtier_start(void)static void damon_sample_mtier_stop (void ) { -damon_stop (ctxs,1 ); -damon_stop (&ctxs[1 ],1 ); +damon_stop (ctxs,2 );damon_destroy_ctx (ctxs[0 ]);damon_destroy_ctx (ctxs[1 ]); }
Signature · 1 line
2.47.3
SJ Park
maintainerRFC PATCH 9/11 mm/damon/core: wait ctx stop in damon_call() before reruning an error
In reply to SJ Parkdamon_call() failure means the DAMON context started its termination. The termination is asynchronously done in kdamond thread. The caller's error handling should handle the race, too. It is complicated and easy to make mistakes.
Update damon_call() to ensure the context is stopped in the case, by waiting until the completion is confirmed.
--- mm/damon/core.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
mm/damon/core.c +10 -2
@@ -2010,6 +2010,8 @@ int damon_kdamond_pid(struct damon_ctx *ctx)* @ctx has succeeded. Otherwise, this function could fall into an indefinite * wait. * +* When this function is failed, the @ctx is guaranteed to be stopped. +* * Return: 0 on success, negative error code otherwise. */ int damon_call (struct damon_ctx *ctx,struct damon_call_control *control)
@@ -2022,7 +2024,7 @@ int damon_call(struct damon_ctx *ctx, struct damon_call_control *control)mutex_lock (&ctx->call_controls_lock);if (ctx->call_controls_obsolete) {mutex_unlock (&ctx->call_controls_lock); -return -ECANCELED ; + goto canceled; }list_add_tail (&control->list, &ctx->call_controls);mutex_unlock (&ctx->call_controls_lock);
@@ -2030,8 +2032,14 @@ int damon_call(struct damon_ctx *ctx, struct damon_call_control *control)return 0 ;wait_for_completion (&control->completion);if (control->canceled) -return -ECANCELED ; + goto canceled;return 0 ; + +canceled: +while (damon_is_running (ctx)) +schedule_timeout_idle (msecs_to_jiffies (100 )); +return -ECANCELED ; + }/**
Signature · 1 line
2.47.3
SJ Park
maintainerRFC PATCH 10/11 samples/damon/wsse: do not stop ctx for damon_call() failure
In reply to SJ Parkdamon_call() failure was causing the context to be stopped, but
asynchronously by the kdamond thread. To handle the race, the caller
had to stop the context. damon_call() is updated to do the stop on its own. Remove the damon_stop() call from the damon_call() error handling.
--- samples/damon/wsse.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
samples/damon/wsse.c +1 -3
@@ -93,10 +93,8 @@ static int damon_sample_wsse_start(void) } repeat_call_control.data = ctx; err =damon_call (ctx, &repeat_call_control); -if (err) { -damon_stop (&ctx,1 ); +if (err)damon_destroy_ctx (ctx); - }return err; }
Signature · 1 line
2.47.3
SJ Park
maintainerRFC PATCH 11/11 samples/damon/prcl: do not stop DAMON for damon_call() failure
In reply to SJ Parkdamon_call() failure was causing the context to be stopped, but
asynchronously by the kdamond thread. To handle the race, the caller
had to stop the context. damon_call() is updated to do the stop on its own. Remove the damon_stop() call from the damon_call() error handling.
--- samples/damon/prcl.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
samples/damon/prcl.c +1 -3
@@ -113,10 +113,8 @@ static int damon_sample_prcl_start(void) repeat_call_control.data = ctx; err =damon_call (ctx, &repeat_call_control); -if (err) { -damon_stop (&ctx,1 ); +if (err)damon_destroy_ctx (ctx); - }return err; }
Signature · 1 line
2.47.3