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. This also simplifies
cleanup in rdt_kill_sb().
Remove cdp_disable_all() as it is not used anymore after the refactor.
Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Babu Moger <babu.moger@amd.com>
---
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 58 ++++++++++++++++----------
1 file changed, 37 insertions(+), 21 deletions(-)
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 3010e3a1394d..80a4f76bc34b 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2290,14 +2290,6 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable)
return 0;
}
-static void cdp_disable_all(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);
-}
-
/*
* We don't allow rdtgroup directories to be created anywhere
* except the root directory. Thus when looking for the rdtgroup
@@ -2377,19 +2369,47 @@ static int mkdir_mondata_all(struct kernfs_node *parent_kn,
struct rdtgroup *prgrp,
struct kernfs_node **mon_data_kn);
+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);
+}
+
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_done;
+ }
- if (!ret && ctx->enable_cdpl3)
+ if (ctx->enable_cdpl3) {
ret = resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, true);
+ if (ret)
+ goto out_cdpl2;
+ }
- if (!ret && ctx->enable_mba_mbps)
+ if (ctx->enable_mba_mbps) {
ret = set_mba_sc(true);
+ if (ret)
+ goto out_cdpl3;
+ }
+
+ return 0;
+out_cdpl3:
+ resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
+out_cdpl2:
+ resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, false);
+out_done:
return ret;
}
@@ -2497,13 +2517,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 +2582,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();
out:
rdt_last_cmd_clear();
mutex_unlock(&rdtgroup_mutex);
@@ -2798,12 +2815,11 @@ static void rdt_kill_sb(struct super_block *sb)
cpus_read_lock();
mutex_lock(&rdtgroup_mutex);
- set_mba_sc(false);
+ rdt_disable_ctx();
/*Put everything back to default values. */
for_each_alloc_capable_rdt_resource(r)
reset_all_ctrls(r);
- cdp_disable_all();
rmdir_all_sub();
rdt_pseudo_lock_release();
rdtgroup_default.mode = RDT_MODE_SHAREABLE;
--
2.34.1On 8/21/23 16:30, 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. This also simplifies > cleanup in rdt_kill_sb(). > > Remove cdp_disable_all() as it is not used anymore after the refactor. > > Suggested-by: Reinette Chatre <reinette.chatre@intel.com> > Signed-off-by: Babu Moger <babu.moger@amd.com> Reviewed-by: Fenghua Yu <fenghua.yu@intel.com> Thanks. -Fenghua
Hi Babu,
On 8/21/2023 4:30 PM, Babu Moger wrote:
> 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_done;
> + }
>
> - if (!ret && ctx->enable_cdpl3)
> + if (ctx->enable_cdpl3) {
> ret = resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, true);
> + if (ret)
> + goto out_cdpl2;
> + }
>
> - if (!ret && ctx->enable_mba_mbps)
> + if (ctx->enable_mba_mbps) {
> ret = set_mba_sc(true);
> + if (ret)
> + goto out_cdpl3;
An error may be encountered here without CDP ever enabled or just
enabled for L2 or L3. I think that the error unwinding should
take care to not unwind an action that was not done. Considering
the information available I think checking either ctx->enable_...
or the checks used in rdt_disable_ctx() would be ok but for consistency
the resctrl_arch_get_cdp_enabled() checks may be most appropriate.
> + }
> +
> + return 0;
>
> +out_cdpl3:
So here I think there should be a check.
if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L3))
> + resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
> +out_cdpl2:
... and here a check:
if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L2))
> + resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, false);
> +out_done:
> return ret;
> }
>
Reinette
Hi Reinette,
On 8/29/23 15:10, Reinette Chatre wrote:
> Hi Babu,
>
> On 8/21/2023 4:30 PM, Babu Moger wrote:
>> 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_done;
>> + }
>>
>> - if (!ret && ctx->enable_cdpl3)
>> + if (ctx->enable_cdpl3) {
>> ret = resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, true);
>> + if (ret)
>> + goto out_cdpl2;
>> + }
>>
>> - if (!ret && ctx->enable_mba_mbps)
>> + if (ctx->enable_mba_mbps) {
>> ret = set_mba_sc(true);
>> + if (ret)
>> + goto out_cdpl3;
>
> An error may be encountered here without CDP ever enabled or just
> enabled for L2 or L3. I think that the error unwinding should
> take care to not unwind an action that was not done. Considering
> the information available I think checking either ctx->enable_...
> or the checks used in rdt_disable_ctx() would be ok but for consistency
> the resctrl_arch_get_cdp_enabled() checks may be most appropriate.
>
>> + }
>> +
>> + return 0;
>>
>> +out_cdpl3:
>
> So here I think there should be a check.
> if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L3))
>
>> + resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
>> +out_cdpl2:
>
> ... and here a check:
> if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L2))
I know it does not hurt to add these checks. But, it may be unnecessary
considering cdp_disable() has the check "if (r_hw->cdp_enabled)" already.
Both are same checks. What do you think?
--
Thanks
Babu Moger
Hi Babu,
On 8/30/2023 9:38 AM, Moger, Babu wrote:
> On 8/29/23 15:10, Reinette Chatre wrote:
>> On 8/21/2023 4:30 PM, Babu Moger wrote:
>>> 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_done;
>>> + }
>>>
>>> - if (!ret && ctx->enable_cdpl3)
>>> + if (ctx->enable_cdpl3) {
>>> ret = resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, true);
>>> + if (ret)
>>> + goto out_cdpl2;
>>> + }
>>>
>>> - if (!ret && ctx->enable_mba_mbps)
>>> + if (ctx->enable_mba_mbps) {
>>> ret = set_mba_sc(true);
>>> + if (ret)
>>> + goto out_cdpl3;
>>
>> An error may be encountered here without CDP ever enabled or just
>> enabled for L2 or L3. I think that the error unwinding should
>> take care to not unwind an action that was not done. Considering
>> the information available I think checking either ctx->enable_...
>> or the checks used in rdt_disable_ctx() would be ok but for consistency
>> the resctrl_arch_get_cdp_enabled() checks may be most appropriate.
>>
>>> + }
>>> +
>>> + return 0;
>>>
>>> +out_cdpl3:
>>
>> So here I think there should be a check.
>> if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L3))
>>
>>> + resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
>>> +out_cdpl2:
>>
>> ... and here a check:
>> if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L2))
>
>
> I know it does not hurt to add these checks. But, it may be unnecessary
> considering cdp_disable() has the check "if (r_hw->cdp_enabled)" already.
> Both are same checks. What do you think?
Yes, good point. Thank you for checking. Considering this it looks like
rdt_disable_ctx() can be simplified also by removing those CDP
enabled checks from it? Also looks like rdt_disable_ctx()-> set_mba_sc(false)
can be called unconditionally.
Reinette
Hi Reinette,
On 8/30/23 12:56, Reinette Chatre wrote:
> Hi Babu,
>
> On 8/30/2023 9:38 AM, Moger, Babu wrote:
>> On 8/29/23 15:10, Reinette Chatre wrote:
>>> On 8/21/2023 4:30 PM, Babu Moger wrote:
>>>> 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_done;
>>>> + }
>>>>
>>>> - if (!ret && ctx->enable_cdpl3)
>>>> + if (ctx->enable_cdpl3) {
>>>> ret = resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, true);
>>>> + if (ret)
>>>> + goto out_cdpl2;
>>>> + }
>>>>
>>>> - if (!ret && ctx->enable_mba_mbps)
>>>> + if (ctx->enable_mba_mbps) {
>>>> ret = set_mba_sc(true);
>>>> + if (ret)
>>>> + goto out_cdpl3;
>>>
>>> An error may be encountered here without CDP ever enabled or just
>>> enabled for L2 or L3. I think that the error unwinding should
>>> take care to not unwind an action that was not done. Considering
>>> the information available I think checking either ctx->enable_...
>>> or the checks used in rdt_disable_ctx() would be ok but for consistency
>>> the resctrl_arch_get_cdp_enabled() checks may be most appropriate.
>>>
>>>> + }
>>>> +
>>>> + return 0;
>>>>
>>>> +out_cdpl3:
>>>
>>> So here I think there should be a check.
>>> if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L3))
>>>
>>>> + resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
>>>> +out_cdpl2:
>>>
>>> ... and here a check:
>>> if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L2))
>>
>>
>> I know it does not hurt to add these checks. But, it may be unnecessary
>> considering cdp_disable() has the check "if (r_hw->cdp_enabled)" already.
>> Both are same checks. What do you think?
>
> Yes, good point. Thank you for checking. Considering this it looks like
> rdt_disable_ctx() can be simplified also by removing those CDP
> enabled checks from it? Also looks like rdt_disable_ctx()-> set_mba_sc(false)
> can be called unconditionally.
Yes. We can do that.
--
Thanks
Babu Moger
© 2016 - 2025 Red Hat, Inc.