rdt_enable_ctx() enables the features provided during resctrl mount.
Additions to rdt_enable_ctx() are required to also modify error paths
of rdt_enable_ctx() callers to ensure correct unwinding if errors
are encountered after calling rdt_enable_ctx(). This is error prone.
Introduce rdt_disable_ctx() to refactor the error unwinding of
rdt_enable_ctx() to simplify future additions.
Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Babu Moger <babu.moger@amd.com>
---
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 44 ++++++++++++++++++++++++--------
1 file changed, 33 insertions(+), 11 deletions(-)
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 3010e3a1394d..0805fac04401 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2377,19 +2377,44 @@ static int mkdir_mondata_all(struct kernfs_node *parent_kn,
struct rdtgroup *prgrp,
struct kernfs_node **mon_data_kn);
+static void rdt_disable_ctx(struct rdt_fs_context *ctx)
+{
+ if (ctx->enable_cdpl2)
+ resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, false);
+
+ if (ctx->enable_cdpl3)
+ resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
+
+ if (ctx->enable_mba_mbps)
+ set_mba_sc(false);
+}
+
static int rdt_enable_ctx(struct rdt_fs_context *ctx)
{
int ret = 0;
- if (ctx->enable_cdpl2)
+ if (ctx->enable_cdpl2) {
ret = resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, true);
+ if (ret)
+ goto out_disable;
+ }
- if (!ret && ctx->enable_cdpl3)
+ if (ctx->enable_cdpl3) {
ret = resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, true);
+ if (ret)
+ goto out_disable;
+ }
- if (!ret && ctx->enable_mba_mbps)
+ if (ctx->enable_mba_mbps) {
ret = set_mba_sc(true);
+ if (ret)
+ goto out_disable;
+ }
+
+ return 0;
+out_disable:
+ rdt_disable_ctx(ctx);
return ret;
}
@@ -2497,13 +2522,13 @@ static int rdt_get_tree(struct fs_context *fc)
}
ret = rdt_enable_ctx(ctx);
- if (ret < 0)
- goto out_cdp;
+ if (ret)
+ goto out;
ret = schemata_list_create();
if (ret) {
schemata_list_destroy();
- goto out_mba;
+ goto out_ctx;
}
closid_init();
@@ -2562,11 +2587,8 @@ static int rdt_get_tree(struct fs_context *fc)
kernfs_remove(kn_info);
out_schemata_free:
schemata_list_destroy();
-out_mba:
- if (ctx->enable_mba_mbps)
- set_mba_sc(false);
-out_cdp:
- cdp_disable_all();
+out_ctx:
+ rdt_disable_ctx(ctx);
out:
rdt_last_cmd_clear();
mutex_unlock(&rdtgroup_mutex);
Hi Babu,
On 8/11/2023 1:09 PM, Babu Moger wrote:
> rdt_enable_ctx() enables the features provided during resctrl mount.
>
> Additions to rdt_enable_ctx() are required to also modify error paths
> of rdt_enable_ctx() callers to ensure correct unwinding if errors
> are encountered after calling rdt_enable_ctx(). This is error prone.
>
> Introduce rdt_disable_ctx() to refactor the error unwinding of
> rdt_enable_ctx() to simplify future additions.
>
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 44 ++++++++++++++++++++++++--------
> 1 file changed, 33 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 3010e3a1394d..0805fac04401 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -2377,19 +2377,44 @@ static int mkdir_mondata_all(struct kernfs_node *parent_kn,
> struct rdtgroup *prgrp,
> struct kernfs_node **mon_data_kn);
>
> +static void rdt_disable_ctx(struct rdt_fs_context *ctx)
> +{
> + if (ctx->enable_cdpl2)
> + resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, false);
> +
> + if (ctx->enable_cdpl3)
> + resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
> +
> + if (ctx->enable_mba_mbps)
> + set_mba_sc(false);
> +}
I am not sure if rdt_disable_ctx() should depend on the
fs context (more later).
> +
> static int rdt_enable_ctx(struct rdt_fs_context *ctx)
> {
> int ret = 0;
>
> - if (ctx->enable_cdpl2)
> + if (ctx->enable_cdpl2) {
> ret = resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, true);
> + if (ret)
> + goto out_disable;
> + }
>
> - if (!ret && ctx->enable_cdpl3)
> + if (ctx->enable_cdpl3) {
> ret = resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, true);
> + if (ret)
> + goto out_disable;
> + }
>
> - if (!ret && ctx->enable_mba_mbps)
> + if (ctx->enable_mba_mbps) {
> ret = set_mba_sc(true);
> + if (ret)
> + goto out_disable;
> + }
> +
> + return 0;
>
> +out_disable:
> + rdt_disable_ctx(ctx);
> return ret;
This is not the exit pattern we usually follow. Also note
that it could lead to incorrect behavior if there is an early failure
in this function but all unwinding would end up being done in
rdt_disable_ctx() because error unwinding is done based on whether
a feature _should_ be enabled not whether it was enabled.
Could you please instead have direct error handling by only undoing
what was already done at the time of the error?
> }
>
> @@ -2497,13 +2522,13 @@ static int rdt_get_tree(struct fs_context *fc)
> }
>
> ret = rdt_enable_ctx(ctx);
> - if (ret < 0)
> - goto out_cdp;
> + if (ret)
> + goto out;
>
> ret = schemata_list_create();
> if (ret) {
> schemata_list_destroy();
> - goto out_mba;
> + goto out_ctx;
> }
>
> closid_init();
> @@ -2562,11 +2587,8 @@ static int rdt_get_tree(struct fs_context *fc)
> kernfs_remove(kn_info);
> out_schemata_free:
> schemata_list_destroy();
> -out_mba:
> - if (ctx->enable_mba_mbps)
> - set_mba_sc(false);
> -out_cdp:
> - cdp_disable_all();
> +out_ctx:
> + rdt_disable_ctx(ctx);
> out:
> rdt_last_cmd_clear();
> mutex_unlock(&rdtgroup_mutex);
>
>
rdt_enable_ctx() is called in rdt_get_tree() and thus its work should
also be cleaned up in rdt_kill_sb(). Note how the cleanup you replace
here is also duplicated in rdt_kill_sb(), meaning the unwinding continues
to be open coded and patch #7 propagates this.
Could rdt_kill_sb() not also call rdt_disable_ctx()? This brings me
back to the earlier comment about it depending on the fs context. I
do not know if the context will be valid at this time. I do not
think that the context is required though it could have no parameters
and do cleanup as is done at the moment.
Reinette
Hi Reinette,
On 8/15/23 17:47, Reinette Chatre wrote:
> Hi Babu,
>
> On 8/11/2023 1:09 PM, Babu Moger wrote:
>> rdt_enable_ctx() enables the features provided during resctrl mount.
>>
>> Additions to rdt_enable_ctx() are required to also modify error paths
>> of rdt_enable_ctx() callers to ensure correct unwinding if errors
>> are encountered after calling rdt_enable_ctx(). This is error prone.
>>
>> Introduce rdt_disable_ctx() to refactor the error unwinding of
>> rdt_enable_ctx() to simplify future additions.
>>
>> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 44 ++++++++++++++++++++++++--------
>> 1 file changed, 33 insertions(+), 11 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 3010e3a1394d..0805fac04401 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -2377,19 +2377,44 @@ static int mkdir_mondata_all(struct kernfs_node *parent_kn,
>> struct rdtgroup *prgrp,
>> struct kernfs_node **mon_data_kn);
>>
>> +static void rdt_disable_ctx(struct rdt_fs_context *ctx)
>> +{
>> + if (ctx->enable_cdpl2)
>> + resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, false);
>> +
>> + if (ctx->enable_cdpl3)
>> + resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
>> +
>> + if (ctx->enable_mba_mbps)
>> + set_mba_sc(false);
>> +}
>
> I am not sure if rdt_disable_ctx() should depend on the
> fs context (more later).
>
>> +
>> static int rdt_enable_ctx(struct rdt_fs_context *ctx)
>> {
>> int ret = 0;
>>
>> - if (ctx->enable_cdpl2)
>> + if (ctx->enable_cdpl2) {
>> ret = resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, true);
>> + if (ret)
>> + goto out_disable;
>> + }
>>
>> - if (!ret && ctx->enable_cdpl3)
>> + if (ctx->enable_cdpl3) {
>> ret = resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, true);
>> + if (ret)
>> + goto out_disable;
>> + }
>>
>> - if (!ret && ctx->enable_mba_mbps)
>> + if (ctx->enable_mba_mbps) {
>> ret = set_mba_sc(true);
>> + if (ret)
>> + goto out_disable;
>> + }
>> +
>> + return 0;
>>
>> +out_disable:
>> + rdt_disable_ctx(ctx);
>> return ret;
>
> This is not the exit pattern we usually follow. Also note
> that it could lead to incorrect behavior if there is an early failure
> in this function but all unwinding would end up being done in
> rdt_disable_ctx() because error unwinding is done based on whether
> a feature _should_ be enabled not whether it was enabled.
Yes. That is correct.
> Could you please instead have direct error handling by only undoing
> what was already done at the time of the error?
Sure.
>
>> }
>>
>> @@ -2497,13 +2522,13 @@ static int rdt_get_tree(struct fs_context *fc)
>> }
>>
>> ret = rdt_enable_ctx(ctx);
>> - if (ret < 0)
>> - goto out_cdp;
>> + if (ret)
>> + goto out;
>>
>> ret = schemata_list_create();
>> if (ret) {
>> schemata_list_destroy();
>> - goto out_mba;
>> + goto out_ctx;
>> }
>>
>> closid_init();
>> @@ -2562,11 +2587,8 @@ static int rdt_get_tree(struct fs_context *fc)
>> kernfs_remove(kn_info);
>> out_schemata_free:
>> schemata_list_destroy();
>> -out_mba:
>> - if (ctx->enable_mba_mbps)
>> - set_mba_sc(false);
>> -out_cdp:
>> - cdp_disable_all();
>> +out_ctx:
>> + rdt_disable_ctx(ctx);
>> out:
>> rdt_last_cmd_clear();
>> mutex_unlock(&rdtgroup_mutex);
>>
>>
>
> rdt_enable_ctx() is called in rdt_get_tree() and thus its work should
> also be cleaned up in rdt_kill_sb(). Note how the cleanup you replace
> here is also duplicated in rdt_kill_sb(), meaning the unwinding continues
> to be open coded and patch #7 propagates this.
> Could rdt_kill_sb() not also call rdt_disable_ctx()? This brings me
> back to the earlier comment about it depending on the fs context. I
> do not know if the context will be valid at this time. I do not
> think that the context is required though it could have no parameters
> and do cleanup as is done at the moment.
At rdt_kill_sb() the fs context is already freed. But, we can call
rdt_disable_ctx() with no parameter. We will have to depend on other
parameters to free the enabled features. We can use the same call both in
rdt_get_tree() (the failure path above) and in rdt_kill_sb().
The function rdt_disable_ctx will look like this.
+static void rdt_disable_ctx(void)
+{
+ if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L3))
+ resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
+
+ if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L2))
+ resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, false);
+
+ if (is_mba_sc(&rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl))
+ set_mba_sc(false);
+}
--
Thanks
Babu Moger
Hi Babu,
On 8/16/2023 11:17 AM, Moger, Babu wrote:
> At rdt_kill_sb() the fs context is already freed. But, we can call
> rdt_disable_ctx() with no parameter. We will have to depend on other
> parameters to free the enabled features. We can use the same call both in
> rdt_get_tree() (the failure path above) and in rdt_kill_sb().
>
> The function rdt_disable_ctx will look like this.
>
> +static void rdt_disable_ctx(void)
> +{
> + if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L3))
> + resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
> +
> + if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L2))
> + resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, false);
> +
> + if (is_mba_sc(&rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl))
> + set_mba_sc(false);
> +}
>
>
This looks good to me. I think this will end up making
cdp_disable_all() unused so it may be candidate for removal as part
of this change.
Reinette
© 2016 - 2025 Red Hat, Inc.