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

[PATCH v7 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.
---
v7:
   Changes since v6:
   While moving the default group file creation on mount, I also moved the
   initialization of default group data structures. Reinette suggested to move
   only the filesystem creation and keep the group initialization as is. Addressed it now.
   Added a new function rdt_disable_ctx to unwind the context related features.
   Few other minor text changes.

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. 

v6: https://lore.kernel.org/lkml/168980872063.1619861.420806535295905172.stgit@bmoger-ubuntu/
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 |  71 ++++++--
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 230 ++++++++++++++++++-------
 3 files changed, 251 insertions(+), 71 deletions(-)
--

[PATCH v7 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

Reviewed-by: Tan Shaopeng <tan.shaopeng@jp.fujitsu.com>
Tested-by: Tan Shaopeng <tan.shaopeng@jp.fujitsu.com>
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 v7 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>
Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Babu Moger <babu.moger@amd.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 v7 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>
Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Babu Moger <babu.moger@amd.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 v7 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 |   49 ++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 2051179a3b91..37800724e002 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -240,6 +240,55 @@ 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_local_bytes_config
+ *
+ *		--> RFTYPE_CTRL_INFO
+ *		    Files: num_closids
+ *
+ *			--> RFTYPE_RES_CACHE
+ *			--> <L2,L3> directories
+ *			    Files: bit_usage, cbm_mask, min_cbm_bits,
+ *				   shareable_bits
+ *
+ *			--> RFTYPE_RES_MB
+ *			--> <MB,SMBA> directories
+ *			    Files: bandwidth_gran, delay_linear,
+ *				   min_bandwidth, thread_throttle_mode
+ *
+ *	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)

[PATCH v7 5/8] x86/resctrl: Unwind the errors inside rdt_enable_ctx()

[Babu Moger]

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);

[PATCH v7 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 based on the mount
option, then each file needs to be created separately and call
kernfs_activate() again.

This can be avoided if all the files are created during the mount and
destroyed during the umount. So, move the 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 |   50 +++++++++++++++++---------------
 2 files changed, 27 insertions(+), 24 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 37800724e002..2bd92c0c3b0c 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -602,5 +602,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(struct rdt_fs_context *ctx);
 
 #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 0805fac04401..a7453c93bad4 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2521,10 +2521,14 @@ static int rdt_get_tree(struct fs_context *fc)
 		goto out;
 	}
 
-	ret = rdt_enable_ctx(ctx);
+	ret = rdtgroup_setup_root(ctx);
 	if (ret)
 		goto out;
 
+	ret = rdt_enable_ctx(ctx);
+	if (ret)
+		goto out_root;
+
 	ret = schemata_list_create();
 	if (ret) {
 		schemata_list_destroy();
@@ -2533,6 +2537,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;
@@ -2589,6 +2599,8 @@ static int rdt_get_tree(struct fs_context *fc)
 	schemata_list_destroy();
 out_ctx:
 	rdt_disable_ctx(ctx);
+out_root:
+	kernfs_destroy_root(rdt_root);
 out:
 	rdt_last_cmd_clear();
 	mutex_unlock(&rdtgroup_mutex);
@@ -2659,7 +2671,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;
@@ -2830,6 +2841,7 @@ static void rdt_kill_sb(struct super_block *sb)
 	rdt_pseudo_lock_release();
 	rdtgroup_default.mode = RDT_MODE_SHAREABLE;
 	schemata_list_destroy();
+	kernfs_destroy_root(rdt_root);
 	static_branch_disable_cpuslocked(&rdt_alloc_enable_key);
 	static_branch_disable_cpuslocked(&rdt_mon_enable_key);
 	static_branch_disable_cpuslocked(&rdt_enable_key);
@@ -3711,10 +3723,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(struct rdt_fs_context *ctx)
 {
-	int ret;
-
 	rdt_root = kernfs_create_root(&rdtgroup_kf_syscall_ops,
 				      KERNFS_ROOT_CREATE_DEACTIVATED |
 				      KERNFS_ROOT_EXTRA_OPEN_PERM_CHECK,
@@ -3722,6 +3732,15 @@ static int __init rdtgroup_setup_root(void)
 	if (IS_ERR(rdt_root))
 		return PTR_ERR(rdt_root);
 
+	ctx->kfc.root = rdt_root;
+
+	rdtgroup_default.kn = kernfs_root_to_node(rdt_root);
+
+	return 0;
+}
+
+static void __init rdtgroup_setup_default(void)
+{
 	mutex_lock(&rdtgroup_mutex);
 
 	rdtgroup_default.closid = 0;
@@ -3731,19 +3750,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), 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;
 }
 
 static void domain_destroy_mon_state(struct rdt_domain *d)
@@ -3865,13 +3872,11 @@ 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;
+	rdtgroup_setup_default();
 
 	ret = sysfs_create_mount_point(fs_kobj, "resctrl");
 	if (ret)
-		goto cleanup_root;
+		return ret;
 
 	ret = register_filesystem(&rdt_fs_type);
 	if (ret)
@@ -3904,8 +3909,6 @@ int __init rdtgroup_init(void)
 
 cleanup_mountpoint:
 	sysfs_remove_mount_point(fs_kobj, "resctrl");
-cleanup_root:
-	kernfs_destroy_root(rdt_root);
 
 	return ret;
 }
@@ -3915,5 +3918,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 v7 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 |   21 +++++++++++++++++++++
 3 files changed, 27 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 2bd92c0c3b0c..4689e87ec638 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)
@@ -297,6 +298,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 a7453c93bad4..6b7e914657fa 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);
@@ -2387,6 +2392,9 @@ static void rdt_disable_ctx(struct rdt_fs_context *ctx)
 
 	if (ctx->enable_mba_mbps)
 		set_mba_sc(false);
+
+	if (ctx->enable_debug)
+		resctrl_debug = false;
 }
 
 static int rdt_enable_ctx(struct rdt_fs_context *ctx)
@@ -2411,6 +2419,9 @@ static int rdt_enable_ctx(struct rdt_fs_context *ctx)
 			goto out_disable;
 	}
 
+	if (ctx->enable_debug)
+		resctrl_debug = true;
+
 	return 0;
 
 out_disable:
@@ -2612,6 +2623,7 @@ enum rdt_param {
 	Opt_cdp,
 	Opt_cdpl2,
 	Opt_mba_mbps,
+	Opt_debug,
 	nr__rdt_params
 };
 
@@ -2619,6 +2631,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),
 	{}
 };
 
@@ -2644,6 +2657,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;
@@ -2833,6 +2849,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);
@@ -3713,6 +3731,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 v7 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

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>
---
 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 6b7e914657fa..94471ad9d905 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 v7 1/8] x86/resctrl: Add multiple tasks to the resctrl group at once

[Reinette Chatre]

Hi Babu,

On 8/11/2023 1:08 PM, Babu Moger wrote:
> 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
> 
> Reviewed-by: Tan Shaopeng <tan.shaopeng@jp.fujitsu.com>
> Tested-by: Tan Shaopeng <tan.shaopeng@jp.fujitsu.com>
> Signed-off-by: Babu Moger <babu.moger@amd.com>

Thank you.

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>

Reinette

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

[Reinette Chatre]

Hi Babu,

On 8/11/2023 1:09 PM, Babu Moger wrote:
> 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 |   49 ++++++++++++++++++++++++++++++++
>  1 file changed, 49 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 2051179a3b91..37800724e002 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -240,6 +240,55 @@ 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_local_bytes_config
> + *

I think the monitor files need the same split as what you did for
control files in this version. That is, there are some files
that depend on RFTYPE_MON_INFO and others that depend on
RFTYPE_MON_INFO | RFTYPE_RES_CACHE. In above it appears that
all files depend on RFTYPE_MON_INFO only.

> + *		--> RFTYPE_CTRL_INFO
> + *		    Files: num_closids
> + *

Looking at this closer I can see some potential confusion about where these
files appear. A note saying that these files appear in all sub-directories
may be helpful because at this point it appears that this file is at the
same level as the directories.

> + *			--> RFTYPE_RES_CACHE
> + *			--> <L2,L3> directories
> + *			    Files: bit_usage, cbm_mask, min_cbm_bits,
> + *				   shareable_bits
> + *
> + *			--> RFTYPE_RES_MB
> + *			--> <MB,SMBA> directories
> + *			    Files: bandwidth_gran, delay_linear,
> + *				   min_bandwidth, thread_throttle_mode
> + *
> + *	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 v7 5/8] x86/resctrl: Unwind the errors inside rdt_enable_ctx()

[Reinette Chatre]

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

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

[Reinette Chatre]

Hi Babu,

Please do note that the subject is no longer accurate.

On 8/11/2023 1:10 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 based on the mount
> option, then each file needs to be created separately and call
> kernfs_activate() again.
> 
> This can be avoided if all the files are created during the mount and
> destroyed during the umount. So, move the default group creation
> in rdt_get_tree() and removal in rdt_kill_sb().

How about a slight rewording (please feel free to change):

  The default resource group and its files are created during kernel
  init time. Upcoming changes will make some resctrl files optional
  based on a mount parameter. If optional files are to be added to the
  default group based on the mount option, then each new file needs to
  be created separately and call kernfs_activate() again.

  Create all files of the default resource group during resctrl
  mount, destroyed during unmount, to avoid scattering resctrl
  file addition across two separate code flows.


> 
> 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 |   50 +++++++++++++++++---------------
>  2 files changed, 27 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 37800724e002..2bd92c0c3b0c 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -602,5 +602,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(struct rdt_fs_context *ctx);
>  
>  #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 0805fac04401..a7453c93bad4 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -2521,10 +2521,14 @@ static int rdt_get_tree(struct fs_context *fc)
>  		goto out;
>  	}
>  
> -	ret = rdt_enable_ctx(ctx);
> +	ret = rdtgroup_setup_root(ctx);
>  	if (ret)
>  		goto out;
>  
> +	ret = rdt_enable_ctx(ctx);
> +	if (ret)
> +		goto out_root;
> +
>  	ret = schemata_list_create();
>  	if (ret) {
>  		schemata_list_destroy();
> @@ -2533,6 +2537,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;
> @@ -2589,6 +2599,8 @@ static int rdt_get_tree(struct fs_context *fc)
>  	schemata_list_destroy();
>  out_ctx:
>  	rdt_disable_ctx(ctx);
> +out_root:
> +	kernfs_destroy_root(rdt_root);

Please ensure that all rdtgroup_setup_root() cleanup is done
here. It seems to me that rdtgroup_default.kn can be left pointing
to freed memory. Since this cleanup is done multiple places (here
and rdt_kill_sb()) it may help to create a helper.

Apart from this the patch looks good to me.

Thank you.

Reinette

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

[Reinette Chatre]

Hi Babu,

On 8/11/2023 1:10 PM, Babu Moger wrote:

...

> @@ -2833,6 +2849,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);

This should not be necessary as rdt_disable_ctx() should already
be called here.

> @@ -3713,6 +3731,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;
>  }
>  
> 
> 

Reinette

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

[Reinette Chatre]

Hi Babu,

On 8/11/2023 1:10 PM, Babu Moger wrote:
> 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

"to refer" -> "to refer to"

> 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
> 
> 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>
> ---
>  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 6b7e914657fa..94471ad9d905 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,
> +	},
>  
>  };
>  
> 
> 

I think the comments introduced in patch #4 may need to be updated
in this patch.

Apart from that this patch looks good to me.

Reinette

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

[Moger, Babu]

Hi Reinette,

On 8/15/23 17:45, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/11/2023 1:09 PM, Babu Moger wrote:
>> 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 |   49 ++++++++++++++++++++++++++++++++
>>  1 file changed, 49 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 2051179a3b91..37800724e002 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -240,6 +240,55 @@ 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_local_bytes_config
>> + *
> 
> I think the monitor files need the same split as what you did for
> control files in this version. That is, there are some files
> that depend on RFTYPE_MON_INFO and others that depend on
> RFTYPE_MON_INFO | RFTYPE_RES_CACHE. In above it appears that
> all files depend on RFTYPE_MON_INFO only.

ok. Sure.


>> + *		--> RFTYPE_CTRL_INFO
>> + *		    Files: num_closids
>> + *
> 
> Looking at this closer I can see some potential confusion about where these
> files appear. A note saying that these files appear in all sub-directories
> may be helpful because at this point it appears that this file is at the
> same level as the directories.

Sure..

With both these changes. Here is the diff on top of current patch.

diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
b/arch/x86/kernel/cpu/resctrl/internal.h
index 37800724e002..412a9ef98171 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -264,12 +264,16 @@ struct rdtgroup {
  *
  *             --> RFTYPE_MON_INFO
  *             --> <L3_MON> directory
- *                 Files: max_threshold_occupancy, mon_features,
- *                        num_rmids, mbm_total_bytes_config,
- *                        mbm_local_bytes_config
+ *                 Files: mon_features, num_rmids
+ *
+ *                     --> RFTYPE_RES_CACHE
+ *                         Files: max_threshold_occupancy,
+ *                                mbm_total_bytes_config,
+ *                                mbm_local_bytes_config
  *
  *             --> RFTYPE_CTRL_INFO
  *                 Files: num_closids
+ *                        These files appear in all the sub-directories.
  *
  *                     --> RFTYPE_RES_CACHE
  *                     --> <L2,L3> directories


Thanks
Babu

> 
>> + *			--> RFTYPE_RES_CACHE
>> + *			--> <L2,L3> directories
>> + *			    Files: bit_usage, cbm_mask, min_cbm_bits,
>> + *				   shareable_bits
>> + *
>> + *			--> RFTYPE_RES_MB
>> + *			--> <MB,SMBA> directories
>> + *			    Files: bandwidth_gran, delay_linear,
>> + *				   min_bandwidth, thread_throttle_mode
>> + *
>> + *	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

Re: [PATCH v7 5/8] x86/resctrl: Unwind the errors inside rdt_enable_ctx()

[Moger, Babu]

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

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

[Reinette Chatre]

Hi Babu,

On 8/16/2023 8:34 AM, Moger, Babu wrote:
> Hi Reinette,
> 
> On 8/15/23 17:45, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 8/11/2023 1:09 PM, Babu Moger wrote:
>>> 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 |   49 ++++++++++++++++++++++++++++++++
>>>  1 file changed, 49 insertions(+)
>>>
>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>>> index 2051179a3b91..37800724e002 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>> @@ -240,6 +240,55 @@ 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_local_bytes_config
>>> + *
>>
>> I think the monitor files need the same split as what you did for
>> control files in this version. That is, there are some files
>> that depend on RFTYPE_MON_INFO and others that depend on
>> RFTYPE_MON_INFO | RFTYPE_RES_CACHE. In above it appears that
>> all files depend on RFTYPE_MON_INFO only.
> 
> ok. Sure.
> 
> 
>>> + *		--> RFTYPE_CTRL_INFO
>>> + *		    Files: num_closids
>>> + *
>>
>> Looking at this closer I can see some potential confusion about where these
>> files appear. A note saying that these files appear in all sub-directories
>> may be helpful because at this point it appears that this file is at the
>> same level as the directories.
> 
> Sure..
> 
> With both these changes. Here is the diff on top of current patch.
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
> b/arch/x86/kernel/cpu/resctrl/internal.h
> index 37800724e002..412a9ef98171 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -264,12 +264,16 @@ struct rdtgroup {
>   *
>   *             --> RFTYPE_MON_INFO
>   *             --> <L3_MON> directory

I understand that resctrl does not use flags for directories
but I do find it inconsistent how the L3_MON directory is
layered when compared to how the, for example, L3 directory
is layered below. I actually start to wonder if it may not
simplify interpretation if the names of directories
are removed entirely from this documentation

I also think that the current hierarchy is confusing since it
uses combined flags while also creating a hierarchy.
What I mean is, the documentation looks like;

      *	--> RFTYPE_INFO
      *	--> <info> directory
      *		--> RFTYPE_TOP_INFO
      *		    Files: last_cmd_status

If I understand correctly the hierarchy is intended to mean
that all items below flag at that level has that flag set.
The confusing part is when combined flags are also used, like
above where RFTYPE_TOP_INFO is (RFTYPE_INFO | RFTYPE_TOP)
If hierarchy is followed, should it not rather be:

      *	--> RFTYPE_INFO
      *	--> <info> directory
      *		--> RFTYPE_TOP
      *		    Files: last_cmd_status



> - *                 Files: max_threshold_occupancy, mon_features,
> - *                        num_rmids, mbm_total_bytes_config,
> - *                        mbm_local_bytes_config
> + *                 Files: mon_features, num_rmids
> + *
> + *                     --> RFTYPE_RES_CACHE
> + *                         Files: max_threshold_occupancy,
> + *                                mbm_total_bytes_config,
> + *                                mbm_local_bytes_config
>   *
>   *             --> RFTYPE_CTRL_INFO
>   *                 Files: num_closids
> + *                        These files appear in all the sub-directories.
>   *
>   *                     --> RFTYPE_RES_CACHE
>   *                     --> <L2,L3> directories


I made an attempt at capturing all the items I mention
above. Please do not just copy this into your next version but
consider first if it makes sense to you with the goal of
reducing ambiguity.

*	info directory structure
*	------------------------------------------------------------------
*	--> RFTYPE_INFO
*		--> RFTYPE_TOP (Files in top level of info directory)
*		    Files: last_cmd_status
*
*		--> RFTYPE_MON (Files for all monitoring resources)
*		    Files: mon_features, num_rmids
*
*		    --> RFTYPE_RES_CACHE (Files for cache monitoring resources)
*		    Files: max_threshold_occupancy,
*			   mbm_total_bytes_config,
*			   mbm_local_bytes_config
*
*		--> RFTYPE_CTRL (Files for all control resources)
*		    Files: num_closids
*
*			--> RFTYPE_RES_CACHE (Files for cache control resources)
*			    Files: bit_usage, cbm_mask, min_cbm_bits,
*				   shareable_bits
*
*			--> RFTYPE_RES_MB (Files for MBA and SMBA control resources)
*			    Files: bandwidth_gran, delay_linear,
*				   min_bandwidth, thread_throttle_mode
*
*	base directory structure
*	------------------------------------------------------------------
*	--> RFTYPE_BASE (Files for both MON and CTRL groups)
*	    Files: cpus, cpus_list, tasks
*
*		--> RFTYPE_CTRL (Files only for CTRL group)
*	    	Files: mode, schemata, size
*

Reinette

Re: [PATCH v7 5/8] x86/resctrl: Unwind the errors inside rdt_enable_ctx()

[Reinette Chatre]

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

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

[Moger, Babu]

Hi Reinette,

On 8/15/23 17:50, Reinette Chatre wrote:
> Hi Babu,
> 
> Please do note that the subject is no longer accurate.

Changing to

x86/resctrl: Move default group file creation during mount

> 
> On 8/11/2023 1:10 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 based on the mount
>> option, then each file needs to be created separately and call
>> kernfs_activate() again.
>>
>> This can be avoided if all the files are created during the mount and
>> destroyed during the umount. So, move the default group creation
>> in rdt_get_tree() and removal in rdt_kill_sb().
> 
> How about a slight rewording (please feel free to change):
> 
>   The default resource group and its files are created during kernel
>   init time. Upcoming changes will make some resctrl files optional
>   based on a mount parameter. If optional files are to be added to the
>   default group based on the mount option, then each new file needs to
>   be created separately and call kernfs_activate() again.
> 
>   Create all files of the default resource group during resctrl
>   mount, destroyed during unmount, to avoid scattering resctrl
>   file addition across two separate code flows.

Sure. Looks good. thanks

> 
> 
>>
>> 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 |   50 +++++++++++++++++---------------
>>  2 files changed, 27 insertions(+), 24 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 37800724e002..2bd92c0c3b0c 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -602,5 +602,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(struct rdt_fs_context *ctx);
>>  
>>  #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 0805fac04401..a7453c93bad4 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -2521,10 +2521,14 @@ static int rdt_get_tree(struct fs_context *fc)
>>  		goto out;
>>  	}
>>  
>> -	ret = rdt_enable_ctx(ctx);
>> +	ret = rdtgroup_setup_root(ctx);
>>  	if (ret)
>>  		goto out;
>>  
>> +	ret = rdt_enable_ctx(ctx);
>> +	if (ret)
>> +		goto out_root;
>> +
>>  	ret = schemata_list_create();
>>  	if (ret) {
>>  		schemata_list_destroy();
>> @@ -2533,6 +2537,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;
>> @@ -2589,6 +2599,8 @@ static int rdt_get_tree(struct fs_context *fc)
>>  	schemata_list_destroy();
>>  out_ctx:
>>  	rdt_disable_ctx(ctx);
>> +out_root:
>> +	kernfs_destroy_root(rdt_root);
> 
> Please ensure that all rdtgroup_setup_root() cleanup is done
> here. It seems to me that rdtgroup_default.kn can be left pointing
> to freed memory. Since this cleanup is done multiple places (here
> and rdt_kill_sb()) it may help to create a helper.

Ok. Sure. Will do.

-- 
Thanks
Babu Moger

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

[Moger, Babu]

Hi Reinette,

On 8/15/23 17:51, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/11/2023 1:10 PM, Babu Moger wrote:
> 
> ...
> 
>> @@ -2833,6 +2849,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);
> 
> This should not be necessary as rdt_disable_ctx() should already
> be called here.

Yes. Sure.

-- 
Thanks
Babu Moger

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

[Reinette Chatre]

Hi Babu,

On 8/16/2023 12:14 PM, Moger, Babu wrote:
> Hi Reinette,
> 
> On 8/15/23 17:50, Reinette Chatre wrote:
>> Hi Babu,
>>
>> Please do note that the subject is no longer accurate.
> 
> Changing to
> 
> x86/resctrl: Move default group file creation during mount
> 

How about:
	x86/resctrl: Move default group file creation to mount

Reinette

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

[Moger, Babu]

Hi Reinette,

On 8/16/23 13:36, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/16/2023 8:34 AM, Moger, Babu wrote:
>> Hi Reinette,
>>
>> On 8/15/23 17:45, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 8/11/2023 1:09 PM, Babu Moger wrote:
>>>> 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 |   49 ++++++++++++++++++++++++++++++++
>>>>  1 file changed, 49 insertions(+)
>>>>
>>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>>>> index 2051179a3b91..37800724e002 100644
>>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>>> @@ -240,6 +240,55 @@ 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_local_bytes_config
>>>> + *
>>>
>>> I think the monitor files need the same split as what you did for
>>> control files in this version. That is, there are some files
>>> that depend on RFTYPE_MON_INFO and others that depend on
>>> RFTYPE_MON_INFO | RFTYPE_RES_CACHE. In above it appears that
>>> all files depend on RFTYPE_MON_INFO only.
>>
>> ok. Sure.
>>
>>
>>>> + *		--> RFTYPE_CTRL_INFO
>>>> + *		    Files: num_closids
>>>> + *
>>>
>>> Looking at this closer I can see some potential confusion about where these
>>> files appear. A note saying that these files appear in all sub-directories
>>> may be helpful because at this point it appears that this file is at the
>>> same level as the directories.
>>
>> Sure..
>>
>> With both these changes. Here is the diff on top of current patch.
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
>> b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 37800724e002..412a9ef98171 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -264,12 +264,16 @@ struct rdtgroup {
>>   *
>>   *             --> RFTYPE_MON_INFO
>>   *             --> <L3_MON> directory
> 
> I understand that resctrl does not use flags for directories
> but I do find it inconsistent how the L3_MON directory is
> layered when compared to how the, for example, L3 directory
> is layered below. I actually start to wonder if it may not
> simplify interpretation if the names of directories
> are removed entirely from this documentation

Yes. I agree. But I was thinking of adding notes about directory may be
helpful. See the patch below.

> 
> I also think that the current hierarchy is confusing since it
> uses combined flags while also creating a hierarchy.
> What I mean is, the documentation looks like;
> 
>       *	--> RFTYPE_INFO
>       *	--> <info> directory
>       *		--> RFTYPE_TOP_INFO
>       *		    Files: last_cmd_status
> 
> If I understand correctly the hierarchy is intended to mean
> that all items below flag at that level has that flag set.
> The confusing part is when combined flags are also used, like
> above where RFTYPE_TOP_INFO is (RFTYPE_INFO | RFTYPE_TOP)
> If hierarchy is followed, should it not rather be:
> 
>       *	--> RFTYPE_INFO
>       *	--> <info> directory
>       *		--> RFTYPE_TOP
>       *		    Files: last_cmd_status
> 

Yes. Agree. This makes more sense.
> 
> 
>> - *                 Files: max_threshold_occupancy, mon_features,
>> - *                        num_rmids, mbm_total_bytes_config,
>> - *                        mbm_local_bytes_config
>> + *                 Files: mon_features, num_rmids
>> + *
>> + *                     --> RFTYPE_RES_CACHE
>> + *                         Files: max_threshold_occupancy,
>> + *                                mbm_total_bytes_config,
>> + *                                mbm_local_bytes_config
>>   *
>>   *             --> RFTYPE_CTRL_INFO
>>   *                 Files: num_closids
>> + *                        These files appear in all the sub-directories.
>>   *
>>   *                     --> RFTYPE_RES_CACHE
>>   *                     --> <L2,L3> directories
> 
> 
> I made an attempt at capturing all the items I mention
> above. Please do not just copy this into your next version but
> consider first if it makes sense to you with the goal of
> reducing ambiguity.
> 
> *	info directory structure
> *	------------------------------------------------------------------
> *	--> RFTYPE_INFO
> *		--> RFTYPE_TOP (Files in top level of info directory)
> *		    Files: last_cmd_status
> *
> *		--> RFTYPE_MON (Files for all monitoring resources)
> *		    Files: mon_features, num_rmids
> *
> *		    --> RFTYPE_RES_CACHE (Files for cache monitoring resources)
> *		    Files: max_threshold_occupancy,
> *			   mbm_total_bytes_config,
> *			   mbm_local_bytes_config
> *
> *		--> RFTYPE_CTRL (Files for all control resources)
> *		    Files: num_closids
> *
> *			--> RFTYPE_RES_CACHE (Files for cache control resources)
> *			    Files: bit_usage, cbm_mask, min_cbm_bits,
> *				   shareable_bits
> *
> *			--> RFTYPE_RES_MB (Files for MBA and SMBA control resources)
> *			    Files: bandwidth_gran, delay_linear,
> *				   min_bandwidth, thread_throttle_mode
> *
> *	base directory structure
> *	------------------------------------------------------------------
> *	--> RFTYPE_BASE (Files for both MON and CTRL groups)
> *	    Files: cpus, cpus_list, tasks
> *
> *		--> RFTYPE_CTRL (Files only for CTRL group)
> *	    	Files: mode, schemata, size
> *

Yea. We can go with it. How about adding a little note about directories?
That might be easy to compare directory structure on a systems with these
flags. Something like this.

+ *
+ * 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.)
+ *
+ *     Note: resctrl does not use flags for directories. Directories are
+ *           created based on the resource type. Added directories below
+ *           for better understanding.
+ *
+ *     info directory structure
+ *     ------------------------------------------------------------------
+ *     --> RFTYPE_INFO
+ *         directory: info
+ *             --> RFTYPE_TOP (Files in top level of info directory)
+ *                 Files: last_cmd_status
+ *
+ *             --> RFTYPE_MON (Files for all monitoring resources)
+ *                 directory: L3_MON
+ *                     Files: mon_features, num_rmids
+ *
+ *                     --> RFTYPE_RES_CACHE (Files for cache monitoring
resources)
+ *                         Files: max_threshold_occupancy,
+ *                                mbm_total_bytes_config,
+ *                                mbm_local_bytes_config
+ *
+ *             --> RFTYPE_CTRL (Files for all control resources)
+ *                 Files: num_closids
+ *
+ *                     --> RFTYPE_RES_CACHE (Files for cache control
resources)
+ *                         directories: L2,L3
+ *                               Files: bit_usage, cbm_mask, min_cbm_bits,
+ *                                      shareable_bits
+ *
+ *                     --> RFTYPE_RES_MB (Files for memory control resources)
+ *                         directories: MB,SMBA
+ *                               Files: bandwidth_gran, delay_linear,
+ *                                      min_bandwidth, thread_throttle_mode
+ *
+ *     base directory structure
+ *     ------------------------------------------------------------------
+ *     --> RFTYPE_BASE (Files common for both MON and CTRL groups)
+ *         Files: cpus, cpus_list, tasks
+ *
+ *             --> RFTYPE_CTRL (Files only for CTRL group)
+ *                 Files: mode, schemata, size


-- 
Thanks
Babu Moger

Re: [PATCH v7 5/8] x86/resctrl: Unwind the errors inside rdt_enable_ctx()

[Moger, Babu]

Hi Reinette,

On 8/16/23 14:07, Reinette Chatre wrote:
> 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.

Yes. We can remove  cdp_disable_all() at part of this patch.
-- 
Thanks
Babu Moger

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

[Moger, Babu]

Reinette,

On 8/16/23 18:02, Reinette Chatre wrote:
> 
> Hi Babu,
> 
> On 8/16/2023 12:14 PM, Moger, Babu wrote:
>> Hi Reinette,
>>
>> On 8/15/23 17:50, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> Please do note that the subject is no longer accurate.
>>
>> Changing to
>>
>> x86/resctrl: Move default group file creation during mount
>>
> 
> How about:
> 	x86/resctrl: Move default group file creation to mount

Yes. Looks good.
Thanks
Babu

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

[Reinette Chatre]

Hi Babu,

On 8/17/2023 7:20 AM, Moger, Babu wrote:

> Yea. We can go with it. How about adding a little note about directories?
> That might be easy to compare directory structure on a systems with these
> flags. Something like this.


Thank you for improving it.

> + *
> + * 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.)
> + *
> + *     Note: resctrl does not use flags for directories. Directories are
> + *           created based on the resource type. Added directories below
> + *           for better understanding.

This is very helpful. How about a more specific "resctrl uses flags for
files, not for directories. Directories are ..."

> + *
> + *     info directory structure
> + *     ------------------------------------------------------------------
> + *     --> RFTYPE_INFO
> + *         directory: info

directory -> Directory ? (for all directory/directories instances)

> + *             --> RFTYPE_TOP (Files in top level of info directory)
> + *                 Files: last_cmd_status

(nitpick) Considering the directory vs directories, maybe this can be "File:"
until more files are added.

> + *
> + *             --> RFTYPE_MON (Files for all monitoring resources)
> + *                 directory: L3_MON

Should this not be below RFTYPE_RES_CACHE? 

> + *                     Files: mon_features, num_rmids



> + *
> + *                     --> RFTYPE_RES_CACHE (Files for cache monitoring
> resources)
> + *                         Files: max_threshold_occupancy,
> + *                                mbm_total_bytes_config,
> + *                                mbm_local_bytes_config
> + *
> + *             --> RFTYPE_CTRL (Files for all control resources)
> + *                 Files: num_closids

Maybe this should just be "File:" for now?

> + *
> + *                     --> RFTYPE_RES_CACHE (Files for cache control
> resources)
> + *                         directories: L2,L3

Please add a space after the comma.

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

The extra indent is not clear to me. Did you do it to represent
a hierarchy or just to line up the ":"?


> + *
> + *                     --> RFTYPE_RES_MB (Files for memory control resources)
> + *                         directories: MB,SMBA

Space after comma here also.

> + *                               Files: bandwidth_gran, delay_linear,
> + *                                      min_bandwidth, thread_throttle_mode
> + *
> + *     base directory structure
> + *     ------------------------------------------------------------------
> + *     --> RFTYPE_BASE (Files common for both MON and CTRL groups)
> + *         Files: cpus, cpus_list, tasks
> + *
> + *             --> RFTYPE_CTRL (Files only for CTRL group)
> + *                 Files: mode, schemata, size
> 
> 

Thank you very much. 

Reinette

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

[Moger, Babu]

Hi Reinette,

On 8/17/23 10:37, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/17/2023 7:20 AM, Moger, Babu wrote:
> 
>> Yea. We can go with it. How about adding a little note about directories?
>> That might be easy to compare directory structure on a systems with these
>> flags. Something like this.
> 
> 
> Thank you for improving it.
> 
>> + *
>> + * 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.)
>> + *
>> + *     Note: resctrl does not use flags for directories. Directories are
>> + *           created based on the resource type. Added directories below
>> + *           for better understanding.
> 
> This is very helpful. How about a more specific "resctrl uses flags for
> files, not for directories. Directories are ..."
> 
Sure.

>> + *
>> + *     info directory structure
>> + *     ------------------------------------------------------------------
>> + *     --> RFTYPE_INFO
>> + *         directory: info
> 
> directory -> Directory ? (for all directory/directories instances)

Sure.

> 
>> + *             --> RFTYPE_TOP (Files in top level of info directory)
>> + *                 Files: last_cmd_status
> 
> (nitpick) Considering the directory vs directories, maybe this can be "File:"
> until more files are added.

Yes.

> 
>> + *
>> + *             --> RFTYPE_MON (Files for all monitoring resources)
>> + *                 directory: L3_MON
> 
> Should this not be below RFTYPE_RES_CACHE? 

This is kind of odd. I know why you are saying it. Wouldn't it confuse the
user? Then, it feels like mon_features and num_rmids don't belong L3_MON.


>> + *                     Files: mon_features, num_rmids
> 
> 
> 
>> + *
>> + *                     --> RFTYPE_RES_CACHE (Files for cache monitoring
>> resources)
>> + *                         Files: max_threshold_occupancy,
>> + *                                mbm_total_bytes_config,
>> + *                                mbm_local_bytes_config
>> + *
>> + *             --> RFTYPE_CTRL (Files for all control resources)
>> + *                 Files: num_closids
> 
> Maybe this should just be "File:" for now?

Sure.

> 
>> + *
>> + *                     --> RFTYPE_RES_CACHE (Files for cache control
>> resources)
>> + *                         directories: L2,L3
> 
> Please add a space after the comma.

Sure

> 
>> + *                               Files: bit_usage, cbm_mask, min_cbm_bits,
>> + *                                      shareable_bits
> 
> The extra indent is not clear to me. Did you do it to represent
> a hierarchy or just to line up the ":"?

This is to line up with :. I can fix it.

Just wondering how do you notice these tabs? My linux diff does not show
any difference. Are you using any utilities to see these tabs?

> 
> 
>> + *
>> + *                     --> RFTYPE_RES_MB (Files for memory control resources)
>> + *                         directories: MB,SMBA
> 
> Space after comma here also.

Sure.

> 
>> + *                               Files: bandwidth_gran, delay_linear,
>> + *                                      min_bandwidth, thread_throttle_mode
>> + *
>> + *     base directory structure
>> + *     ------------------------------------------------------------------
>> + *     --> RFTYPE_BASE (Files common for both MON and CTRL groups)
>> + *         Files: cpus, cpus_list, tasks
>> + *
>> + *             --> RFTYPE_CTRL (Files only for CTRL group)
>> + *                 Files: mode, schemata, size
>>
>>
> 
> Thank you very much. 
> 
> Reinette

-- 
Thanks
Babu Moger

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

[Reinette Chatre]

Hi Babu,

On 8/17/2023 10:07 AM, Moger, Babu wrote:
> On 8/17/23 10:37, Reinette Chatre wrote:
>> On 8/17/2023 7:20 AM, Moger, Babu wrote:

>>> + *
>>> + *             --> RFTYPE_MON (Files for all monitoring resources)
>>> + *                 directory: L3_MON
>>
>> Should this not be below RFTYPE_RES_CACHE? 
> 
> This is kind of odd. I know why you are saying it. Wouldn't it confuse the
> user? Then, it feels like mon_features and num_rmids don't belong L3_MON.
> 

This is exactly the confusion that I attempted to highlight in my
first response to this patch. The same issue is present for
"num_closids" (which is currently treated differently ... at least
these need to be consistent). How can it be made obvious that
these files are present in all resource sub-directories while
also capturing the hierarchy of the RFTYPE flags? I could not
find a clear way and that is why I ended up removing the directories
in my earlier proposal and just stick to documenting the file flags
that only applies to files anyway. 

What do you think of something like below? It has duplication
but may be less confusing while still capturing the flags
accurately?

	--> RFTYPE_MON (Files for all monitoring resources)
	    Directory: L3_MON
	    Files: mon_features, num_rmids

	    --> RFTYPE_RES_CACHE (Files for cache monitoring resources)
		Directory: L3_MON
		Files: max_threshold_occupancy, mbm_total_bytes_config,
		       mbm_local_bytes_config

	--> RFTYPE_CTRL (Files for all control resources)
	    Directories: L2, L3, MB, SMBA
	    File: num_closids

	    --> RFTYPE_CTRL (Files for all control resources)
		Directories: L2, L3
		Files: bit_usage, cbm_mask, min_cbm_bits,
		       shareable_bits
	
	    --> RFTYPE_RES_MB (Files for memory control resources)
		Directories: MB, SMBA
		Files: bandwidth_gran, delay_linear,
		       min_bandwidth, thread_throttle_mode

	
>>> + *                               Files: bit_usage, cbm_mask, min_cbm_bits,
>>> + *                                      shareable_bits
>>
>> The extra indent is not clear to me. Did you do it to represent
>> a hierarchy or just to line up the ":"?
> 
> This is to line up with :. I can fix it.
> 
> Just wondering how do you notice these tabs? My linux diff does not show
> any difference. Are you using any utilities to see these tabs?

My editor is configured to visualize tabs and trailing spaces. In
this case it was just how my email client displayed it though.

Reinette

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

[Moger, Babu]

Hi Reinette,

On 8/17/23 12:42, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/17/2023 10:07 AM, Moger, Babu wrote:
>> On 8/17/23 10:37, Reinette Chatre wrote:
>>> On 8/17/2023 7:20 AM, Moger, Babu wrote:
> 
>>>> + *
>>>> + *             --> RFTYPE_MON (Files for all monitoring resources)
>>>> + *                 directory: L3_MON
>>>
>>> Should this not be below RFTYPE_RES_CACHE? 
>>
>> This is kind of odd. I know why you are saying it. Wouldn't it confuse the
>> user? Then, it feels like mon_features and num_rmids don't belong L3_MON.
>>
> 
> This is exactly the confusion that I attempted to highlight in my
> first response to this patch. The same issue is present for
> "num_closids" (which is currently treated differently ... at least
> these need to be consistent). How can it be made obvious that
> these files are present in all resource sub-directories while
> also capturing the hierarchy of the RFTYPE flags? I could not
> find a clear way and that is why I ended up removing the directories
> in my earlier proposal and just stick to documenting the file flags
> that only applies to files anyway. 
> 
> What do you think of something like below? It has duplication
> but may be less confusing while still capturing the flags
> accurately?

Yes. This looks good to me.

> 
> 	--> RFTYPE_MON (Files for all monitoring resources)
> 	    Directory: L3_MON
> 	    Files: mon_features, num_rmids
> 
> 	    --> RFTYPE_RES_CACHE (Files for cache monitoring resources)
> 		Directory: L3_MON
> 		Files: max_threshold_occupancy, mbm_total_bytes_config,
> 		       mbm_local_bytes_config
> 
> 	--> RFTYPE_CTRL (Files for all control resources)
> 	    Directories: L2, L3, MB, SMBA
> 	    File: num_closids
> 
> 	    --> RFTYPE_CTRL (Files for all control resources)
> 		Directories: L2, L3
> 		Files: bit_usage, cbm_mask, min_cbm_bits,
> 		       shareable_bits
> 	
> 	    --> RFTYPE_RES_MB (Files for memory control resources)
> 		Directories: MB, SMBA
> 		Files: bandwidth_gran, delay_linear,
> 		       min_bandwidth, thread_throttle_mode
> 
> 	
>>>> + *                               Files: bit_usage, cbm_mask, min_cbm_bits,
>>>> + *                                      shareable_bits
>>>
>>> The extra indent is not clear to me. Did you do it to represent
>>> a hierarchy or just to line up the ":"?
>>
>> This is to line up with :. I can fix it.
>>
>> Just wondering how do you notice these tabs? My linux diff does not show
>> any difference. Are you using any utilities to see these tabs?
> 
> My editor is configured to visualize tabs and trailing spaces. In
> this case it was just how my email client displayed it though.
> 

ok.
Thanks
Babu Moger

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

[Moger, Babu]

Hi Reinette,

On 8/15/23 17:52, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/11/2023 1:10 PM, Babu Moger wrote:
>> 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
> 
> "to refer" -> "to refer to"

Sure.

> 
>> 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
>>
>> 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>
>> ---
>>  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 6b7e914657fa..94471ad9d905 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,
>> +	},
>>  
>>  };
>>  
>>
>>
> 
> I think the comments introduced in patch #4 may need to be updated
> in this patch.

Sure. Will add it.
> 
> Apart from that this patch looks good to me.
> Thanks
Babu Moger