diff mbox series

[v11,05/10] x86/resctrl: Unwind the errors inside rdt_enable_ctx()

Message ID 20231003235430.1231238-6-babu.moger@amd.com
State New
Headers
Series x86/resctrl: Miscellaneous resctrl features |

Commit Message

Moger, Babu Oct. 3, 2023, 11:54 p.m. UTC 
  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>
Tested-by: Peter Newman <peternewman@google.com>
Reviewed-by: Peter Newman <peternewman@google.com>
Tested-by: Tan Shaopeng <tan.shaopeng@jp.fujitsu.com>
Reviewed-by: Tan Shaopeng <tan.shaopeng@jp.fujitsu.com>
Reviewed-by: Fenghua Yu <fenghua.yu@intel.com>
Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 53 ++++++++++++++++----------
 1 file changed, 32 insertions(+), 21 deletions(-)

Comments

Borislav Petkov Oct. 9, 2023, 5:25 p.m. UTC | #1 
On Tue, Oct 03, 2023 at 06:54:25PM -0500, 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.

Do not talk about *what* the patch is doing in the commit message - that
should be obvious from the diff itself. Rather, concentrate on the *why*
it needs to be done.

Check your whole series.
Fenghua Yu Oct. 9, 2023, 5:40 p.m. UTC | #2 
Hi, Babu,

On 10/9/23 10:25, Borislav Petkov wrote:
> On Tue, Oct 03, 2023 at 06:54:25PM -0500, 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.
> 
> Do not talk about *what* the patch is doing in the commit message - that
> should be obvious from the diff itself. Rather, concentrate on the *why*
> it needs to be done.
> 
> Check your whole series.

When you send the next patch set, please add the change log in each 
patch after the "---" instead of in the cover patch only. So Boris and 
others clearly know what are changed in each patch.

Thanks.

-Fenghua
Reinette Chatre Oct. 9, 2023, 5:59 p.m. UTC | #3 
Hi Boris,

On 10/9/2023 10:25 AM, Borislav Petkov wrote:
> On Tue, Oct 03, 2023 at 06:54:25PM -0500, 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.
> 
> Do not talk about *what* the patch is doing in the commit message - that
> should be obvious from the diff itself. Rather, concentrate on the *why*
> it needs to be done.

I worked with Babu on this commit message and would appreciate guidance to
get this (and others) right. The second paragraph intends to explain the
current problem with rdt_enable_ctx() with the third paragraph providing an
overview of how the problem will be fixed and why (future code needs to touch
this area).

Is it the fourth paragraph (mentioning cdp_disable_all()) that is annoying? I
can see that it is redundant. Would it be more palatable if the fourth paragraph
is just dropped?

> 
> Check your whole series.
> 

Reinette
Borislav Petkov Oct. 9, 2023, 7:23 p.m. UTC | #4 
On Mon, Oct 09, 2023 at 10:59:27AM -0700, Reinette Chatre wrote:
> Is it the fourth paragraph (mentioning cdp_disable_all()) that is annoying? I
> can see that it is redundant. Would it be more palatable if the fourth paragraph
> is just dropped?

Yes, basically you don't want to explain what a patch does as that
should be obvious from the diff. Rather, it should talk about why
a change is being done. Sure, sometimes, you need to talk about the
change in case you want to highlight certain aspects of why the code is
being changed in the first place but explaining in text what is already
visible in the diff is not very useful.

I always give the example about git archeology here: put enough info in
the commit message so that any future reader of it can understand why
the change was done. The "what" of a patch doesn't belong to that text.

I hope that makes more sense.
Moger, Babu Oct. 9, 2023, 7:50 p.m. UTC | #5 
Hi Fenghua,

On 10/9/23 12:40, Fenghua Yu wrote:
> Hi, Babu,
> 
> On 10/9/23 10:25, Borislav Petkov wrote:
>> On Tue, Oct 03, 2023 at 06:54:25PM -0500, 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.
>>
>> Do not talk about *what* the patch is doing in the commit message - that
>> should be obvious from the diff itself. Rather, concentrate on the *why*
>> it needs to be done.
>>
>> Check your whole series.
> 
> When you send the next patch set, please add the change log in each patch
> after the "---" instead of in the cover patch only. So Boris and others
> clearly know what are changed in each patch.

Sure. Will do in future.
Moger, Babu Oct. 9, 2023, 7:58 p.m. UTC | #6 
On 10/9/23 14:23, Borislav Petkov wrote:
> On Mon, Oct 09, 2023 at 10:59:27AM -0700, Reinette Chatre wrote:
>> Is it the fourth paragraph (mentioning cdp_disable_all()) that is annoying? I
>> can see that it is redundant. Would it be more palatable if the fourth paragraph
>> is just dropped?
> 
> Yes, basically you don't want to explain what a patch does as that
> should be obvious from the diff. Rather, it should talk about why
> a change is being done. Sure, sometimes, you need to talk about the
> change in case you want to highlight certain aspects of why the code is
> being changed in the first place but explaining in text what is already
> visible in the diff is not very useful.
> 
> I always give the example about git archeology here: put enough info in
> the commit message so that any future reader of it can understand why
> the change was done. The "what" of a patch doesn't belong to that text.
> 
> I hope that makes more sense.
> 

Sure. Will drop the last paragraph in next revision.
Reinette Chatre Oct. 9, 2023, 8:10 p.m. UTC | #7 
Hi Boris,

On 10/9/2023 12:23 PM, Borislav Petkov wrote:
> On Mon, Oct 09, 2023 at 10:59:27AM -0700, Reinette Chatre wrote:
>> Is it the fourth paragraph (mentioning cdp_disable_all()) that is annoying? I
>> can see that it is redundant. Would it be more palatable if the fourth paragraph
>> is just dropped?
> 
> Yes, basically you don't want to explain what a patch does as that
> should be obvious from the diff. Rather, it should talk about why
> a change is being done. Sure, sometimes, you need to talk about the
> change in case you want to highlight certain aspects of why the code is
> being changed in the first place but explaining in text what is already
> visible in the diff is not very useful.
> 
> I always give the example about git archeology here: put enough info in
> the commit message so that any future reader of it can understand why
> the change was done. The "what" of a patch doesn't belong to that text.
> 
> I hope that makes more sense.
> 

This is clear. Thank you very much. (I am still working on getting it right
in practice though.)

Reinette
diff mbox series

Patch

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 35945b4bf196..3ea874c80c22 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,42 @@  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)
+{
+	resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
+	resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, false);
+	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 +2512,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 +2577,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 +2810,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;