[PATCH v9 0/9] x86/resctrl: Miscellaneous resctrl features

[PATCH v9 0/9] 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.
---
v9:
   Changes since v8:
   Split the RMID display in a separate patch. RMID is a special case here as it
   should be printed only if monitoring is enabled.
   Made rdtgroup_setup_root and rdtgroup_destroy_root as static functions.
   Added code to print pid_str in case of task parse error.
  
v8:
   Changes since v7:
   Earlier moved both default group initialization and file creation on mount.
   Now kept the group initialization as is and only moved the file creation on mount.
   Re-organized the RFTYPE flags comments little more to avoid confusion. Thanks
   to Reinette for feedback.
   Re-factored the rdt_enable_ctx and added rdt_disable_ctx to unwind the errors.
   And same call also used in rdt_kill_sb which simplifies the code.
   Few other minor text changes.
   
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. 

v8: https://lore.kernel.org/lkml/20230821233048.434531-1-babu.moger@amd.com/
v7: https://lore.kernel.org/lkml/169178429591.1147205.4030367096506551808.stgit@bmoger-ubuntu/
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 (9):
  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 group file creation to mount
  x86/resctrl: Introduce "-o debug" mount option
  x86/resctrl: Display CLOSID for resource group
  x86/resctrl: Display RMID of resource group

 Documentation/arch/x86/resctrl.rst     |  22 ++-
 arch/x86/kernel/cpu/resctrl/internal.h |  88 +++++++--
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 255 ++++++++++++++++++-------
 3 files changed, 283 insertions(+), 82 deletions(-)

-- 
2.34.1

[PATCH v9 1/9] 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>
Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 Documentation/arch/x86/resctrl.rst     |  9 ++++++++-
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 25 ++++++++++++++++++++++---
 2 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
index cb05d90111b4..8154e9975d1e 100644
--- a/Documentation/arch/x86/resctrl.rst
+++ b/Documentation/arch/x86/resctrl.rst
@@ -299,7 +299,14 @@ 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 and
+	already added tasks before the failure will remain in the group.
+	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..f0d163950969 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_printf("Task list parsing error pid %s\n", pid_str);
+			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);
-- 
2.34.1

[PATCH v9 2/9] 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>
Reviewed-by: Fenghua Yu <fenghua.yu@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 f0d163950969..7ddfa4b470e6 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");
-- 
2.34.1

[PATCH v9 3/9] 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>
Reviewed-by: Fenghua Yu <fenghua.yu@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 7ddfa4b470e6..35945b4bf196 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;
-- 
2.34.1

[PATCH v9 4/9] 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.

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

diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 2051179a3b91..c97578e3115a 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -240,6 +240,64 @@ 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.)
+ *
+ *	Note: resctrl uses flags for files, not 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)
+ *		    File: 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)
+ *			    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, L2CODE, L2DATA, L3CODE, L3DATA
+ *		           File: num_closids
+ *
+ *			--> RFTYPE_RES_CACHE (Files for cache control resources)
+ *			    Directories: L2, L3, L2CODE, L2DATA, L3CODE, L3DATA
+ *			          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
+ *
  */
 #define RFTYPE_INFO			BIT(0)
 #define RFTYPE_BASE			BIT(1)
-- 
2.34.1

[PATCH v9 5/9] 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. This also simplifies
cleanup in rdt_kill_sb().

Remove cdp_disable_all() as it is not used anymore after the refactor.

Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
Reviewed-by: Fenghua Yu <fenghua.yu@intel.com>
Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 55 ++++++++++++++++----------
 1 file changed, 34 insertions(+), 21 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 35945b4bf196..34cb512be1de 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2290,14 +2290,6 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable)
 	return 0;
 }
 
-static void cdp_disable_all(void)
-{
-	if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L3))
-		resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
-	if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L2))
-		resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, false);
-}
-
 /*
  * We don't allow rdtgroup directories to be created anywhere
  * except the root directory. Thus when looking for the rdtgroup
@@ -2377,19 +2369,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(void)
+{
+	resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
+
+	resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, false);
+
+	set_mba_sc(false);
+}
+
 static int rdt_enable_ctx(struct rdt_fs_context *ctx)
 {
 	int ret = 0;
 
-	if (ctx->enable_cdpl2)
+	if (ctx->enable_cdpl2) {
 		ret = resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, true);
+		if (ret)
+			goto out_done;
+	}
 
-	if (!ret && ctx->enable_cdpl3)
+	if (ctx->enable_cdpl3) {
 		ret = resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, true);
+		if (ret)
+			goto out_cdpl2;
+	}
 
-	if (!ret && ctx->enable_mba_mbps)
+	if (ctx->enable_mba_mbps) {
 		ret = set_mba_sc(true);
+		if (ret)
+			goto out_cdpl3;
+	}
 
+	return 0;
+
+out_cdpl3:
+	resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
+out_cdpl2:
+	resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, false);
+out_done:
 	return ret;
 }
 
@@ -2497,13 +2514,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 +2579,8 @@ static int rdt_get_tree(struct fs_context *fc)
 	kernfs_remove(kn_info);
 out_schemata_free:
 	schemata_list_destroy();
-out_mba:
-	if (ctx->enable_mba_mbps)
-		set_mba_sc(false);
-out_cdp:
-	cdp_disable_all();
+out_ctx:
+	rdt_disable_ctx();
 out:
 	rdt_last_cmd_clear();
 	mutex_unlock(&rdtgroup_mutex);
@@ -2798,12 +2812,11 @@ static void rdt_kill_sb(struct super_block *sb)
 	cpus_read_lock();
 	mutex_lock(&rdtgroup_mutex);
 
-	set_mba_sc(false);
+	rdt_disable_ctx();
 
 	/*Put everything back to default values. */
 	for_each_alloc_capable_rdt_resource(r)
 		reset_all_ctrls(r);
-	cdp_disable_all();
 	rmdir_all_sub();
 	rdt_pseudo_lock_release();
 	rdtgroup_default.mode = RDT_MODE_SHAREABLE;
-- 
2.34.1

[PATCH v9 6/9] x86/resctrl: Move default group file creation to mount

[Babu Moger]

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/rdtgroup.c | 58 +++++++++++++++-----------
 1 file changed, 34 insertions(+), 24 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 34cb512be1de..27cae0cc25af 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -54,6 +54,9 @@ static struct kernfs_node *kn_mondata;
 static struct seq_buf last_cmd_status;
 static char last_cmd_status_buf[512];
 
+static int rdtgroup_setup_root(struct rdt_fs_context *ctx);
+static void rdtgroup_destroy_root(void);
+
 struct dentry *debugfs_resctrl;
 
 void rdt_last_cmd_clear(void)
@@ -2513,10 +2516,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();
@@ -2525,6 +2532,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;
@@ -2581,6 +2594,8 @@ static int rdt_get_tree(struct fs_context *fc)
 	schemata_list_destroy();
 out_ctx:
 	rdt_disable_ctx();
+out_root:
+	rdtgroup_destroy_root();
 out:
 	rdt_last_cmd_clear();
 	mutex_unlock(&rdtgroup_mutex);
@@ -2651,7 +2666,6 @@ static int rdt_init_fs_context(struct fs_context *fc)
 	if (!ctx)
 		return -ENOMEM;
 
-	ctx->kfc.root = rdt_root;
 	ctx->kfc.magic = RDTGROUP_SUPER_MAGIC;
 	fc->fs_private = &ctx->kfc;
 	fc->ops = &rdt_fs_context_ops;
@@ -2821,6 +2835,7 @@ static void rdt_kill_sb(struct super_block *sb)
 	rdt_pseudo_lock_release();
 	rdtgroup_default.mode = RDT_MODE_SHAREABLE;
 	schemata_list_destroy();
+	rdtgroup_destroy_root();
 	static_branch_disable_cpuslocked(&rdt_alloc_enable_key);
 	static_branch_disable_cpuslocked(&rdt_mon_enable_key);
 	static_branch_disable_cpuslocked(&rdt_enable_key);
@@ -3702,10 +3717,8 @@ static struct kernfs_syscall_ops rdtgroup_kf_syscall_ops = {
 	.show_options	= rdtgroup_show_options,
 };
 
-static int __init rdtgroup_setup_root(void)
+static 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,
@@ -3713,6 +3726,20 @@ 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 rdtgroup_destroy_root(void)
+{
+	kernfs_destroy_root(rdt_root);
+	rdtgroup_default.kn = NULL;
+}
+
+static void __init rdtgroup_setup_default(void)
+{
 	mutex_lock(&rdtgroup_mutex);
 
 	rdtgroup_default.closid = 0;
@@ -3722,19 +3749,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)
@@ -3856,13 +3871,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)
@@ -3895,8 +3908,6 @@ int __init rdtgroup_init(void)
 
 cleanup_mountpoint:
 	sysfs_remove_mount_point(fs_kobj, "resctrl");
-cleanup_root:
-	kernfs_destroy_root(rdt_root);
 
 	return ret;
 }
@@ -3906,5 +3917,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);
 }
-- 
2.34.1

[PATCH v9 7/9] x86/resctrl: Introduce "-o debug" mount option

[Babu Moger]

Add "-o debug" option to mount resctrl filesystem in debug mode.
When in debug mode resctrl displays files that have the new
RFTYPE_DEBUG flag to help resctrl debugging.

Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
Reviewed-by: Fenghua Yu <fenghua.yu@intel.com>
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 | 18 ++++++++++++++++++
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
index 8154e9975d1e..28d35aaa74b4 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 c97578e3115a..38b99a21ccd8 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)
@@ -306,6 +307,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 27cae0cc25af..67b315f3a26c 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -59,6 +59,8 @@ static void rdtgroup_destroy_root(void);
 
 struct dentry *debugfs_resctrl;
 
+static bool resctrl_debug;
+
 void rdt_last_cmd_clear(void)
 {
 	lockdep_assert_held(&rdtgroup_mutex);
@@ -1874,6 +1876,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);
@@ -2379,6 +2384,8 @@ static void rdt_disable_ctx(void)
 	resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, false);
 
 	set_mba_sc(false);
+
+	resctrl_debug = false;
 }
 
 static int rdt_enable_ctx(struct rdt_fs_context *ctx)
@@ -2403,6 +2410,9 @@ static int rdt_enable_ctx(struct rdt_fs_context *ctx)
 			goto out_cdpl3;
 	}
 
+	if (ctx->enable_debug)
+		resctrl_debug = true;
+
 	return 0;
 
 out_cdpl3:
@@ -2607,6 +2617,7 @@ enum rdt_param {
 	Opt_cdp,
 	Opt_cdpl2,
 	Opt_mba_mbps,
+	Opt_debug,
 	nr__rdt_params
 };
 
@@ -2614,6 +2625,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),
 	{}
 };
 
@@ -2639,6 +2651,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;
@@ -3707,6 +3722,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;
 }
 
-- 
2.34.1

[PATCH v9 8/9] x86/resctrl: Display CLOSID for resource group

[Babu Moger]

In x86, hardware uses CLOSID to identify a control group. When a user
creates a control group this information is not visible to the user.
It can help resctrl debugging.

Add CLOSID(ctrl_hw_id) to the control groups display in resctrl
interface. Users can see this detail when resctrl is mounted with
"-o debug" option.

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

For example:
 $cat /sys/fs/resctrl/ctrl_grp1/ctrl_hw_id
 1

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

diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
index 28d35aaa74b4..54691c8b832d 100644
--- a/Documentation/arch/x86/resctrl.rst
+++ b/Documentation/arch/x86/resctrl.rst
@@ -352,6 +352,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":
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 38b99a21ccd8..ccdbed615d41 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -299,6 +299,9 @@ struct rdtgroup {
  *		--> RFTYPE_CTRL (Files only for CTRL group)
  *		    Files: mode, schemata, size
  *
+ *			--> RFTYPE_DEBUG (Files to help resctrl debugging)
+ *			    File: ctrl_hw_id
+ *
  */
 #define RFTYPE_INFO			BIT(0)
 #define RFTYPE_BASE			BIT(1)
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 67b315f3a26c..8be0fb323ad3 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -779,6 +779,22 @@ 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;
+}
+
 #ifdef CONFIG_PROC_CPU_RESCTRL
 
 /*
@@ -1863,6 +1879,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,
+	},
 
 };
 
-- 
2.34.1

[PATCH v9 9/9] x86/resctrl: Display RMID of resource group

[Babu Moger]

In x86, hardware uses RMID to identify a a monitoring group. When a
user creates a monitor group these details are not visible. These details
can help resctrl debugging.

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

Other architectures do not use "RMID". Use the name mon_hw_id to refer
to "RMID" in an effort to keep the naming generic.

Add the flag RFTYPE_MON_BASE, which contains the files required only
for the MON group.

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

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 Documentation/arch/x86/resctrl.rst     |  4 +++
 arch/x86/kernel/cpu/resctrl/internal.h |  6 ++++
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 38 +++++++++++++++++++++++---
 3 files changed, 44 insertions(+), 4 deletions(-)

diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
index 54691c8b832d..98b0eb509ed4 100644
--- a/Documentation/arch/x86/resctrl.rst
+++ b/Documentation/arch/x86/resctrl.rst
@@ -369,6 +369,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/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index ccdbed615d41..b4910892b0a6 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -296,6 +296,11 @@ struct rdtgroup {
  *	--> RFTYPE_BASE (Files common for both MON and CTRL groups)
  *	    Files: cpus, cpus_list, tasks
  *
+ *		--> RFTYPE_MON (Files only for MON group)
+ *
+ *			--> RFTYPE_DEBUG (Files to help resctrl debugging)
+ *			    File: mon_hw_id
+ *
  *		--> RFTYPE_CTRL (Files only for CTRL group)
  *		    Files: mode, schemata, size
  *
@@ -315,6 +320,7 @@ struct rdtgroup {
 #define RFTYPE_MON_INFO			(RFTYPE_INFO | RFTYPE_MON)
 #define RFTYPE_TOP_INFO			(RFTYPE_INFO | RFTYPE_TOP)
 #define RFTYPE_CTRL_BASE		(RFTYPE_BASE | RFTYPE_CTRL)
+#define RFTYPE_MON_BASE			(RFTYPE_BASE | RFTYPE_MON)
 
 /* List of all resource groups */
 extern struct list_head rdt_all_groups;
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 8be0fb323ad3..fc830ffce82a 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -795,6 +795,22 @@ static int rdtgroup_closid_show(struct kernfs_open_file *of,
 	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
 
 /*
@@ -1856,6 +1872,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_MON_BASE | RFTYPE_DEBUG,
+	},
 	{
 		.name		= "schemata",
 		.mode		= 0644,
@@ -2535,6 +2558,7 @@ static void schemata_list_destroy(void)
 static int rdt_get_tree(struct fs_context *fc)
 {
 	struct rdt_fs_context *ctx = rdt_fc2context(fc);
+	unsigned long flags = RFTYPE_CTRL_BASE;
 	struct rdt_domain *dom;
 	struct rdt_resource *r;
 	int ret;
@@ -2565,7 +2589,10 @@ static int rdt_get_tree(struct fs_context *fc)
 
 	closid_init();
 
-	ret = rdtgroup_add_files(rdtgroup_default.kn, RFTYPE_CTRL_BASE);
+	if (rdt_mon_capable)
+		flags |= RFTYPE_MON;
+
+	ret = rdtgroup_add_files(rdtgroup_default.kn, flags);
 	if (ret)
 		goto out_schemata_free;
 
@@ -3255,8 +3282,8 @@ static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
 			     enum rdt_group_type rtype, struct rdtgroup **r)
 {
 	struct rdtgroup *prdtgrp, *rdtgrp;
+	unsigned long files = 0;
 	struct kernfs_node *kn;
-	uint files = 0;
 	int ret;
 
 	prdtgrp = rdtgroup_kn_lock_live(parent_kn);
@@ -3308,10 +3335,13 @@ static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
 		goto out_destroy;
 	}
 
-	if (rtype == RDTCTRL_GROUP)
+	if (rtype == RDTCTRL_GROUP) {
 		files = RFTYPE_BASE | RFTYPE_CTRL;
-	else
+		if (rdt_mon_capable)
+			files |= RFTYPE_MON;
+	} else {
 		files = RFTYPE_BASE | RFTYPE_MON;
+	}
 
 	ret = rdtgroup_add_files(kn, files);
 	if (ret) {
-- 
2.34.1

Re: [PATCH v9 1/9] x86/resctrl: Add multiple tasks to the resctrl group at once

[Fenghua Yu]

On 9/7/23 16:51, 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>
> Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Babu Moger <babu.moger@amd.com>

Reviewed-by: Fenghua Yu <fenghua.yu@intel.com>

Thanks.

-Fenghua

Re: [PATCH v9 4/9] x86/resctrl: Add comments on RFTYPE flags hierarchy

[Fenghua Yu]

On 9/7/23 16:51, 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.
> 
> Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Babu Moger <babu.moger@amd.com>

Reviewed-by: Fenghua Yu <fenghua.yu@intel.com>

Thanks.

-Fenghua

Re: [PATCH v9 6/9] x86/resctrl: Move default group file creation to mount

[Fenghua Yu]

On 9/7/23 16:51, Babu Moger wrote:
> 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>

Reviewed-by: Fenghua Yu <fenghua.yu@intel.com>

Thanks.

-Fenghua

Re: [PATCH v9 8/9] x86/resctrl: Display CLOSID for resource group

[Fenghua Yu]

Hi, Babu,

On 9/7/23 16:51, Babu Moger wrote:
> In x86, hardware uses CLOSID to identify a control group. When a user
> creates a control group this information is not visible to the user.
> It can help resctrl debugging.
> 
> Add CLOSID(ctrl_hw_id) to the control groups display in resctrl
> interface. Users can see this detail when resctrl is mounted with
> "-o debug" option.
> 
> Other architectures do not use "CLOSID". Use the names ctrl_hw_id
> to refer to "CLOSID" in an effort to keep the naming generic.
> 
> For example:
>   $cat /sys/fs/resctrl/ctrl_grp1/ctrl_hw_id
>   1
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>   Documentation/arch/x86/resctrl.rst     |  4 ++++
>   arch/x86/kernel/cpu/resctrl/internal.h |  3 +++
>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 23 +++++++++++++++++++++++
>   3 files changed, 30 insertions(+)
> 
> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
> index 28d35aaa74b4..54691c8b832d 100644
> --- a/Documentation/arch/x86/resctrl.rst
> +++ b/Documentation/arch/x86/resctrl.rst
> @@ -352,6 +352,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":
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 38b99a21ccd8..ccdbed615d41 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -299,6 +299,9 @@ struct rdtgroup {
>    *		--> RFTYPE_CTRL (Files only for CTRL group)
>    *		    Files: mode, schemata, size
>    *
> + *			--> RFTYPE_DEBUG (Files to help resctrl debugging)
> + *			    File: ctrl_hw_id
> + *
>    */
>   #define RFTYPE_INFO			BIT(0)
>   #define RFTYPE_BASE			BIT(1)
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 67b315f3a26c..8be0fb323ad3 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -779,6 +779,22 @@ 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;
> +}
> +
>   #ifdef CONFIG_PROC_CPU_RESCTRL
>   
>   /*
> @@ -1863,6 +1879,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,

Is it better to rename "rdtgroup_closid_show" as arch neutral name 
"rdtgroup_ctrl_hw_id_show"? So the name is arch neutral and reflects the 
ctrl_hw_id. So this can eventually go to generic fs code without 
renaming it. The getting closid implementation in the function will be 
arch specific.

> +		.fflags		= RFTYPE_CTRL_BASE | RFTYPE_DEBUG,
> +	},
>   
>   };
>   

Thanks.

-Fenghua

Re: [PATCH v9 9/9] x86/resctrl: Display RMID of resource group

[Fenghua Yu]

Hi, Babu,

On 9/7/23 16:51, Babu Moger wrote:
> In x86, hardware uses RMID to identify a a monitoring group. When a

s/a a/a/

> user creates a monitor group these details are not visible. These details
> can help resctrl debugging.
> 
> Add RMID(mon_hw_id) to the monitor groups display in resctrl interface.
> Users can see these details when resctrl is mounted with "-o debug" option.
> 
> Other architectures do not use "RMID". Use the name mon_hw_id to refer
> to "RMID" in an effort to keep the naming generic.
> 
> Add the flag RFTYPE_MON_BASE, which contains the files required only
> for the MON group.
> 
> For example:
>   $cat /sys/fs/resctrl/mon_groups/mon_grp1/mon_hw_id
>   3
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>   Documentation/arch/x86/resctrl.rst     |  4 +++
>   arch/x86/kernel/cpu/resctrl/internal.h |  6 ++++
>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 38 +++++++++++++++++++++++---
>   3 files changed, 44 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
> index 54691c8b832d..98b0eb509ed4 100644
> --- a/Documentation/arch/x86/resctrl.rst
> +++ b/Documentation/arch/x86/resctrl.rst
> @@ -369,6 +369,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/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index ccdbed615d41..b4910892b0a6 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -296,6 +296,11 @@ struct rdtgroup {
>    *	--> RFTYPE_BASE (Files common for both MON and CTRL groups)
>    *	    Files: cpus, cpus_list, tasks
>    *
> + *		--> RFTYPE_MON (Files only for MON group)
> + *
> + *			--> RFTYPE_DEBUG (Files to help resctrl debugging)
> + *			    File: mon_hw_id
> + *
>    *		--> RFTYPE_CTRL (Files only for CTRL group)
>    *		    Files: mode, schemata, size
>    *
> @@ -315,6 +320,7 @@ struct rdtgroup {
>   #define RFTYPE_MON_INFO			(RFTYPE_INFO | RFTYPE_MON)
>   #define RFTYPE_TOP_INFO			(RFTYPE_INFO | RFTYPE_TOP)
>   #define RFTYPE_CTRL_BASE		(RFTYPE_BASE | RFTYPE_CTRL)
> +#define RFTYPE_MON_BASE			(RFTYPE_BASE | RFTYPE_MON)
>   
>   /* List of all resource groups */
>   extern struct list_head rdt_all_groups;
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 8be0fb323ad3..fc830ffce82a 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -795,6 +795,22 @@ static int rdtgroup_closid_show(struct kernfs_open_file *of,
>   	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
>   
>   /*
> @@ -1856,6 +1872,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,

Similar to showing ctrl_hw_id, is it better to rename 
"rdtgroup_rmid_show" as "rdtgroup_mon_hw_id_show" for arch neutral naming?

> +		.fflags		= RFTYPE_MON_BASE | RFTYPE_DEBUG,
> +	},
>   	{
>   		.name		= "schemata",
>   		.mode		= 0644,
> @@ -2535,6 +2558,7 @@ static void schemata_list_destroy(void)
>   static int rdt_get_tree(struct fs_context *fc)
>   {
>   	struct rdt_fs_context *ctx = rdt_fc2context(fc);
> +	unsigned long flags = RFTYPE_CTRL_BASE;
>   	struct rdt_domain *dom;
>   	struct rdt_resource *r;
>   	int ret;
> @@ -2565,7 +2589,10 @@ static int rdt_get_tree(struct fs_context *fc)
>   
>   	closid_init();
>   
> -	ret = rdtgroup_add_files(rdtgroup_default.kn, RFTYPE_CTRL_BASE);
> +	if (rdt_mon_capable)
> +		flags |= RFTYPE_MON;
> +
> +	ret = rdtgroup_add_files(rdtgroup_default.kn, flags);
>   	if (ret)
>   		goto out_schemata_free;
>   
> @@ -3255,8 +3282,8 @@ static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
>   			     enum rdt_group_type rtype, struct rdtgroup **r)
>   {
>   	struct rdtgroup *prdtgrp, *rdtgrp;
> +	unsigned long files = 0;
>   	struct kernfs_node *kn;
> -	uint files = 0;
>   	int ret;
>   
>   	prdtgrp = rdtgroup_kn_lock_live(parent_kn);
> @@ -3308,10 +3335,13 @@ static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
>   		goto out_destroy;
>   	}
>   
> -	if (rtype == RDTCTRL_GROUP)
> +	if (rtype == RDTCTRL_GROUP) {
>   		files = RFTYPE_BASE | RFTYPE_CTRL;
> -	else
> +		if (rdt_mon_capable)
> +			files |= RFTYPE_MON;
> +	} else {
>   		files = RFTYPE_BASE | RFTYPE_MON;
> +	}
>   
>   	ret = rdtgroup_add_files(kn, files);
>   	if (ret) {

Thanks.

-Fenghua

Re: [PATCH v9 8/9] x86/resctrl: Display CLOSID for resource group

[Reinette Chatre]

Hi Fenghua,

On 9/11/2023 11:08 AM, Fenghua Yu wrote:
> On 9/7/23 16:51, Babu Moger wrote:

...
>> @@ -1863,6 +1879,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,
> 
> Is it better to rename "rdtgroup_closid_show" as arch neutral name
> "rdtgroup_ctrl_hw_id_show"? So the name is arch neutral and reflects
> the ctrl_hw_id. So this can eventually go to generic fs code without
> renaming it. The getting closid implementation in the function will
> be arch specific.

This is not so obvious to me. We have to draw the line somewhere. This
series draws the line between the kernel and user interface. That is,
from user perspective it is "ctrl_hw_id" and from kernel perspective
it is "closid".

I think renaming rdtgroup_closid_show() to rdtgroup_ctrl_hw_id_show() may
be mixing up the two because the function called rdtgroup_ctrl_hw_id_show() 
would operate on rdtgroup->closid - so in a sense renaming the function
to rdtgroup_ctrl_hw_id_show() would move the name further from what the
function does. Would that mean that we need to refactor the whole kernel
the naming to match between kernel space and user space by changing
rdtgroup->closid to rdtgroup->ctrl_hw_id? That escalates quite far and
wide and I wonder if it is not just simpler to keep the naming
boundary between kernel space and user space.

Reinette

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

[Reinette Chatre]

Hi Babu,

On 9/7/2023 4:51 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. This also simplifies
> cleanup in rdt_kill_sb().
> 
> Remove cdp_disable_all() as it is not used anymore after the refactor.
> 
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Reviewed-by: Fenghua Yu <fenghua.yu@intel.com>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---

This is one of the patches that received a change that was not
mentioned in cover letter nor in a patch specific list of changes.

Having a list of changes within each patch in this area is
helpful in reviews.

>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 55 ++++++++++++++++----------
>  1 file changed, 34 insertions(+), 21 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 35945b4bf196..34cb512be1de 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -2290,14 +2290,6 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable)
>  	return 0;
>  }
>  
> -static void cdp_disable_all(void)
> -{
> -	if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L3))
> -		resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
> -	if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L2))
> -		resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, false);
> -}
> -
>  /*
>   * We don't allow rdtgroup directories to be created anywhere
>   * except the root directory. Thus when looking for the rdtgroup
> @@ -2377,19 +2369,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(void)
> +{
> +	resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
> +
> +	resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, false);
> +
> +	set_mba_sc(false);
> +}

Could you please remove the empty line following each call?

With the above change you can add:

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

Reinette

Re: [PATCH v9 6/9] x86/resctrl: Move default group file creation to mount

[Reinette Chatre]

Hi Babu,

On 9/7/2023 4:51 PM, Babu Moger wrote:
> 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>

Thank you.

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

Reinette

Re: [PATCH v9 9/9] x86/resctrl: Display RMID of resource group

[Reinette Chatre]

Hi Babu,

On 9/7/2023 4:51 PM, Babu Moger wrote:
> In x86, hardware uses RMID to identify a a monitoring group. When a
> user creates a monitor group these details are not visible. These details
> can help resctrl debugging.
> 
> Add RMID(mon_hw_id) to the monitor groups display in resctrl interface.
> Users can see these details when resctrl is mounted with "-o debug" option.
> 
> Other architectures do not use "RMID". Use the name mon_hw_id to refer
> to "RMID" in an effort to keep the naming generic.
> 
> Add the flag RFTYPE_MON_BASE, which contains the files required only
> for the MON group.

As I mentioned in [1] I believe adding support for files with RFTYPE_MON flag
to resctrl needs to be in a separate patch. With the core support added a file
with this flag can be introduced separately. 

Reinette

[1] https://lore.kernel.org/lkml/2feb3e01-96c7-fdde-a0d2-509fa1527243@intel.com/

Re: [PATCH v9 0/9] x86/resctrl: Miscellaneous resctrl features

[Reinette Chatre]

Hi Babu,

On 9/7/2023 4:51 PM, Babu Moger wrote:
> 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.
> ---
> v9:
>    Changes since v8:
>    Split the RMID display in a separate patch. RMID is a special case here as it
>    should be printed only if monitoring is enabled.
>    Made rdtgroup_setup_root and rdtgroup_destroy_root as static functions.
>    Added code to print pid_str in case of task parse error.

fyi ... I found this version difficult to review. The cover letter contained this
high level list of changes while some patches were changed without getting a mention
here nor in patch-specific list of changes. I ended up needing to do a diff with the
previous version to get an idea of what actually changed.

Reinette

Re: [PATCH v9 0/9] x86/resctrl: Miscellaneous resctrl features

[Moger, Babu]

Hi Reinette,

On 9/11/23 18:08, Reinette Chatre wrote:
> Hi Babu,
> 
> On 9/7/2023 4:51 PM, Babu Moger wrote:
>> 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.
>> ---
>> v9:
>>    Changes since v8:
>>    Split the RMID display in a separate patch. RMID is a special case here as it
>>    should be printed only if monitoring is enabled.
>>    Made rdtgroup_setup_root and rdtgroup_destroy_root as static functions.
>>    Added code to print pid_str in case of task parse error.
> 
> fyi ... I found this version difficult to review. The cover letter contained this
> high level list of changes while some patches were changed without getting a mention
> here nor in patch-specific list of changes. I ended up needing to do a diff with the
> previous version to get an idea of what actually changed.

Yes. My bad. Should have listed the changes on each patch. Sure. Will do
next time.
thanks
Babu

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

[Moger, Babu]

Hi Reinette,

On 9/11/23 17:58, Reinette Chatre wrote:
> Hi Babu,
> 
> On 9/7/2023 4:51 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. This also simplifies
>> cleanup in rdt_kill_sb().
>>
>> Remove cdp_disable_all() as it is not used anymore after the refactor.
>>
>> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
>> Reviewed-by: Fenghua Yu <fenghua.yu@intel.com>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
> 
> This is one of the patches that received a change that was not
> mentioned in cover letter nor in a patch specific list of changes.

Yes. My bad.

> 
> Having a list of changes within each patch in this area is
> helpful in reviews.
> 
>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 55 ++++++++++++++++----------
>>  1 file changed, 34 insertions(+), 21 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 35945b4bf196..34cb512be1de 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -2290,14 +2290,6 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable)
>>  	return 0;
>>  }
>>  
>> -static void cdp_disable_all(void)
>> -{
>> -	if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L3))
>> -		resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
>> -	if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L2))
>> -		resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, false);
>> -}
>> -
>>  /*
>>   * We don't allow rdtgroup directories to be created anywhere
>>   * except the root directory. Thus when looking for the rdtgroup
>> @@ -2377,19 +2369,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(void)
>> +{
>> +	resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
>> +
>> +	resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, false);
>> +
>> +	set_mba_sc(false);
>> +}
> 
> Could you please remove the empty line following each call?

Sure.

> 
> With the above change you can add:
> 
> Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
-- 
Thanks
Babu Moger

Re: [PATCH v9 9/9] x86/resctrl: Display RMID of resource group

[Moger, Babu]

Hi Fenghua,

On 9/11/23 13:14, Fenghua Yu wrote:
> Hi, Babu,
> 
> On 9/7/23 16:51, Babu Moger wrote:
>> In x86, hardware uses RMID to identify a a monitoring group. When a
> 
> s/a a/a/

Sure.
Thanks for the review. Reinette has already addressed other comments.
Thanks
Babu

> 
>> user creates a monitor group these details are not visible. These details
>> can help resctrl debugging.
>>
>> Add RMID(mon_hw_id) to the monitor groups display in resctrl interface.
>> Users can see these details when resctrl is mounted with "-o debug" option.
>>
>> Other architectures do not use "RMID". Use the name mon_hw_id to refer
>> to "RMID" in an effort to keep the naming generic.
>>
>> Add the flag RFTYPE_MON_BASE, which contains the files required only
>> for the MON group.
>>
>> For example:
>>   $cat /sys/fs/resctrl/mon_groups/mon_grp1/mon_hw_id
>>   3
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>   Documentation/arch/x86/resctrl.rst     |  4 +++
>>   arch/x86/kernel/cpu/resctrl/internal.h |  6 ++++
>>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 38 +++++++++++++++++++++++---
>>   3 files changed, 44 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/arch/x86/resctrl.rst
>> b/Documentation/arch/x86/resctrl.rst
>> index 54691c8b832d..98b0eb509ed4 100644
>> --- a/Documentation/arch/x86/resctrl.rst
>> +++ b/Documentation/arch/x86/resctrl.rst
>> @@ -369,6 +369,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/internal.h
>> b/arch/x86/kernel/cpu/resctrl/internal.h
>> index ccdbed615d41..b4910892b0a6 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -296,6 +296,11 @@ struct rdtgroup {
>>    *    --> RFTYPE_BASE (Files common for both MON and CTRL groups)
>>    *        Files: cpus, cpus_list, tasks
>>    *
>> + *        --> RFTYPE_MON (Files only for MON group)
>> + *
>> + *            --> RFTYPE_DEBUG (Files to help resctrl debugging)
>> + *                File: mon_hw_id
>> + *
>>    *        --> RFTYPE_CTRL (Files only for CTRL group)
>>    *            Files: mode, schemata, size
>>    *
>> @@ -315,6 +320,7 @@ struct rdtgroup {
>>   #define RFTYPE_MON_INFO            (RFTYPE_INFO | RFTYPE_MON)
>>   #define RFTYPE_TOP_INFO            (RFTYPE_INFO | RFTYPE_TOP)
>>   #define RFTYPE_CTRL_BASE        (RFTYPE_BASE | RFTYPE_CTRL)
>> +#define RFTYPE_MON_BASE            (RFTYPE_BASE | RFTYPE_MON)
>>     /* List of all resource groups */
>>   extern struct list_head rdt_all_groups;
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 8be0fb323ad3..fc830ffce82a 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -795,6 +795,22 @@ static int rdtgroup_closid_show(struct
>> kernfs_open_file *of,
>>       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
>>     /*
>> @@ -1856,6 +1872,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,
> 
> Similar to showing ctrl_hw_id, is it better to rename "rdtgroup_rmid_show"
> as "rdtgroup_mon_hw_id_show" for arch neutral naming?
> 
>> +        .fflags        = RFTYPE_MON_BASE | RFTYPE_DEBUG,
>> +    },
>>       {
>>           .name        = "schemata",
>>           .mode        = 0644,
>> @@ -2535,6 +2558,7 @@ static void schemata_list_destroy(void)
>>   static int rdt_get_tree(struct fs_context *fc)
>>   {
>>       struct rdt_fs_context *ctx = rdt_fc2context(fc);
>> +    unsigned long flags = RFTYPE_CTRL_BASE;
>>       struct rdt_domain *dom;
>>       struct rdt_resource *r;
>>       int ret;
>> @@ -2565,7 +2589,10 @@ static int rdt_get_tree(struct fs_context *fc)
>>         closid_init();
>>   -    ret = rdtgroup_add_files(rdtgroup_default.kn, RFTYPE_CTRL_BASE);
>> +    if (rdt_mon_capable)
>> +        flags |= RFTYPE_MON;
>> +
>> +    ret = rdtgroup_add_files(rdtgroup_default.kn, flags);
>>       if (ret)
>>           goto out_schemata_free;
>>   @@ -3255,8 +3282,8 @@ static int mkdir_rdt_prepare(struct kernfs_node
>> *parent_kn,
>>                    enum rdt_group_type rtype, struct rdtgroup **r)
>>   {
>>       struct rdtgroup *prdtgrp, *rdtgrp;
>> +    unsigned long files = 0;
>>       struct kernfs_node *kn;
>> -    uint files = 0;
>>       int ret;
>>         prdtgrp = rdtgroup_kn_lock_live(parent_kn);
>> @@ -3308,10 +3335,13 @@ static int mkdir_rdt_prepare(struct kernfs_node
>> *parent_kn,
>>           goto out_destroy;
>>       }
>>   -    if (rtype == RDTCTRL_GROUP)
>> +    if (rtype == RDTCTRL_GROUP) {
>>           files = RFTYPE_BASE | RFTYPE_CTRL;
>> -    else
>> +        if (rdt_mon_capable)
>> +            files |= RFTYPE_MON;
>> +    } else {
>>           files = RFTYPE_BASE | RFTYPE_MON;
>> +    }
>>         ret = rdtgroup_add_files(kn, files);
>>       if (ret) {
> 
> Thanks.
> 
> -Fenghua

-- 
Thanks
Babu Moger

Re: [PATCH v9 9/9] x86/resctrl: Display RMID of resource group

[Moger, Babu]

Hi Reinette,


On 9/11/23 18:08, Reinette Chatre wrote:
> Hi Babu,
> 
> On 9/7/2023 4:51 PM, Babu Moger wrote:
>> In x86, hardware uses RMID to identify a a monitoring group. When a
>> user creates a monitor group these details are not visible. These details
>> can help resctrl debugging.
>>
>> Add RMID(mon_hw_id) to the monitor groups display in resctrl interface.
>> Users can see these details when resctrl is mounted with "-o debug" option.
>>
>> Other architectures do not use "RMID". Use the name mon_hw_id to refer
>> to "RMID" in an effort to keep the naming generic.
>>
>> Add the flag RFTYPE_MON_BASE, which contains the files required only
>> for the MON group.
> 
> As I mentioned in [1] I believe adding support for files with RFTYPE_MON flag
> to resctrl needs to be in a separate patch. With the core support added a file
> with this flag can be introduced separately. 

Yes. We discussed it. Thought it may not fit as a separate patch on its
own. So, i combined it.

Now, I split patch 9 into two. Let me know if it looks ok.

==========================================================================

Author: Babu Moger <babu.moger@amd.com>
Date:   Thu Sep 7 18:51:28 2023 -0500

    x86/resctrl: Add RFTYPE_MON_BASE for MON groups

    Add the flag RFTYPE_MON_BASE, which contains the files required only
    for the MON group. Files with these flags are only visible when
    monitoring is enabled.

    Signed-off-by: Babu Moger <babu.moger@amd.com>

diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
b/arch/x86/kernel/cpu/resctrl/internal.h
index ccdbed615d41..72988e5c52a7 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -296,6 +296,8 @@ struct rdtgroup {
  *     --> RFTYPE_BASE (Files common for both MON and CTRL groups)
  *         Files: cpus, cpus_list, tasks
  *
+ *             --> RFTYPE_MON (Files only for MON group)
+ *
  *             --> RFTYPE_CTRL (Files only for CTRL group)
  *                 Files: mode, schemata, size
  *
@@ -315,6 +317,7 @@ struct rdtgroup {
 #define RFTYPE_MON_INFO                        (RFTYPE_INFO | RFTYPE_MON)
 #define RFTYPE_TOP_INFO                        (RFTYPE_INFO | RFTYPE_TOP)
 #define RFTYPE_CTRL_BASE               (RFTYPE_BASE | RFTYPE_CTRL)
+#define RFTYPE_MON_BASE                        (RFTYPE_BASE | RFTYPE_MON)

 /* List of all resource groups */
 extern struct list_head rdt_all_groups;

=============================================================================
Thanks
Babu Moger

Re: [PATCH v9 9/9] x86/resctrl: Display RMID of resource group

[Reinette Chatre]

Hi Babu,

On 9/12/2023 10:39 AM, Moger, Babu wrote:
> On 9/11/23 18:08, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 9/7/2023 4:51 PM, Babu Moger wrote:
>>> In x86, hardware uses RMID to identify a a monitoring group. When a
>>> user creates a monitor group these details are not visible. These details
>>> can help resctrl debugging.
>>>
>>> Add RMID(mon_hw_id) to the monitor groups display in resctrl interface.
>>> Users can see these details when resctrl is mounted with "-o debug" option.
>>>
>>> Other architectures do not use "RMID". Use the name mon_hw_id to refer
>>> to "RMID" in an effort to keep the naming generic.
>>>
>>> Add the flag RFTYPE_MON_BASE, which contains the files required only
>>> for the MON group.
>>
>> As I mentioned in [1] I believe adding support for files with RFTYPE_MON flag
>> to resctrl needs to be in a separate patch. With the core support added a file
>> with this flag can be introduced separately. 
> 
> Yes. We discussed it. Thought it may not fit as a separate patch on its
> own. So, i combined it.
> 
> Now, I split patch 9 into two. Let me know if it looks ok.
> 
> ==========================================================================
> 
> Author: Babu Moger <babu.moger@amd.com>
> Date:   Thu Sep 7 18:51:28 2023 -0500
> 
>     x86/resctrl: Add RFTYPE_MON_BASE for MON groups
> 
>     Add the flag RFTYPE_MON_BASE, which contains the files required only
>     for the MON group. Files with these flags are only visible when
>     monitoring is enabled.
> 
>     Signed-off-by: Babu Moger <babu.moger@amd.com>
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
> b/arch/x86/kernel/cpu/resctrl/internal.h
> index ccdbed615d41..72988e5c52a7 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -296,6 +296,8 @@ struct rdtgroup {
>   *     --> RFTYPE_BASE (Files common for both MON and CTRL groups)
>   *         Files: cpus, cpus_list, tasks
>   *
> + *             --> RFTYPE_MON (Files only for MON group)
> + *
>   *             --> RFTYPE_CTRL (Files only for CTRL group)
>   *                 Files: mode, schemata, size
>   *

I think it would be ok to add above hunk as part of the patch
that adds mon_hw_id, similar to what you did in patch 8.

> @@ -315,6 +317,7 @@ struct rdtgroup {
>  #define RFTYPE_MON_INFO                        (RFTYPE_INFO | RFTYPE_MON)
>  #define RFTYPE_TOP_INFO                        (RFTYPE_INFO | RFTYPE_TOP)
>  #define RFTYPE_CTRL_BASE               (RFTYPE_BASE | RFTYPE_CTRL)
> +#define RFTYPE_MON_BASE                        (RFTYPE_BASE | RFTYPE_MON)
> 

This hunk may be appropriate for patch 3.

>  /* List of all resource groups */
>  extern struct list_head rdt_all_groups;
> 

This is not how I expected it to look. There is currently an issue
in resctrl where files with RFTYPE_MON are not created for CTRL_MON
groups. This has not actually been a problem because there are
no files with the RFTYPE_MON flag. This series introduces the first
file with RFTYPE_MON flag and thus this resctrl issue needs to be fixed
before this new file can be supported.

Considering this I expected to see the changes in rdt_get_tree()
and mkdir_rdt_prepare() done as a separate patch. This will add
support for RFTYPE_MON files to resctrl. After this change
a separate patch can add the file that has this flag.

Reinette

Re: [PATCH v9 9/9] x86/resctrl: Display RMID of resource group

[Moger, Babu]

Hi Reinette,

On 9/12/23 12:57, Reinette Chatre wrote:
> Hi Babu,
> 
> On 9/12/2023 10:39 AM, Moger, Babu wrote:
>> On 9/11/23 18:08, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 9/7/2023 4:51 PM, Babu Moger wrote:
>>>> In x86, hardware uses RMID to identify a a monitoring group. When a
>>>> user creates a monitor group these details are not visible. These details
>>>> can help resctrl debugging.
>>>>
>>>> Add RMID(mon_hw_id) to the monitor groups display in resctrl interface.
>>>> Users can see these details when resctrl is mounted with "-o debug" option.
>>>>
>>>> Other architectures do not use "RMID". Use the name mon_hw_id to refer
>>>> to "RMID" in an effort to keep the naming generic.
>>>>
>>>> Add the flag RFTYPE_MON_BASE, which contains the files required only
>>>> for the MON group.
>>>
>>> As I mentioned in [1] I believe adding support for files with RFTYPE_MON flag
>>> to resctrl needs to be in a separate patch. With the core support added a file
>>> with this flag can be introduced separately. 
>>
>> Yes. We discussed it. Thought it may not fit as a separate patch on its
>> own. So, i combined it.
>>
>> Now, I split patch 9 into two. Let me know if it looks ok.
>>
>> ==========================================================================
>>
>> Author: Babu Moger <babu.moger@amd.com>
>> Date:   Thu Sep 7 18:51:28 2023 -0500
>>
>>     x86/resctrl: Add RFTYPE_MON_BASE for MON groups
>>
>>     Add the flag RFTYPE_MON_BASE, which contains the files required only
>>     for the MON group. Files with these flags are only visible when
>>     monitoring is enabled.
>>
>>     Signed-off-by: Babu Moger <babu.moger@amd.com>
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
>> b/arch/x86/kernel/cpu/resctrl/internal.h
>> index ccdbed615d41..72988e5c52a7 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -296,6 +296,8 @@ struct rdtgroup {
>>   *     --> RFTYPE_BASE (Files common for both MON and CTRL groups)
>>   *         Files: cpus, cpus_list, tasks
>>   *
>> + *             --> RFTYPE_MON (Files only for MON group)
>> + *
>>   *             --> RFTYPE_CTRL (Files only for CTRL group)
>>   *                 Files: mode, schemata, size
>>   *
> 
> I think it would be ok to add above hunk as part of the patch
> that adds mon_hw_id, similar to what you did in patch 8.

Ok. Sure.
> 
>> @@ -315,6 +317,7 @@ struct rdtgroup {
>>  #define RFTYPE_MON_INFO                        (RFTYPE_INFO | RFTYPE_MON)
>>  #define RFTYPE_TOP_INFO                        (RFTYPE_INFO | RFTYPE_TOP)
>>  #define RFTYPE_CTRL_BASE               (RFTYPE_BASE | RFTYPE_CTRL)
>> +#define RFTYPE_MON_BASE                        (RFTYPE_BASE | RFTYPE_MON)
>>
> 
> This hunk may be appropriate for patch 3.

Sure.

> 
>>  /* List of all resource groups */
>>  extern struct list_head rdt_all_groups;
>>
> 
> This is not how I expected it to look. There is currently an issue
> in resctrl where files with RFTYPE_MON are not created for CTRL_MON
> groups. This has not actually been a problem because there are
> no files with the RFTYPE_MON flag. This series introduces the first
> file with RFTYPE_MON flag and thus this resctrl issue needs to be fixed
> before this new file can be supported.
Yes.
> 
> Considering this I expected to see the changes in rdt_get_tree()
> and mkdir_rdt_prepare() done as a separate patch. This will add
> support for RFTYPE_MON files to resctrl. After this change
> a separate patch can add the file that has this flag.
> 
Sure. Got it.
Thanks
Babu Moger