[PATCH v6 0/8] x86/resctrl: Miscellaneous resctrl features

[PATCH v6 0/8] x86/resctrl: Miscellaneous resctrl features

[Babu Moger]

These series adds support few minor features.
1. Support assigning multiple tasks to control/mon groups in one command.
2. Add debug mount option for resctrl interface.
3. Add RMID and CLOSID in resctrl interface when mounted with debug option.
4. Moves the default control group creation during the mount instead of during init.
5. While doing these above changes, found that rftype flags needed some cleanup.
   They were named inconsistently. Re-arranged them much more cleanly now and added
   few comments. Hope it can help future additions.
---
v6:
   Changes since v5:
   Moved the default group creation during mount instead of kernel init.
   The rdt_root creation moved to rdt_get_tree as suggested by Reinette.
   https://lore.kernel.org/lkml/8f68ace7-e05b-ad6d-fa74-5ff8e179aec9@intel.com/
   Needed to modify rdtgroup_setup_root to take care of this.
   Re-arraged the patches to move the default group creation earlier.
   Others are mostly text changes and few minor changes.
   Patches are based on tip/master commit 1a2945f27157825a561be7840023e3664111ab2f

v5:
   Changes since v4:
   Moved the default group creation during mount instead of kernel init.
   Tried to address most of the comments on commit log. Added more context and details.
   Addressed feedback about the patch4. Removed the code changes and only kept the comments.
   I am ok to drop patch4. But I will wait for the comment on that.
   There were lots of comments. Hope I did not miss anything. Even if I missed, it is
   not intentional. 

v4: Changes since v3
    Addressed comments from Reinette and others.
    Removed newline requirement when adding tasks.
    Dropped one of the changes on flags. Kept the flag names mostly same.
    Changed the names of closid and rmid to ctrl_hw_id and mon_hw_id respectively.
    James had some concerns about adding these files. Addressed it by making these
    files x86 specific.
    Tried to address Reinette's comment on patch 7. But due to current code design
    I could not do it exact way. But changed it little bit to make it easy debug
    file additions in the future.  

v3: Changes since v2
    Still waiting for more comments. While waiting, addressed few comments from Fenghua.
    Added few more texts in the documentation about multiple tasks assignment feature.
    Added pid in last_cmd_status when applicable.
    Introduced static resctrl_debug to save the debug option.
    Few minor text changes.
  
v2: Changes since v1
  a. Removed the changes to add the task's threads automatically. It required
     book keeping to handle the failures and gets complicated. Removed that change
     for now.
  b. Added -o debug option to mount in debug mode(comment from Fenghua)
  c. Added debug files rmid and closid. Stephane wanted to rename them more
     generic to accommodate ARM. It kind of loses meaning if is renamed differently.
     Kept it same for now. Will change if he feels strong about it. 

v5: https://lore.kernel.org/lkml/168564586603.527584.10518315376465080920.stgit@bmoger-ubuntu/
v4: https://lore.kernel.org/lkml/168177435378.1758847.8317743523931859131.stgit@bmoger-ubuntu/
v3: https://lore.kernel.org/lkml/167778850105.1053859.14596357862185564029.stgit@bmoger-ubuntu/
v2: https://lore.kernel.org/lkml/167537433143.647488.9641864719195184123.stgit@bmoger-ubuntu/
v1: https://lore.kernel.org/lkml/167278351577.34228.12803395505584557101.stgit@bmoger-ubuntu/


Babu Moger (8):
      x86/resctrl: Add multiple tasks to the resctrl group at once
      x86/resctrl: Simplify rftype flag definitions
      x86/resctrl: Rename rftype flags for consistency
      x86/resctrl: Add comments on RFTYPE flags hierarchy
      x86/resctrl: Unwind the errors inside rdt_enable_ctx
      x86/resctrl: Move default control group creation during mount
      x86/resctrl: Introduce "-o debug" mount option
      x86/resctrl: Display hardware ids of resource groups


 Documentation/arch/x86/resctrl.rst     |  21 ++-
 arch/x86/kernel/cpu/resctrl/internal.h |  70 ++++++--
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 217 ++++++++++++++++++-------
 3 files changed, 236 insertions(+), 72 deletions(-)

--

[PATCH v6 1/8] x86/resctrl: Add multiple tasks to the resctrl group at once

[Babu Moger]

The resctrl task assignment for monitor or control group needs to be
done one at a time. For example:

  $mount -t resctrl resctrl /sys/fs/resctrl/
  $mkdir /sys/fs/resctrl/ctrl_grp1
  $echo 123 > /sys/fs/resctrl/ctrl_grp1/tasks
  $echo 456 > /sys/fs/resctrl/ctrl_grp1/tasks
  $echo 789 > /sys/fs/resctrl/ctrl_grp1/tasks

This is not user-friendly when dealing with hundreds of tasks.

Support multiple task assignment in one command with tasks ids separated
by commas. For example:
  $echo 123,456,789 > /sys/fs/resctrl/ctrl_grp1/tasks

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 Documentation/arch/x86/resctrl.rst     |    8 +++++++-
 arch/x86/kernel/cpu/resctrl/rdtgroup.c |   25 ++++++++++++++++++++++---
 2 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
index cb05d90111b4..af234681756e 100644
--- a/Documentation/arch/x86/resctrl.rst
+++ b/Documentation/arch/x86/resctrl.rst
@@ -299,7 +299,13 @@ All groups contain the following files:
 "tasks":
 	Reading this file shows the list of all tasks that belong to
 	this group. Writing a task id to the file will add a task to the
-	group. If the group is a CTRL_MON group the task is removed from
+	group. Multiple tasks can be added by separating the task ids
+	with commas. Tasks will be assigned sequentially. Multiple
+	failures are not supported. A single failure encountered while
+	attempting to assign a task will cause the operation to abort.
+	Failures will be logged to /sys/fs/resctrl/info/last_cmd_status.
+
+	If the group is a CTRL_MON group the task is removed from
 	whichever previous CTRL_MON group owned the task and also from
 	any MON group that owned the task. If the group is a MON group,
 	then the task must already belong to the CTRL_MON parent of this
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 725344048f85..8c91c333f9b3 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -696,11 +696,10 @@ static ssize_t rdtgroup_tasks_write(struct kernfs_open_file *of,
 				    char *buf, size_t nbytes, loff_t off)
 {
 	struct rdtgroup *rdtgrp;
+	char *pid_str;
 	int ret = 0;
 	pid_t pid;
 
-	if (kstrtoint(strstrip(buf), 0, &pid) || pid < 0)
-		return -EINVAL;
 	rdtgrp = rdtgroup_kn_lock_live(of->kn);
 	if (!rdtgrp) {
 		rdtgroup_kn_unlock(of->kn);
@@ -715,7 +714,27 @@ static ssize_t rdtgroup_tasks_write(struct kernfs_open_file *of,
 		goto unlock;
 	}
 
-	ret = rdtgroup_move_task(pid, rdtgrp, of);
+	while (buf && buf[0] != '\0' && buf[0] != '\n') {
+		pid_str = strim(strsep(&buf, ","));
+
+		if (kstrtoint(pid_str, 0, &pid)) {
+			rdt_last_cmd_puts("Task list parsing error\n");
+			ret = -EINVAL;
+			break;
+		}
+
+		if (pid < 0) {
+			rdt_last_cmd_printf("Invalid pid %d\n", pid);
+			ret = -EINVAL;
+			break;
+		}
+
+		ret = rdtgroup_move_task(pid, rdtgrp, of);
+		if (ret) {
+			rdt_last_cmd_printf("Error while processing task %d\n", pid);
+			break;
+		}
+	}
 
 unlock:
 	rdtgroup_kn_unlock(of->kn);

[PATCH v6 2/8] x86/resctrl: Simplify rftype flag definitions

[Babu Moger]

The rftype flags are bitmaps used for adding files under resctrl
filesystem. Some of these bitmaps have one extra level of indirection
which is not necessary.

Make them all direct definition to be consistent and easier to read.

Reviewed-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
Tested-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
Signed-off-by: Babu Moger <babu.moger@amd.com>
Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
---
 arch/x86/kernel/cpu/resctrl/internal.h |    9 +++------
 arch/x86/kernel/cpu/resctrl/rdtgroup.c |    6 +++++-
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 85ceaf9a31ac..62767774810d 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -243,12 +243,9 @@ struct rdtgroup {
  */
 #define RFTYPE_INFO			BIT(0)
 #define RFTYPE_BASE			BIT(1)
-#define RF_CTRLSHIFT			4
-#define RF_MONSHIFT			5
-#define RF_TOPSHIFT			6
-#define RFTYPE_CTRL			BIT(RF_CTRLSHIFT)
-#define RFTYPE_MON			BIT(RF_MONSHIFT)
-#define RFTYPE_TOP			BIT(RF_TOPSHIFT)
+#define RFTYPE_CTRL			BIT(4)
+#define RFTYPE_MON			BIT(5)
+#define RFTYPE_TOP			BIT(6)
 #define RFTYPE_RES_CACHE		BIT(8)
 #define RFTYPE_RES_MB			BIT(9)
 #define RF_CTRL_INFO			(RFTYPE_INFO | RFTYPE_CTRL)
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 8c91c333f9b3..2f1b9f69326f 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -3242,7 +3242,11 @@ static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
 		goto out_destroy;
 	}
 
-	files = RFTYPE_BASE | BIT(RF_CTRLSHIFT + rtype);
+	if (rtype == RDTCTRL_GROUP)
+		files = RFTYPE_BASE | RFTYPE_CTRL;
+	else
+		files = RFTYPE_BASE | RFTYPE_MON;
+
 	ret = rdtgroup_add_files(kn, files);
 	if (ret) {
 		rdt_last_cmd_puts("kernfs fill error\n");

[PATCH v6 3/8] x86/resctrl: Rename rftype flags for consistency

[Babu Moger]

resctrl associates rftype flags with its files so that files can be chosen
based on the resource, whether it is info or base, and if it is control
or monitor type file. These flags use the RF_ as well as RFTYPE_ prefixes.

Change the prefix to RFTYPE_ for all these flags to be consistent.

Reviewed-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
Tested-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
Signed-off-by: Babu Moger <babu.moger@amd.com>
Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
---
 arch/x86/kernel/cpu/resctrl/internal.h |   10 ++++----
 arch/x86/kernel/cpu/resctrl/rdtgroup.c |   42 ++++++++++++++++----------------
 2 files changed, 26 insertions(+), 26 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 62767774810d..2051179a3b91 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -248,10 +248,10 @@ struct rdtgroup {
 #define RFTYPE_TOP			BIT(6)
 #define RFTYPE_RES_CACHE		BIT(8)
 #define RFTYPE_RES_MB			BIT(9)
-#define RF_CTRL_INFO			(RFTYPE_INFO | RFTYPE_CTRL)
-#define RF_MON_INFO			(RFTYPE_INFO | RFTYPE_MON)
-#define RF_TOP_INFO			(RFTYPE_INFO | RFTYPE_TOP)
-#define RF_CTRL_BASE			(RFTYPE_BASE | RFTYPE_CTRL)
+#define RFTYPE_CTRL_INFO		(RFTYPE_INFO | RFTYPE_CTRL)
+#define RFTYPE_MON_INFO			(RFTYPE_INFO | RFTYPE_MON)
+#define RFTYPE_TOP_INFO			(RFTYPE_INFO | RFTYPE_TOP)
+#define RFTYPE_CTRL_BASE		(RFTYPE_BASE | RFTYPE_CTRL)
 
 /* List of all resource groups */
 extern struct list_head rdt_all_groups;
@@ -267,7 +267,7 @@ void __exit rdtgroup_exit(void);
  * @mode:	Access mode
  * @kf_ops:	File operations
  * @flags:	File specific RFTYPE_FLAGS_* flags
- * @fflags:	File specific RF_* or RFTYPE_* flags
+ * @fflags:	File specific RFTYPE_* flags
  * @seq_show:	Show content of the file
  * @write:	Write to the file
  */
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 2f1b9f69326f..3010e3a1394d 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1705,77 +1705,77 @@ static struct rftype res_common_files[] = {
 		.mode		= 0444,
 		.kf_ops		= &rdtgroup_kf_single_ops,
 		.seq_show	= rdt_last_cmd_status_show,
-		.fflags		= RF_TOP_INFO,
+		.fflags		= RFTYPE_TOP_INFO,
 	},
 	{
 		.name		= "num_closids",
 		.mode		= 0444,
 		.kf_ops		= &rdtgroup_kf_single_ops,
 		.seq_show	= rdt_num_closids_show,
-		.fflags		= RF_CTRL_INFO,
+		.fflags		= RFTYPE_CTRL_INFO,
 	},
 	{
 		.name		= "mon_features",
 		.mode		= 0444,
 		.kf_ops		= &rdtgroup_kf_single_ops,
 		.seq_show	= rdt_mon_features_show,
-		.fflags		= RF_MON_INFO,
+		.fflags		= RFTYPE_MON_INFO,
 	},
 	{
 		.name		= "num_rmids",
 		.mode		= 0444,
 		.kf_ops		= &rdtgroup_kf_single_ops,
 		.seq_show	= rdt_num_rmids_show,
-		.fflags		= RF_MON_INFO,
+		.fflags		= RFTYPE_MON_INFO,
 	},
 	{
 		.name		= "cbm_mask",
 		.mode		= 0444,
 		.kf_ops		= &rdtgroup_kf_single_ops,
 		.seq_show	= rdt_default_ctrl_show,
-		.fflags		= RF_CTRL_INFO | RFTYPE_RES_CACHE,
+		.fflags		= RFTYPE_CTRL_INFO | RFTYPE_RES_CACHE,
 	},
 	{
 		.name		= "min_cbm_bits",
 		.mode		= 0444,
 		.kf_ops		= &rdtgroup_kf_single_ops,
 		.seq_show	= rdt_min_cbm_bits_show,
-		.fflags		= RF_CTRL_INFO | RFTYPE_RES_CACHE,
+		.fflags		= RFTYPE_CTRL_INFO | RFTYPE_RES_CACHE,
 	},
 	{
 		.name		= "shareable_bits",
 		.mode		= 0444,
 		.kf_ops		= &rdtgroup_kf_single_ops,
 		.seq_show	= rdt_shareable_bits_show,
-		.fflags		= RF_CTRL_INFO | RFTYPE_RES_CACHE,
+		.fflags		= RFTYPE_CTRL_INFO | RFTYPE_RES_CACHE,
 	},
 	{
 		.name		= "bit_usage",
 		.mode		= 0444,
 		.kf_ops		= &rdtgroup_kf_single_ops,
 		.seq_show	= rdt_bit_usage_show,
-		.fflags		= RF_CTRL_INFO | RFTYPE_RES_CACHE,
+		.fflags		= RFTYPE_CTRL_INFO | RFTYPE_RES_CACHE,
 	},
 	{
 		.name		= "min_bandwidth",
 		.mode		= 0444,
 		.kf_ops		= &rdtgroup_kf_single_ops,
 		.seq_show	= rdt_min_bw_show,
-		.fflags		= RF_CTRL_INFO | RFTYPE_RES_MB,
+		.fflags		= RFTYPE_CTRL_INFO | RFTYPE_RES_MB,
 	},
 	{
 		.name		= "bandwidth_gran",
 		.mode		= 0444,
 		.kf_ops		= &rdtgroup_kf_single_ops,
 		.seq_show	= rdt_bw_gran_show,
-		.fflags		= RF_CTRL_INFO | RFTYPE_RES_MB,
+		.fflags		= RFTYPE_CTRL_INFO | RFTYPE_RES_MB,
 	},
 	{
 		.name		= "delay_linear",
 		.mode		= 0444,
 		.kf_ops		= &rdtgroup_kf_single_ops,
 		.seq_show	= rdt_delay_linear_show,
-		.fflags		= RF_CTRL_INFO | RFTYPE_RES_MB,
+		.fflags		= RFTYPE_CTRL_INFO | RFTYPE_RES_MB,
 	},
 	/*
 	 * Platform specific which (if any) capabilities are provided by
@@ -1794,7 +1794,7 @@ static struct rftype res_common_files[] = {
 		.kf_ops		= &rdtgroup_kf_single_ops,
 		.write		= max_threshold_occ_write,
 		.seq_show	= max_threshold_occ_show,
-		.fflags		= RF_MON_INFO | RFTYPE_RES_CACHE,
+		.fflags		= RFTYPE_MON_INFO | RFTYPE_RES_CACHE,
 	},
 	{
 		.name		= "mbm_total_bytes_config",
@@ -1841,7 +1841,7 @@ static struct rftype res_common_files[] = {
 		.kf_ops		= &rdtgroup_kf_single_ops,
 		.write		= rdtgroup_schemata_write,
 		.seq_show	= rdtgroup_schemata_show,
-		.fflags		= RF_CTRL_BASE,
+		.fflags		= RFTYPE_CTRL_BASE,
 	},
 	{
 		.name		= "mode",
@@ -1849,14 +1849,14 @@ static struct rftype res_common_files[] = {
 		.kf_ops		= &rdtgroup_kf_single_ops,
 		.write		= rdtgroup_mode_write,
 		.seq_show	= rdtgroup_mode_show,
-		.fflags		= RF_CTRL_BASE,
+		.fflags		= RFTYPE_CTRL_BASE,
 	},
 	{
 		.name		= "size",
 		.mode		= 0444,
 		.kf_ops		= &rdtgroup_kf_single_ops,
 		.seq_show	= rdtgroup_size_show,
-		.fflags		= RF_CTRL_BASE,
+		.fflags		= RFTYPE_CTRL_BASE,
 	},
 
 };
@@ -1913,7 +1913,7 @@ void __init thread_throttle_mode_init(void)
 	if (!rft)
 		return;
 
-	rft->fflags = RF_CTRL_INFO | RFTYPE_RES_MB;
+	rft->fflags = RFTYPE_CTRL_INFO | RFTYPE_RES_MB;
 }
 
 void __init mbm_config_rftype_init(const char *config)
@@ -1922,7 +1922,7 @@ void __init mbm_config_rftype_init(const char *config)
 
 	rft = rdtgroup_get_rftype_by_name(config);
 	if (rft)
-		rft->fflags = RF_MON_INFO | RFTYPE_RES_CACHE;
+		rft->fflags = RFTYPE_MON_INFO | RFTYPE_RES_CACHE;
 }
 
 /**
@@ -2057,21 +2057,21 @@ static int rdtgroup_create_info_dir(struct kernfs_node *parent_kn)
 	if (IS_ERR(kn_info))
 		return PTR_ERR(kn_info);
 
-	ret = rdtgroup_add_files(kn_info, RF_TOP_INFO);
+	ret = rdtgroup_add_files(kn_info, RFTYPE_TOP_INFO);
 	if (ret)
 		goto out_destroy;
 
 	/* loop over enabled controls, these are all alloc_capable */
 	list_for_each_entry(s, &resctrl_schema_all, list) {
 		r = s->res;
-		fflags =  r->fflags | RF_CTRL_INFO;
+		fflags = r->fflags | RFTYPE_CTRL_INFO;
 		ret = rdtgroup_mkdir_info_resdir(s, s->name, fflags);
 		if (ret)
 			goto out_destroy;
 	}
 
 	for_each_mon_capable_rdt_resource(r) {
-		fflags =  r->fflags | RF_MON_INFO;
+		fflags = r->fflags | RFTYPE_MON_INFO;
 		sprintf(name, "%s_MON", r->name);
 		ret = rdtgroup_mkdir_info_resdir(r, name, fflags);
 		if (ret)
@@ -3709,7 +3709,7 @@ static int __init rdtgroup_setup_root(void)
 
 	list_add(&rdtgroup_default.rdtgroup_list, &rdt_all_groups);
 
-	ret = rdtgroup_add_files(kernfs_root_to_node(rdt_root), RF_CTRL_BASE);
+	ret = rdtgroup_add_files(kernfs_root_to_node(rdt_root), RFTYPE_CTRL_BASE);
 	if (ret) {
 		kernfs_destroy_root(rdt_root);
 		goto out;

[PATCH v6 4/8] x86/resctrl: Add comments on RFTYPE flags hierarchy

[Babu Moger]

resctrl uses RFTYPE flags for creating resctrl directory structure.

Definitions and directory structures are not documented. Add
comments to improve the readability and help future additions.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 arch/x86/kernel/cpu/resctrl/internal.h |   48 ++++++++++++++++++++++++++++++++
 1 file changed, 48 insertions(+)

diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 2051179a3b91..783c3e36633c 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -240,6 +240,54 @@ struct rdtgroup {
 
 /*
  * Define the file type flags for base and info directories.
+ *
+ * RESCTRL filesystem has two main components
+ *	a. info
+ *	b. base
+ *
+ * /sys/fs/resctrl/
+ *	|
+ *	--> info (Top level directory named "info". Contains files that
+ *	|	  provide details on control and monitoring resources.)
+ *	|
+ *	--> base (Root directory associated with default resource group
+ *		  as well as directories created by user for MON and CTRL
+ *		  groups. Contains files to interact with MON and CTRL
+ *		  groups.)
+ *
+ *	info directory structure
+ *	------------------------------------------------------------------
+ *	--> RFTYPE_INFO
+ *	--> <info> directory
+ *		--> RFTYPE_TOP_INFO
+ *		    Files: last_cmd_status
+ *
+ *		--> RFTYPE_MON_INFO
+ *		--> <L3_MON> directory
+ *		    Files: max_threshold_occupancy, mon_features,
+ *		           num_rmids, mbm_total_bytes_config,
+ *		           mbm_locat_bytes_config
+ *
+ *		--> RFTYPE_CTRL_INFO
+ *			--> RFTYPE_RES_CACHE
+ *			--> <L2,L3> directory
+ *			    Files: bit_usage, cbm_mask, min_cbm_bits,
+ *			           num_closids, shareable_bits
+ *
+ *			--> RFTYPE_RES_MB
+ *			--> <MB,SMBA> directory
+ *			    Files: bandwidth_gran, delay_linear,
+ * 			           min_bandwidth, num_closids,
+ *			           thread_throttle_mode
+ *
+ *	base (root) directory structure
+ *	------------------------------------------------------------------
+ *	--> RFTYPE_BASE (Files common for both MON and CTRL groups)
+ *	    Files: cpus, cpus_list, tasks
+ *
+ *	--> RFTYPE_CTRL_BASE (Files only for CTRL group)
+ *	    Files: mode, schemata, size
+ *
  */
 #define RFTYPE_INFO			BIT(0)
 #define RFTYPE_BASE			BIT(1)

[PATCH v6 5/8] x86/resctrl: Unwind the errors inside rdt_enable_ctx

[Babu Moger]

rdt_enable_ctx() takes care of enabling the features provided during
resctrl mount. The error unwinding of rdt_enable_ctx is done from the
caller rdt_get_tree. This is not ideal and can cause some error unwinding
to be omitted.

Fix this by moving all the error unwinding inside rdt_enable_ctx.

Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 arch/x86/kernel/cpu/resctrl/rdtgroup.c |   31 +++++++++++++++++++++++--------
 1 file changed, 23 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 3010e3a1394d..9a7204f71d2d 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2381,15 +2381,31 @@ 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;
+	}
 
-	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:
 	return ret;
 }
 
@@ -2497,13 +2513,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,10 +2578,9 @@ static int rdt_get_tree(struct fs_context *fc)
 	kernfs_remove(kn_info);
 out_schemata_free:
 	schemata_list_destroy();
-out_mba:
+out_ctx:
 	if (ctx->enable_mba_mbps)
 		set_mba_sc(false);
-out_cdp:
 	cdp_disable_all();
 out:
 	rdt_last_cmd_clear();

[PATCH v6 6/8] x86/resctrl: Move default control group creation during mount

[Babu Moger]

The resctrl default control group is created during kernel init time. If
the new files are to be added to the default group during the mount based
on the mount option, then each file needs to be created separately and
call kernfs_activate.

This can avoided if all the files are created during the mount and
destroyed during the umount. Move the root and default group creation
in rdt_get_tree and removal in rdt_kill_sb.

Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 arch/x86/kernel/cpu/resctrl/internal.h |    1 +
 arch/x86/kernel/cpu/resctrl/rdtgroup.c |   51 ++++++++++++++------------------
 2 files changed, 24 insertions(+), 28 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 783c3e36633c..885b656d1088 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -601,5 +601,6 @@ void rdt_domain_reconfigure_cdp(struct rdt_resource *r);
 void __init thread_throttle_mode_init(void);
 void __init mbm_config_rftype_init(const char *config);
 void rdt_staged_configs_clear(void);
+int rdtgroup_setup_root(void);
 
 #endif /* _ASM_X86_RESCTRL_INTERNAL_H */
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 9a7204f71d2d..e0570bce76a2 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2512,10 +2512,16 @@ static int rdt_get_tree(struct fs_context *fc)
 		goto out;
 	}
 
-	ret = rdt_enable_ctx(ctx);
+	ret = rdtgroup_setup_root();
 	if (ret)
 		goto out;
 
+	ctx->kfc.root = rdt_root;
+
+	ret = rdt_enable_ctx(ctx);
+	if (ret)
+		goto out_root;
+
 	ret = schemata_list_create();
 	if (ret) {
 		schemata_list_destroy();
@@ -2524,6 +2530,12 @@ static int rdt_get_tree(struct fs_context *fc)
 
 	closid_init();
 
+	ret = rdtgroup_add_files(rdtgroup_default.kn, RFTYPE_CTRL_BASE);
+	if (ret)
+		goto out_schemata_free;
+
+	kernfs_activate(rdtgroup_default.kn);
+
 	ret = rdtgroup_create_info_dir(rdtgroup_default.kn);
 	if (ret < 0)
 		goto out_schemata_free;
@@ -2582,6 +2594,9 @@ static int rdt_get_tree(struct fs_context *fc)
 	if (ctx->enable_mba_mbps)
 		set_mba_sc(false);
 	cdp_disable_all();
+out_root:
+	list_del(&rdtgroup_default.rdtgroup_list);
+	kernfs_destroy_root(rdt_root);
 out:
 	rdt_last_cmd_clear();
 	mutex_unlock(&rdtgroup_mutex);
@@ -2652,7 +2667,6 @@ static int rdt_init_fs_context(struct fs_context *fc)
 	if (!ctx)
 		return -ENOMEM;
 
-	ctx->kfc.root = rdt_root;
 	ctx->kfc.magic = RDTGROUP_SUPER_MAGIC;
 	fc->fs_private = &ctx->kfc;
 	fc->ops = &rdt_fs_context_ops;
@@ -2821,7 +2835,9 @@ static void rdt_kill_sb(struct super_block *sb)
 	cdp_disable_all();
 	rmdir_all_sub();
 	rdt_pseudo_lock_release();
-	rdtgroup_default.mode = RDT_MODE_SHAREABLE;
+	/* Remove the default group and cleanup the root */
+	list_del(&rdtgroup_default.rdtgroup_list);
+	kernfs_destroy_root(rdt_root);
 	schemata_list_destroy();
 	static_branch_disable_cpuslocked(&rdt_alloc_enable_key);
 	static_branch_disable_cpuslocked(&rdt_mon_enable_key);
@@ -3704,10 +3720,8 @@ static struct kernfs_syscall_ops rdtgroup_kf_syscall_ops = {
 	.show_options	= rdtgroup_show_options,
 };
 
-static int __init rdtgroup_setup_root(void)
+int rdtgroup_setup_root(void)
 {
-	int ret;
-
 	rdt_root = kernfs_create_root(&rdtgroup_kf_syscall_ops,
 				      KERNFS_ROOT_CREATE_DEACTIVATED |
 				      KERNFS_ROOT_EXTRA_OPEN_PERM_CHECK,
@@ -3715,28 +3729,17 @@ static int __init rdtgroup_setup_root(void)
 	if (IS_ERR(rdt_root))
 		return PTR_ERR(rdt_root);
 
-	mutex_lock(&rdtgroup_mutex);
-
 	rdtgroup_default.closid = 0;
 	rdtgroup_default.mon.rmid = 0;
 	rdtgroup_default.type = RDTCTRL_GROUP;
+	rdtgroup_default.mode = RDT_MODE_SHAREABLE;
 	INIT_LIST_HEAD(&rdtgroup_default.mon.crdtgrp_list);
 
 	list_add(&rdtgroup_default.rdtgroup_list, &rdt_all_groups);
 
-	ret = rdtgroup_add_files(kernfs_root_to_node(rdt_root), RFTYPE_CTRL_BASE);
-	if (ret) {
-		kernfs_destroy_root(rdt_root);
-		goto out;
-	}
-
 	rdtgroup_default.kn = kernfs_root_to_node(rdt_root);
-	kernfs_activate(rdtgroup_default.kn);
 
-out:
-	mutex_unlock(&rdtgroup_mutex);
-
-	return ret;
+	return 0;
 }
 
 static void domain_destroy_mon_state(struct rdt_domain *d)
@@ -3858,13 +3861,9 @@ int __init rdtgroup_init(void)
 	seq_buf_init(&last_cmd_status, last_cmd_status_buf,
 		     sizeof(last_cmd_status_buf));
 
-	ret = rdtgroup_setup_root();
-	if (ret)
-		return ret;
-
 	ret = sysfs_create_mount_point(fs_kobj, "resctrl");
 	if (ret)
-		goto cleanup_root;
+		return ret;
 
 	ret = register_filesystem(&rdt_fs_type);
 	if (ret)
@@ -3897,9 +3896,6 @@ int __init rdtgroup_init(void)
 
 cleanup_mountpoint:
 	sysfs_remove_mount_point(fs_kobj, "resctrl");
-cleanup_root:
-	kernfs_destroy_root(rdt_root);
-
 	return ret;
 }
 
@@ -3908,5 +3904,4 @@ void __exit rdtgroup_exit(void)
 	debugfs_remove_recursive(debugfs_resctrl);
 	unregister_filesystem(&rdt_fs_type);
 	sysfs_remove_mount_point(fs_kobj, "resctrl");
-	kernfs_destroy_root(rdt_root);
 }

[PATCH v6 7/8] x86/resctrl: Introduce "-o debug" mount option

[Babu Moger]

Add "-o debug" option to mount resctrl filesystem in debug mode. This
option is used for adding extra files to help resctrl debugging.

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 Documentation/arch/x86/resctrl.rst     |    5 ++++-
 arch/x86/kernel/cpu/resctrl/internal.h |    2 ++
 arch/x86/kernel/cpu/resctrl/rdtgroup.c |   20 ++++++++++++++++++++
 3 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
index af234681756e..5a2346d2c561 100644
--- a/Documentation/arch/x86/resctrl.rst
+++ b/Documentation/arch/x86/resctrl.rst
@@ -35,7 +35,7 @@ about the feature from resctrl's info directory.
 
 To use the feature mount the file system::
 
- # mount -t resctrl resctrl [-o cdp[,cdpl2][,mba_MBps]] /sys/fs/resctrl
+ # mount -t resctrl resctrl [-o cdp[,cdpl2][,mba_MBps][,debug]] /sys/fs/resctrl
 
 mount options are:
 
@@ -46,6 +46,9 @@ mount options are:
 "mba_MBps":
 	Enable the MBA Software Controller(mba_sc) to specify MBA
 	bandwidth in MBps
+"debug":
+	Make debug files accessible. Available debug files are annotated with
+	"Available only with debug option".
 
 L2 and L3 CDP are controlled separately.
 
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 885b656d1088..5bc18accb887 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -59,6 +59,7 @@ struct rdt_fs_context {
 	bool				enable_cdpl2;
 	bool				enable_cdpl3;
 	bool				enable_mba_mbps;
+	bool				enable_debug;
 };
 
 static inline struct rdt_fs_context *rdt_fc2context(struct fs_context *fc)
@@ -296,6 +297,7 @@ struct rdtgroup {
 #define RFTYPE_TOP			BIT(6)
 #define RFTYPE_RES_CACHE		BIT(8)
 #define RFTYPE_RES_MB			BIT(9)
+#define RFTYPE_DEBUG			BIT(10)
 #define RFTYPE_CTRL_INFO		(RFTYPE_INFO | RFTYPE_CTRL)
 #define RFTYPE_MON_INFO			(RFTYPE_INFO | RFTYPE_MON)
 #define RFTYPE_TOP_INFO			(RFTYPE_INFO | RFTYPE_TOP)
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index e0570bce76a2..058c1dedb2d7 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -56,6 +56,8 @@ static char last_cmd_status_buf[512];
 
 struct dentry *debugfs_resctrl;
 
+static bool resctrl_debug;
+
 void rdt_last_cmd_clear(void)
 {
 	lockdep_assert_held(&rdtgroup_mutex);
@@ -1871,6 +1873,9 @@ static int rdtgroup_add_files(struct kernfs_node *kn, unsigned long fflags)
 
 	lockdep_assert_held(&rdtgroup_mutex);
 
+	if (resctrl_debug)
+		fflags |= RFTYPE_DEBUG;
+
 	for (rft = rfts; rft < rfts + len; rft++) {
 		if (rft->fflags && ((fflags & rft->fflags) == rft->fflags)) {
 			ret = rdtgroup_add_file(kn, rft);
@@ -2399,6 +2404,9 @@ static int rdt_enable_ctx(struct rdt_fs_context *ctx)
 			goto out_cdpl3;
 	}
 
+	if (ctx->enable_debug)
+		resctrl_debug = true;
+
 	return 0;
 
 out_cdpl3:
@@ -2591,6 +2599,8 @@ static int rdt_get_tree(struct fs_context *fc)
 out_schemata_free:
 	schemata_list_destroy();
 out_ctx:
+	if (ctx->enable_debug)
+		resctrl_debug = false;
 	if (ctx->enable_mba_mbps)
 		set_mba_sc(false);
 	cdp_disable_all();
@@ -2608,6 +2618,7 @@ enum rdt_param {
 	Opt_cdp,
 	Opt_cdpl2,
 	Opt_mba_mbps,
+	Opt_debug,
 	nr__rdt_params
 };
 
@@ -2615,6 +2626,7 @@ static const struct fs_parameter_spec rdt_fs_parameters[] = {
 	fsparam_flag("cdp",		Opt_cdp),
 	fsparam_flag("cdpl2",		Opt_cdpl2),
 	fsparam_flag("mba_MBps",	Opt_mba_mbps),
+	fsparam_flag("debug",		Opt_debug),
 	{}
 };
 
@@ -2640,6 +2652,9 @@ static int rdt_parse_param(struct fs_context *fc, struct fs_parameter *param)
 			return -EINVAL;
 		ctx->enable_mba_mbps = true;
 		return 0;
+	case Opt_debug:
+		ctx->enable_debug = true;
+		return 0;
 	}
 
 	return -EINVAL;
@@ -2829,6 +2844,8 @@ static void rdt_kill_sb(struct super_block *sb)
 
 	set_mba_sc(false);
 
+	resctrl_debug = false;
+
 	/*Put everything back to default values. */
 	for_each_alloc_capable_rdt_resource(r)
 		reset_all_ctrls(r);
@@ -3710,6 +3727,9 @@ static int rdtgroup_show_options(struct seq_file *seq, struct kernfs_root *kf)
 	if (is_mba_sc(&rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl))
 		seq_puts(seq, ",mba_MBps");
 
+	if (resctrl_debug)
+		seq_puts(seq, ",debug");
+
 	return 0;
 }
 

[PATCH v6 8/8] x86/resctrl: Display hardware ids of resource groups

[Babu Moger]

In x86, hardware uses CLOSID and an RMID to identify a control group and
a monitoring group respectively. When a user creates a control or monitor
group these details are not visible to the user. These details can help
debugging.

Add CLOSID(ctrl_hw_id) and RMID(mon_hw_id) to the control/monitor groups
display in resctrl interface. Users can see these details when resctrl
is mounted with "-o debug" option.

Other architectures do not use "CLOSID" and "RMID". Use the names
ctrl_hw_id and mon_hw_id to refer "CLOSID" and "RMID" respectively in an
effort to keep the naming generic.

For example:
 $cat /sys/fs/resctrl/ctrl_grp1/ctrl_hw_id
 1
 $cat /sys/fs/resctrl/mon_groups/mon_grp1/mon_hw_id
 3

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 Documentation/arch/x86/resctrl.rst     |    8 ++++++
 arch/x86/kernel/cpu/resctrl/rdtgroup.c |   46 ++++++++++++++++++++++++++++++++
 2 files changed, 54 insertions(+)

diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
index 5a2346d2c561..41ad9b1f0c6a 100644
--- a/Documentation/arch/x86/resctrl.rst
+++ b/Documentation/arch/x86/resctrl.rst
@@ -351,6 +351,10 @@ When control is enabled all CTRL_MON groups will also contain:
 	file. On successful pseudo-locked region creation the mode will
 	automatically change to "pseudo-locked".
 
+"ctrl_hw_id":
+	Available only with debug option. The identifier used by hardware
+	for the control group. On x86 this is the CLOSID.
+
 When monitoring is enabled all MON groups will also contain:
 
 "mon_data":
@@ -364,6 +368,10 @@ When monitoring is enabled all MON groups will also contain:
 	the sum for all tasks in the CTRL_MON group and all tasks in
 	MON groups. Please see example section for more details on usage.
 
+"mon_hw_id":
+	Available only with debug option. The identifier used by hardware
+	for the monitor group. On x86 this is the RMID.
+
 Resource allocation rules
 -------------------------
 
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 058c1dedb2d7..e8b35013d7c1 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -776,6 +776,38 @@ static int rdtgroup_tasks_show(struct kernfs_open_file *of,
 	return ret;
 }
 
+static int rdtgroup_closid_show(struct kernfs_open_file *of,
+				struct seq_file *s, void *v)
+{
+	struct rdtgroup *rdtgrp;
+	int ret = 0;
+
+	rdtgrp = rdtgroup_kn_lock_live(of->kn);
+	if (rdtgrp)
+		seq_printf(s, "%u\n", rdtgrp->closid);
+	else
+		ret = -ENOENT;
+	rdtgroup_kn_unlock(of->kn);
+
+	return ret;
+}
+
+static int rdtgroup_rmid_show(struct kernfs_open_file *of,
+			      struct seq_file *s, void *v)
+{
+	struct rdtgroup *rdtgrp;
+	int ret = 0;
+
+	rdtgrp = rdtgroup_kn_lock_live(of->kn);
+	if (rdtgrp)
+		seq_printf(s, "%u\n", rdtgrp->mon.rmid);
+	else
+		ret = -ENOENT;
+	rdtgroup_kn_unlock(of->kn);
+
+	return ret;
+}
+
 #ifdef CONFIG_PROC_CPU_RESCTRL
 
 /*
@@ -1837,6 +1869,13 @@ static struct rftype res_common_files[] = {
 		.seq_show	= rdtgroup_tasks_show,
 		.fflags		= RFTYPE_BASE,
 	},
+	{
+		.name		= "mon_hw_id",
+		.mode		= 0444,
+		.kf_ops		= &rdtgroup_kf_single_ops,
+		.seq_show	= rdtgroup_rmid_show,
+		.fflags		= RFTYPE_BASE | RFTYPE_DEBUG,
+	},
 	{
 		.name		= "schemata",
 		.mode		= 0644,
@@ -1860,6 +1899,13 @@ static struct rftype res_common_files[] = {
 		.seq_show	= rdtgroup_size_show,
 		.fflags		= RFTYPE_CTRL_BASE,
 	},
+	{
+		.name		= "ctrl_hw_id",
+		.mode		= 0444,
+		.kf_ops		= &rdtgroup_kf_single_ops,
+		.seq_show	= rdtgroup_closid_show,
+		.fflags		= RFTYPE_CTRL_BASE | RFTYPE_DEBUG,
+	},
 
 };
 

RE: [PATCH v6 0/8] x86/resctrl: Miscellaneous resctrl features

[Shaopeng Tan (Fujitsu)]

Hi Babu,

I reviewed this patch series(v6) and it looks fine.
I tested these features and ran the selftests/resctrl test set,
and there is no problem.

<Reviewed-by:tan.shaopeng@jp.fujitsu.com>
<Tested-by:tan.shaopeng@jp.fujitsu.com>

Best regards,
Shaopeng TAN

Re: [PATCH v6 4/8] x86/resctrl: Add comments on RFTYPE flags hierarchy

[Reinette Chatre]

Hi Babu,

On 7/19/2023 4:21 PM, Babu Moger wrote:
> + *	------------------------------------------------------------------
> + *	--> RFTYPE_INFO
> + *	--> <info> directory
> + *		--> RFTYPE_TOP_INFO
> + *		    Files: last_cmd_status
> + *
> + *		--> RFTYPE_MON_INFO
> + *		--> <L3_MON> directory
> + *		    Files: max_threshold_occupancy, mon_features,
> + *		           num_rmids, mbm_total_bytes_config,
> + *		           mbm_locat_bytes_config

mbm_locat_bytes_config -> mbm_local_bytes_config

> + *
> + *		--> RFTYPE_CTRL_INFO
> + *			--> RFTYPE_RES_CACHE
> + *			--> <L2,L3> directory

Should this be "directories"?

> + *			    Files: bit_usage, cbm_mask, min_cbm_bits,
> + *			           num_closids, shareable_bits

Based on the hierarchy presented the files mentioned here may be expected
to be associated with RFTYPE_CTRL_INFO | RFTYPE_RES_CACHE.
For accuracy it may be better to move num_closids one level higher so
that it is only associated with RFTYPE_CTRL_INFO?

> + *
> + *			--> RFTYPE_RES_MB
> + *			--> <MB,SMBA> directory

directories?

> + *			    Files: bandwidth_gran, delay_linear,
> + * 			           min_bandwidth, num_closids,

Please remove space before tab for a clean checkpatch.pl run.

> + *			           thread_throttle_mode
> + *
> + *	base (root) directory structure

Since "base" refers to more than the root directory I think this can
just be "base directory structure".

> + *	------------------------------------------------------------------
> + *	--> RFTYPE_BASE (Files common for both MON and CTRL groups)
> + *	    Files: cpus, cpus_list, tasks
> + *
> + *	--> RFTYPE_CTRL_BASE (Files only for CTRL group)
> + *	    Files: mode, schemata, size
> + *
>   */
>  #define RFTYPE_INFO			BIT(0)
>  #define RFTYPE_BASE			BIT(1)
> 
> 

Reinette

Re: [PATCH v6 5/8] x86/resctrl: Unwind the errors inside rdt_enable_ctx

[Reinette Chatre]

Hi Babu,

On 7/19/2023 4:22 PM, Babu Moger wrote:
> rdt_enable_ctx() takes care of enabling the features provided during

"rdt_enable_ctx() takes care of enabling" can just be "rdt_enable_ctx()
enables"

> resctrl mount. The error unwinding of rdt_enable_ctx is done from the
> caller rdt_get_tree. This is not ideal and can cause some error unwinding
> to be omitted.
> 

Please consistently use () to indicate function names (in
changelog and subject).

"This is not ideal and can cause some error unwinding to be omitted."
is a bit vague. How about (in a new paragraph):
"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."

> Fix this by moving all the error unwinding inside rdt_enable_ctx.

"Fix" creates expectation for a "fixes" tag which is not needed here. This
refactors code to simplify future additions.

Even so, I do not think this solution addresses the stated problem (more
below).

> 
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |   31 +++++++++++++++++++++++--------
>  1 file changed, 23 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 3010e3a1394d..9a7204f71d2d 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -2381,15 +2381,31 @@ 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;
> +	}
>  
> -	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);

Be careful here. There is no dependency between L3 and L2 CDP ...
if L3 CDP was enabled it does not mean that L2 CDP was enabled also.
Similarly, if the software controller was enabled it does not mean that
CDP was also enabled.
Since resctrl_arch_set_cdp_enabled() does much more than just change
a flag value I think these should first check if it was enabled
before disabling the feature.

> +out:
>  	return ret;
>  }
>  
> @@ -2497,13 +2513,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,10 +2578,9 @@ static int rdt_get_tree(struct fs_context *fc)
>  	kernfs_remove(kn_info);
>  out_schemata_free:
>  	schemata_list_destroy();
> -out_mba:
> +out_ctx:
>  	if (ctx->enable_mba_mbps)
>  		set_mba_sc(false);
> -out_cdp:
>  	cdp_disable_all();
>  out:
>  	rdt_last_cmd_clear();
> 

The problem statement in the changelog was that rdt_get_tree() is
doing error unwinding of rdt_enable_ctx(). Looking at the above it
seems that the problem remains ... callers of rdt_enable_ctx()
still need to know all internals of that function to do error
unwind correctly. Could it perhaps be made simpler with a new
rdt_disable_ctx() that undoes rdt_enable_ctx()? New additions
to rdt_enable_ctx() would have more clarity where changes are
needed and callers only need to call a single rdt_disable_ctx().

Reinette

 

Re: [PATCH v6 6/8] x86/resctrl: Move default control group creation during mount

[Reinette Chatre]

Hi Babu,

On 7/19/2023 4:22 PM, Babu Moger wrote:
> The resctrl default control group is created during kernel init time. If
> the new files are to be added to the default group during the mount based
> on the mount option, then each file needs to be created separately and
> call kernfs_activate.
> 
> This can avoided if all the files are created during the mount and
> destroyed during the umount. Move the root and default group creation
> in rdt_get_tree and removal in rdt_kill_sb.

Please use () to indicate function names.

> 
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---

...

> @@ -3704,10 +3720,8 @@ static struct kernfs_syscall_ops rdtgroup_kf_syscall_ops = {
>  	.show_options	= rdtgroup_show_options,
>  };
>  
> -static int __init rdtgroup_setup_root(void)
> +int rdtgroup_setup_root(void)
>  {
> -	int ret;
> -
>  	rdt_root = kernfs_create_root(&rdtgroup_kf_syscall_ops,
>  				      KERNFS_ROOT_CREATE_DEACTIVATED |
>  				      KERNFS_ROOT_EXTRA_OPEN_PERM_CHECK,
> @@ -3715,28 +3729,17 @@ static int __init rdtgroup_setup_root(void)
>  	if (IS_ERR(rdt_root))
>  		return PTR_ERR(rdt_root);
>  
> -	mutex_lock(&rdtgroup_mutex);
> -
>  	rdtgroup_default.closid = 0;
>  	rdtgroup_default.mon.rmid = 0;
>  	rdtgroup_default.type = RDTCTRL_GROUP;
> +	rdtgroup_default.mode = RDT_MODE_SHAREABLE;
>  	INIT_LIST_HEAD(&rdtgroup_default.mon.crdtgrp_list);
>  
>  	list_add(&rdtgroup_default.rdtgroup_list, &rdt_all_groups);
>  
> -	ret = rdtgroup_add_files(kernfs_root_to_node(rdt_root), RFTYPE_CTRL_BASE);
> -	if (ret) {
> -		kernfs_destroy_root(rdt_root);
> -		goto out;
> -	}
> -
>  	rdtgroup_default.kn = kernfs_root_to_node(rdt_root);
> -	kernfs_activate(rdtgroup_default.kn);
>  
> -out:
> -	mutex_unlock(&rdtgroup_mutex);
> -
> -	return ret;
> +	return 0;
>  }

I am missing something here. Why is it now needed to re-initialize
and add default resource group on every mount of resctrl? I expected
only the kernfs related changes to move.

Reinette

Re: [PATCH v6 7/8] x86/resctrl: Introduce "-o debug" mount option

[Reinette Chatre]

Hi Babu,

On 7/19/2023 4:22 PM, Babu Moger wrote:
> @@ -2591,6 +2599,8 @@ static int rdt_get_tree(struct fs_context *fc)
>  out_schemata_free:
>  	schemata_list_destroy();
>  out_ctx:
> +	if (ctx->enable_debug)
> +		resctrl_debug = false;
>  	if (ctx->enable_mba_mbps)
>  		set_mba_sc(false);
>  	cdp_disable_all();

These changes are a red flag to me. Developers still need to
do what patch #4 was aiming to prevent.

Reinette

Re: [PATCH v6 4/8] x86/resctrl: Add comments on RFTYPE flags hierarchy

[Moger, Babu]

Hi Reinette,

On 8/4/23 15:39, Reinette Chatre wrote:
> Hi Babu,
> 
> On 7/19/2023 4:21 PM, Babu Moger wrote:
>> + *	------------------------------------------------------------------
>> + *	--> RFTYPE_INFO
>> + *	--> <info> directory
>> + *		--> RFTYPE_TOP_INFO
>> + *		    Files: last_cmd_status
>> + *
>> + *		--> RFTYPE_MON_INFO
>> + *		--> <L3_MON> directory
>> + *		    Files: max_threshold_occupancy, mon_features,
>> + *		           num_rmids, mbm_total_bytes_config,
>> + *		           mbm_locat_bytes_config
> 
> mbm_locat_bytes_config -> mbm_local_bytes_config

Good catch. Thanks

> 
>> + *
>> + *		--> RFTYPE_CTRL_INFO
>> + *			--> RFTYPE_RES_CACHE
>> + *			--> <L2,L3> directory
> 
> Should this be "directories"?

Yes.

> 
>> + *			    Files: bit_usage, cbm_mask, min_cbm_bits,
>> + *			           num_closids, shareable_bits
> 
> Based on the hierarchy presented the files mentioned here may be expected
> to be associated with RFTYPE_CTRL_INFO | RFTYPE_RES_CACHE.
> For accuracy it may be better to move num_closids one level higher so
> that it is only associated with RFTYPE_CTRL_INFO?
> 
>> + *
>> + *			--> RFTYPE_RES_MB
>> + *			--> <MB,SMBA> directory
> 
> directories?

Yes.

> 
>> + *			    Files: bandwidth_gran, delay_linear,
>> + * 			           min_bandwidth, num_closids,
> 
> Please remove space before tab for a clean checkpatch.pl run.

Sure.
> 
>> + *			           thread_throttle_mode
>> + *
>> + *	base (root) directory structure
> 
> Since "base" refers to more than the root directory I think this can
> just be "base directory structure".

Sure.

> 
>> + *	------------------------------------------------------------------
>> + *	--> RFTYPE_BASE (Files common for both MON and CTRL groups)
>> + *	    Files: cpus, cpus_list, tasks
>> + *
>> + *	--> RFTYPE_CTRL_BASE (Files only for CTRL group)
>> + *	    Files: mode, schemata, size
>> + *
>>   */
>>  #define RFTYPE_INFO			BIT(0)
>>  #define RFTYPE_BASE			BIT(1)
>>
>>
> 
> Reinette

-- 
Thanks
Babu Moger

Re: [PATCH v6 5/8] x86/resctrl: Unwind the errors inside rdt_enable_ctx

[Moger, Babu]

Hi Reinette,

On 8/4/23 15:41, Reinette Chatre wrote:
> Hi Babu,
> 
> On 7/19/2023 4:22 PM, Babu Moger wrote:
>> rdt_enable_ctx() takes care of enabling the features provided during
> 
> "rdt_enable_ctx() takes care of enabling" can just be "rdt_enable_ctx()
> enables"

Sure.

> 
>> resctrl mount. The error unwinding of rdt_enable_ctx is done from the
>> caller rdt_get_tree. This is not ideal and can cause some error unwinding
>> to be omitted.
>>
> 
> Please consistently use () to indicate function names (in
> changelog and subject).

Sure.

> 
> "This is not ideal and can cause some error unwinding to be omitted."
> is a bit vague. How about (in a new paragraph):
> "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."

Sure.

> 
>> Fix this by moving all the error unwinding inside rdt_enable_ctx.
> 
> "Fix" creates expectation for a "fixes" tag which is not needed here. This
> refactors code to simplify future additions.

Sure.
> 
> Even so, I do not think this solution addresses the stated problem (more
> below).
> 
>>
>> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |   31 +++++++++++++++++++++++--------
>>  1 file changed, 23 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 3010e3a1394d..9a7204f71d2d 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -2381,15 +2381,31 @@ 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;
>> +	}
>>  
>> -	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);
> 
> Be careful here. There is no dependency between L3 and L2 CDP ...
> if L3 CDP was enabled it does not mean that L2 CDP was enabled also.
> Similarly, if the software controller was enabled it does not mean that
> CDP was also enabled.
> Since resctrl_arch_set_cdp_enabled() does much more than just change
> a flag value I think these should first check if it was enabled
> before disabling the feature.

Yes. Agree.
> 
>> +out:
>>  	return ret;
>>  }
>>  
>> @@ -2497,13 +2513,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,10 +2578,9 @@ static int rdt_get_tree(struct fs_context *fc)
>>  	kernfs_remove(kn_info);
>>  out_schemata_free:
>>  	schemata_list_destroy();
>> -out_mba:
>> +out_ctx:
>>  	if (ctx->enable_mba_mbps)
>>  		set_mba_sc(false);
>> -out_cdp:
>>  	cdp_disable_all();
>>  out:
>>  	rdt_last_cmd_clear();
>>
> 
> The problem statement in the changelog was that rdt_get_tree() is
> doing error unwinding of rdt_enable_ctx(). Looking at the above it
> seems that the problem remains ... callers of rdt_enable_ctx()
> still need to know all internals of that function to do error
> unwind correctly. Could it perhaps be made simpler with a new
> rdt_disable_ctx() that undoes rdt_enable_ctx()? New additions
> to rdt_enable_ctx() would have more clarity where changes are
> needed and callers only need to call a single rdt_disable_ctx().
> 

Yes. We can do that.
-- 
Thanks
Babu Moger

Re: [PATCH v6 6/8] x86/resctrl: Move default control group creation during mount

[Moger, Babu]

Hi Reinette,

On 8/4/23 15:42, Reinette Chatre wrote:
> Hi Babu,
> 
> On 7/19/2023 4:22 PM, Babu Moger wrote:
>> The resctrl default control group is created during kernel init time. If
>> the new files are to be added to the default group during the mount based
>> on the mount option, then each file needs to be created separately and
>> call kernfs_activate.
>>
>> This can avoided if all the files are created during the mount and
>> destroyed during the umount. Move the root and default group creation
>> in rdt_get_tree and removal in rdt_kill_sb.
> 
> Please use () to indicate function names.

Sure.

> 
>>
>> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
> 
> ...
> 
>> @@ -3704,10 +3720,8 @@ static struct kernfs_syscall_ops rdtgroup_kf_syscall_ops = {
>>  	.show_options	= rdtgroup_show_options,
>>  };
>>  
>> -static int __init rdtgroup_setup_root(void)
>> +int rdtgroup_setup_root(void)
>>  {
>> -	int ret;
>> -
>>  	rdt_root = kernfs_create_root(&rdtgroup_kf_syscall_ops,
>>  				      KERNFS_ROOT_CREATE_DEACTIVATED |
>>  				      KERNFS_ROOT_EXTRA_OPEN_PERM_CHECK,
>> @@ -3715,28 +3729,17 @@ static int __init rdtgroup_setup_root(void)
>>  	if (IS_ERR(rdt_root))
>>  		return PTR_ERR(rdt_root);
>>  
>> -	mutex_lock(&rdtgroup_mutex);
>> -
>>  	rdtgroup_default.closid = 0;
>>  	rdtgroup_default.mon.rmid = 0;
>>  	rdtgroup_default.type = RDTCTRL_GROUP;
>> +	rdtgroup_default.mode = RDT_MODE_SHAREABLE;
>>  	INIT_LIST_HEAD(&rdtgroup_default.mon.crdtgrp_list);
>>  
>>  	list_add(&rdtgroup_default.rdtgroup_list, &rdt_all_groups);
>>  
>> -	ret = rdtgroup_add_files(kernfs_root_to_node(rdt_root), RFTYPE_CTRL_BASE);
>> -	if (ret) {
>> -		kernfs_destroy_root(rdt_root);
>> -		goto out;
>> -	}
>> -
>>  	rdtgroup_default.kn = kernfs_root_to_node(rdt_root);
>> -	kernfs_activate(rdtgroup_default.kn);
>>  
>> -out:
>> -	mutex_unlock(&rdtgroup_mutex);
>> -
>> -	return ret;
>> +	return 0;
>>  }
> 
> I am missing something here. Why is it now needed to re-initialize
> and add default resource group on every mount of resctrl? I expected
> only the kernfs related changes to move.

Yes. We can do that. I think I started with the previous version and went
that route. We dont have to change the default group initialization.
Will send new version soon.
Thanks
Babu Moger

Re: [PATCH v6 7/8] x86/resctrl: Introduce "-o debug" mount option

[Moger, Babu]

Hi Reinette,

On 8/4/23 15:42, Reinette Chatre wrote:
> Hi Babu,
> 
> On 7/19/2023 4:22 PM, Babu Moger wrote:
>> @@ -2591,6 +2599,8 @@ static int rdt_get_tree(struct fs_context *fc)
>>  out_schemata_free:
>>  	schemata_list_destroy();
>>  out_ctx:
>> +	if (ctx->enable_debug)
>> +		resctrl_debug = false;
>>  	if (ctx->enable_mba_mbps)
>>  		set_mba_sc(false);
>>  	cdp_disable_all();
> 
> These changes are a red flag to me. Developers still need to
> do what patch #4 was aiming to prevent.

I think you meant patch 5 (x86/resctrl: Unwind the errors inside
rdt_enable_ctx).

I can take care of this unwind part in rdt_disable_ctx() and call it here.
https://lore.kernel.org/lkml/9fd70ef3-ca90-65e3-4746-7d574bdd159b@intel.com/#t

Hope that is what you meant.

-- 
Thanks
Babu Moger

Re: [PATCH v6 0/8] x86/resctrl: Miscellaneous resctrl features

[Moger, Babu]

On 8/1/23 20:36, Shaopeng Tan (Fujitsu) wrote:
> Hi Babu,
> 
> I reviewed this patch series(v6) and it looks fine.
> I tested these features and ran the selftests/resctrl test set,
> and there is no problem.
> 
> <Reviewed-by:tan.shaopeng@jp.fujitsu.com>
> <Tested-by:tan.shaopeng@jp.fujitsu.com>
> 
Thank you.
Babu Moger

Re: [PATCH v6 7/8] x86/resctrl: Introduce "-o debug" mount option

[Reinette Chatre]

Hi Babu,

On 8/8/2023 9:29 AM, Moger, Babu wrote:
> On 8/4/23 15:42, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 7/19/2023 4:22 PM, Babu Moger wrote:
>>> @@ -2591,6 +2599,8 @@ static int rdt_get_tree(struct fs_context *fc)
>>>  out_schemata_free:
>>>  	schemata_list_destroy();
>>>  out_ctx:
>>> +	if (ctx->enable_debug)
>>> +		resctrl_debug = false;
>>>  	if (ctx->enable_mba_mbps)
>>>  		set_mba_sc(false);
>>>  	cdp_disable_all();
>>
>> These changes are a red flag to me. Developers still need to
>> do what patch #4 was aiming to prevent.
> 
> I think you meant patch 5 (x86/resctrl: Unwind the errors inside
> rdt_enable_ctx).
> 
> I can take care of this unwind part in rdt_disable_ctx() and call it here.
> https://lore.kernel.org/lkml/9fd70ef3-ca90-65e3-4746-7d574bdd159b@intel.com/#t
> 
> Hope that is what you meant.

It is. Thank you for reading what I meant to write.

Reinette
 

Re: [PATCH v6 4/8] x86/resctrl: Add comments on RFTYPE flags hierarchy

[Moger, Babu]

Reinette,

On 8/4/23 15:39, Reinette Chatre wrote:
> Hi Babu,
> 
> On 7/19/2023 4:21 PM, Babu Moger wrote:
>> + *	------------------------------------------------------------------
>> + *	--> RFTYPE_INFO
>> + *	--> <info> directory
>> + *		--> RFTYPE_TOP_INFO
>> + *		    Files: last_cmd_status
>> + *
>> + *		--> RFTYPE_MON_INFO
>> + *		--> <L3_MON> directory
>> + *		    Files: max_threshold_occupancy, mon_features,
>> + *		           num_rmids, mbm_total_bytes_config,
>> + *		           mbm_locat_bytes_config
> 
> mbm_locat_bytes_config -> mbm_local_bytes_config
> 
>> + *
>> + *		--> RFTYPE_CTRL_INFO
>> + *			--> RFTYPE_RES_CACHE
>> + *			--> <L2,L3> directory
> 
> Should this be "directories"?
> 
>> + *			    Files: bit_usage, cbm_mask, min_cbm_bits,
>> + *			           num_closids, shareable_bits
> 
> Based on the hierarchy presented the files mentioned here may be expected
> to be associated with RFTYPE_CTRL_INFO | RFTYPE_RES_CACHE.
> For accuracy it may be better to move num_closids one level higher so
> that it is only associated with RFTYPE_CTRL_INFO?

Missed this earlier. Sure.

> 
>> + *
>> + *			--> RFTYPE_RES_MB
>> + *			--> <MB,SMBA> directory
> 
> directories?
> 
>> + *			    Files: bandwidth_gran, delay_linear,
>> + * 			           min_bandwidth, num_closids,
> 
> Please remove space before tab for a clean checkpatch.pl run.
> 
>> + *			           thread_throttle_mode
>> + *
>> + *	base (root) directory structure
> 
> Since "base" refers to more than the root directory I think this can
> just be "base directory structure".
> 
>> + *	------------------------------------------------------------------
>> + *	--> RFTYPE_BASE (Files common for both MON and CTRL groups)
>> + *	    Files: cpus, cpus_list, tasks
>> + *
>> + *	--> RFTYPE_CTRL_BASE (Files only for CTRL group)
>> + *	    Files: mode, schemata, size
>> + *
>>   */
>>  #define RFTYPE_INFO			BIT(0)
>>  #define RFTYPE_BASE			BIT(1)
>>
>>
> 
> Reinette

-- 
Thanks
Babu Moger