From eeb477701d1159b82af04865f239bc0071e6f968 Mon Sep 17 00:00:00 2001 From: Douglas Raillard Date: Mon, 20 Jun 2022 10:45:36 +0100 Subject: [PATCH 1/4] lisa._assets.kmodules: Add missing static in sched_helpers.h FIX Make autogroup_path() static so that the helper can be included in more than one compilation unit. --- lisa/_assets/kmodules/sched_tp/sched_helpers.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lisa/_assets/kmodules/sched_tp/sched_helpers.h b/lisa/_assets/kmodules/sched_tp/sched_helpers.h index 424fbfa61..6305cefcd 100644 --- a/lisa/_assets/kmodules/sched_tp/sched_helpers.h +++ b/lisa/_assets/kmodules/sched_tp/sched_helpers.h @@ -44,7 +44,7 @@ static inline bool task_group_is_autogroup(struct task_group *tg) #endif } -int autogroup_path(struct task_group *tg, char *buf, int buflen) +static int autogroup_path(struct task_group *tg, char *buf, int buflen) { #ifdef CONFIG_SCHED_AUTOGROUP if (!task_group_is_autogroup(tg)) -- GitLab From 2ab4080e027a1e769464b4e6efedb538d2d95926 Mon Sep 17 00:00:00 2001 From: Douglas Raillard Date: Tue, 21 Jun 2022 10:44:09 +0100 Subject: [PATCH 2/4] lisa._assets.kmodules.sched_tp: Fix failed feature dependency FIX When a feature fails to enable, record the return code and serve it again to any other dependent feature when they try to enable it. Previously, later attempts to enable would return 0, and the dependent feature would think the feature is enabled and ready for use. --- lisa/_assets/kmodules/sched_tp/features.c | 4 +++- lisa/_assets/kmodules/sched_tp/features.h | 5 +++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/lisa/_assets/kmodules/sched_tp/features.c b/lisa/_assets/kmodules/sched_tp/features.c index 36b225267..ca60176a0 100644 --- a/lisa/_assets/kmodules/sched_tp/features.c +++ b/lisa/_assets/kmodules/sched_tp/features.c @@ -13,13 +13,15 @@ int __enable_feature(struct feature* feature) { mutex_lock(feature->lock); if (feature->enabled) { - ret = 0; + ret = feature->__enable_ret; } else { pr_info("Enabling lisa feature %s\n", feature->name); if (feature->enable) ret = feature->enable(feature); else ret = 0; + feature->__enable_ret = ret; + if (ret) pr_err("Failed to enable feature %s: %i", feature->name, ret); } diff --git a/lisa/_assets/kmodules/sched_tp/features.h b/lisa/_assets/kmodules/sched_tp/features.h index 973bc7144..b17411f5c 100644 --- a/lisa/_assets/kmodules/sched_tp/features.h +++ b/lisa/_assets/kmodules/sched_tp/features.h @@ -31,6 +31,9 @@ struct feature { int (*enable)(struct feature*); int (*disable)(struct feature*); + /* Return code of the enable() function */ + int __enable_ret; + /* true if the feature has been explicitly enabled by the user */ bool __explicitly_enabled; /* true if the feature is internal, i.e. not exposed to the user. @@ -68,6 +71,7 @@ int __placeholder_deinit(struct feature *feature); .disable = __placeholder_deinit, \ .lock = &__lisa_mutex_feature_##feature_name, \ .__internal = true, \ + .__enable_ret = 0, \ }; #define __DEFINE_FEATURE_STRONG(feature_name, enable_f, disable_f, internal) \ @@ -81,6 +85,7 @@ int __placeholder_deinit(struct feature *feature); .disable = disable_f, \ .lock = &__lisa_mutex_feature_##feature_name, \ .__internal = internal, \ + .__enable_ret = 0, \ }; /** -- GitLab From b04f2ab0a6185ef50b789366c0d1d93e7f887fd8 Mon Sep 17 00:00:00 2001 From: Douglas Raillard Date: Tue, 21 Jun 2022 10:59:46 +0100 Subject: [PATCH 3/4] lisa._assets.kmodules.sched_tp: Attempt to initialize as many features as possible Instead of bailing out at the first feature that fails to enable, enable all the features we can and report the issue at the end. --- lisa/_assets/kmodules/sched_tp/features.c | 14 ++++++-------- lisa/_assets/kmodules/sched_tp/main.c | 3 ++- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/lisa/_assets/kmodules/sched_tp/features.c b/lisa/_assets/kmodules/sched_tp/features.c index ca60176a0..ee253288b 100644 --- a/lisa/_assets/kmodules/sched_tp/features.c +++ b/lisa/_assets/kmodules/sched_tp/features.c @@ -78,9 +78,7 @@ static int __process_features(char **selected, size_t selected_len, feature_proc int ret = 0; for (feature=__lisa_features_start; feature < __lisa_features_stop; feature++) { - ret = __select_feature(feature, selected, selected_len, process); - if (ret) - return ret; + ret |= __select_feature(feature, selected, selected_len, process); } return ret; } @@ -100,6 +98,8 @@ static int __enable_feature_explicitly(struct feature* feature) { } int init_features(char **selected, size_t selected_len) { + BUG_ON(MAX_FEATURES < ((__lisa_features_stop - __lisa_features_start) / sizeof(struct feature))); + pr_info("Available features: "); __process_features(NULL, 0, __list_feature); pr_info("\n"); @@ -108,16 +108,14 @@ int init_features(char **selected, size_t selected_len) { static int __disable_explicitly_enabled_feature(struct feature* feature) { bool selected; - - BUG_ON(MAX_FEATURES < ((__lisa_features_stop - __lisa_features_start) / sizeof(struct feature))); + int ret = 0; mutex_lock(feature->lock); selected = feature->__explicitly_enabled; mutex_unlock(feature->lock); if (selected) - __disable_feature(feature); - /* Always return 0 to avoid the early return of __process_features() */ - return 0; + ret |= __disable_feature(feature); + return ret; } int deinit_features(void) { diff --git a/lisa/_assets/kmodules/sched_tp/main.c b/lisa/_assets/kmodules/sched_tp/main.c index 46ad99f3b..62fd915a1 100644 --- a/lisa/_assets/kmodules/sched_tp/main.c +++ b/lisa/_assets/kmodules/sched_tp/main.c @@ -11,7 +11,8 @@ module_param_array(features, charp, &features_len, 0); MODULE_PARM_DESC(features, "Comma-separated list of features to enable. Available features are printed when loading the module"); static void modexit(void) { - deinit_features(); + if (deinit_features()) + pr_err("Some errors happened while unloading LISA kernel module\n"); } static int __init modinit(void) { -- GitLab From 5b07a437f033bb1070d57818a89db2f793d1f82a Mon Sep 17 00:00:00 2001 From: Douglas Raillard Date: Tue, 21 Jun 2022 11:09:04 +0100 Subject: [PATCH 4/4] lisa._assets.kmodules.sched_tp: Do not fail module load when no feature was manually selected FEATURE When the "features" kernel module parameter is not specified, do not fail the module load in case some features fail to enable. This allows enabling as many features as possible, which is likely what the user expects. If features were manually selected, we still fail the module load so the user can rely on having the features they asked for or an obvious error. --- lisa/_assets/kmodules/sched_tp/main.c | 34 +++++++++++++++++---------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/lisa/_assets/kmodules/sched_tp/main.c b/lisa/_assets/kmodules/sched_tp/main.c index 62fd915a1..446ccdbde 100644 --- a/lisa/_assets/kmodules/sched_tp/main.c +++ b/lisa/_assets/kmodules/sched_tp/main.c @@ -16,23 +16,31 @@ static void modexit(void) { } static int __init modinit(void) { - int ret = 0; - ret |= init_features(features, features_len); + int ret = init_features(features, features_len); - /* Use one of the standard error code */ - if (ret) - ret = -EINVAL; - - /* Call modexit() explicitly, since it will not be called when ret != 0. - * Not calling modexit() can (and will) result in kernel panic handlers - * installed by the module are not deregistered before the module code - * vanishes. - */ if (ret) { pr_err("Some errors happened while loading LISA kernel module\n"); - modexit(); + + /* Use one of the standard error code */ + ret = -EINVAL; + + /* If the user selected features manually, make module loading fail so + * that they are aware that things went wrong. Otherwise, just + * keep going as the user just wanted to enable as many features + * as possible. + */ + if (features_len) { + /* Call modexit() explicitly, since it will not be called when ret != 0. + * Not calling modexit() can (and will) result in kernel panic handlers + * installed by the module are not deregistered before the module code + * vanishes. + */ + modexit(); + return ret; + + } } - return ret; + return 0; } module_init(modinit); -- GitLab