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

[PATCH v8 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.
---
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. 

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 (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 group file creation to 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 |  87 +++++++--
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 246 ++++++++++++++++++-------
 3 files changed, 273 insertions(+), 81 deletions(-)

-- 
2.34.1

[PATCH v8 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>
Reviewed-by: Reinette Chatre <reinette.chatre@intel.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);
-- 
2.34.1

[PATCH v8 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");
-- 
2.34.1

[PATCH v8 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;
-- 
2.34.1

[PATCH v8 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 | 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..b09e7abd1299 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
+ *		           File: 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
+ *
  */
 #define RFTYPE_INFO			BIT(0)
 #define RFTYPE_BASE			BIT(1)
-- 
2.34.1

[PATCH v8 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. This also simplifies
cleanup in rdt_kill_sb().

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

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

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 3010e3a1394d..80a4f76bc34b 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2290,14 +2290,6 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable)
 	return 0;
 }
 
-static void cdp_disable_all(void)
-{
-	if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L3))
-		resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
-	if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L2))
-		resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, false);
-}
-
 /*
  * We don't allow rdtgroup directories to be created anywhere
  * except the root directory. Thus when looking for the rdtgroup
@@ -2377,19 +2369,47 @@ static int mkdir_mondata_all(struct kernfs_node *parent_kn,
 			     struct rdtgroup *prgrp,
 			     struct kernfs_node **mon_data_kn);
 
+static void rdt_disable_ctx(void)
+{
+	if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L3))
+		resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
+
+	if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L2))
+		resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, false);
+
+	if (is_mba_sc(&rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl))
+		set_mba_sc(false);
+}
+
 static int rdt_enable_ctx(struct rdt_fs_context *ctx)
 {
 	int ret = 0;
 
-	if (ctx->enable_cdpl2)
+	if (ctx->enable_cdpl2) {
 		ret = resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, true);
+		if (ret)
+			goto out_done;
+	}
 
-	if (!ret && ctx->enable_cdpl3)
+	if (ctx->enable_cdpl3) {
 		ret = resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, true);
+		if (ret)
+			goto out_cdpl2;
+	}
 
-	if (!ret && ctx->enable_mba_mbps)
+	if (ctx->enable_mba_mbps) {
 		ret = set_mba_sc(true);
+		if (ret)
+			goto out_cdpl3;
+	}
+
+	return 0;
 
+out_cdpl3:
+	resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
+out_cdpl2:
+	resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, false);
+out_done:
 	return ret;
 }
 
@@ -2497,13 +2517,13 @@ static int rdt_get_tree(struct fs_context *fc)
 	}
 
 	ret = rdt_enable_ctx(ctx);
-	if (ret < 0)
-		goto out_cdp;
+	if (ret)
+		goto out;
 
 	ret = schemata_list_create();
 	if (ret) {
 		schemata_list_destroy();
-		goto out_mba;
+		goto out_ctx;
 	}
 
 	closid_init();
@@ -2562,11 +2582,8 @@ static int rdt_get_tree(struct fs_context *fc)
 	kernfs_remove(kn_info);
 out_schemata_free:
 	schemata_list_destroy();
-out_mba:
-	if (ctx->enable_mba_mbps)
-		set_mba_sc(false);
-out_cdp:
-	cdp_disable_all();
+out_ctx:
+	rdt_disable_ctx();
 out:
 	rdt_last_cmd_clear();
 	mutex_unlock(&rdtgroup_mutex);
@@ -2798,12 +2815,11 @@ static void rdt_kill_sb(struct super_block *sb)
 	cpus_read_lock();
 	mutex_lock(&rdtgroup_mutex);
 
-	set_mba_sc(false);
+	rdt_disable_ctx();
 
 	/*Put everything back to default values. */
 	for_each_alloc_capable_rdt_resource(r)
 		reset_all_ctrls(r);
-	cdp_disable_all();
 	rmdir_all_sub();
 	rdt_pseudo_lock_release();
 	rdtgroup_default.mode = RDT_MODE_SHAREABLE;
-- 
2.34.1

[PATCH v8 6/8] 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/internal.h |  2 +
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 55 +++++++++++++++-----------
 2 files changed, 33 insertions(+), 24 deletions(-)

diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index b09e7abd1299..44ad98f8c7af 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -611,5 +611,7 @@ 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);
+void rdtgroup_destroy_root(void);
 
 #endif /* _ASM_X86_RESCTRL_INTERNAL_H */
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 80a4f76bc34b..98f9f880c30b 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2516,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();
@@ -2528,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;
@@ -2584,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);
@@ -2654,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;
@@ -2824,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);
@@ -3705,10 +3717,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,
@@ -3716,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;
+}
+
+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;
@@ -3725,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)
@@ -3859,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)
@@ -3898,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;
 }
@@ -3909,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 v8 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 | 18 ++++++++++++++++++
 3 files changed, 24 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 44ad98f8c7af..2fae6d9e24d3 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 98f9f880c30b..94bd69f9964c 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);
@@ -2379,6 +2384,8 @@ static void rdt_disable_ctx(void)
 
 	if (is_mba_sc(&rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl))
 		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 v8 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 to "CLOSID" and "RMID" respectively in
an effort to keep the naming generic.

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

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 Documentation/arch/x86/resctrl.rst     |  8 +++++
 arch/x86/kernel/cpu/resctrl/internal.h |  6 ++++
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 46 ++++++++++++++++++++++++++
 3 files changed, 60 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/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 2fae6d9e24d3..3fa32c1c2677 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -296,9 +296,15 @@ struct rdtgroup {
  *	--> RFTYPE_BASE (Files common for both MON and CTRL groups)
  *	    Files: cpus, cpus_list, tasks
  *
+ *		--> RFTYPE_DEBUG (Files to help resctrl debugging)
+ *		    File: mon_hw_id
+ *
  *		--> 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 94bd69f9964c..e0c2479acf49 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,
+	},
 
 };
 
-- 
2.34.1

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

[Shaopeng Tan (Fujitsu)]

Hi Babu,


> 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 | 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..b09e7abd1299 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
"directory" ->"Directory"

> + *		--> 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
> + *		           File: 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
I think this is reinette's advice,
but "		--> RFTYPE_CTRL (Files only for CTRL group)" looks like a subdirectory.
Is this expected?

Best regards,
Shaopeng TAN

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

[Shaopeng Tan (Fujitsu)]

Hi Babu,

I tested these features and ran the selftests/resctrl test set,
and there is no problem.

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

Best regards,
Shaopeng TAN

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

[Moger, Babu]

Hi Shaopeng,

On 8/23/23 02:06, Shaopeng Tan (Fujitsu) wrote:
> Hi Babu,
> 
> I tested these features and ran the selftests/resctrl test set,
> and there is no problem.
> 
> <Reviewed-by:tan.shaopeng@jp.fujitsu.com>
> <Tested-by:tan.shaopeng@jp.fujitsu.com>

Thank you
Babu Moger

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

[Moger, Babu]

Hi Shaopeng,

On 8/23/23 02:03, Shaopeng Tan (Fujitsu) wrote:
> Hi Babu,
> 
> 
>> 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 | 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..b09e7abd1299 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
> "directory" ->"Directory"

Sorry.. I missed that. Will fix.

> 
>> + *		--> 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
>> + *		           File: 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
> I think this is reinette's advice,
> but "		--> RFTYPE_CTRL (Files only for CTRL group)" looks like a subdirectory.
> Is this expected?

Yes. It is expected. We have two combinations here.
1. RFTYPE_BASE
2. RFTYPE_BASE | RFTYPE_CTRL

Because it is OR'd with two flags, I would say it is displayed as expected.

Thanks
Babu Moger

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

[Reinette Chatre]

Hi Babu,

On 8/21/2023 4:30 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>

After the typo that Shaopeng pointed out is fixed you
can add:

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

Reinette

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

[Reinette Chatre]

Hi Babu,

On 8/21/2023 4:30 PM, Babu Moger wrote:
>  static int rdt_enable_ctx(struct rdt_fs_context *ctx)
>  {
>  	int ret = 0;
>  
> -	if (ctx->enable_cdpl2)
> +	if (ctx->enable_cdpl2) {
>  		ret = resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, true);
> +		if (ret)
> +			goto out_done;
> +	}
>  
> -	if (!ret && ctx->enable_cdpl3)
> +	if (ctx->enable_cdpl3) {
>  		ret = resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, true);
> +		if (ret)
> +			goto out_cdpl2;
> +	}
>  
> -	if (!ret && ctx->enable_mba_mbps)
> +	if (ctx->enable_mba_mbps) {
>  		ret = set_mba_sc(true);
> +		if (ret)
> +			goto out_cdpl3;

An error may be encountered here without CDP ever enabled or just
enabled for L2 or L3. I think that the error unwinding should
take care to not unwind an action that was not done. Considering
the information available I think checking either ctx->enable_...
or the checks used in rdt_disable_ctx() would be ok but for consistency
the resctrl_arch_get_cdp_enabled() checks may be most appropriate.

> +	}
> +
> +	return 0;
>  
> +out_cdpl3:

So here I think there should be a check. 
	if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L3))

> +	resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
> +out_cdpl2:

... and here a check:
	if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L2))

> +	resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, false);
> +out_done:
>  	return ret;
>  }
>  

Reinette

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

[Reinette Chatre]

Hi Babu,

On 8/21/2023 4:30 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>
> ---
>  arch/x86/kernel/cpu/resctrl/internal.h |  2 +
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 55 +++++++++++++++-----------
>  2 files changed, 33 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index b09e7abd1299..44ad98f8c7af 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -611,5 +611,7 @@ 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);
> +void rdtgroup_destroy_root(void);
>  

From what I can tell these functions are only used in rdtgroup.c.
Can this export be avoided by just moving these functions within
rdtgroup.c and making them static?

>  #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 80a4f76bc34b..98f9f880c30b 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -2516,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();
> @@ -2528,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;
> @@ -2584,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);
> @@ -2654,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;
> @@ -2824,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);
> @@ -3705,10 +3717,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,
> @@ -3716,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;
> +}
> +
> +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;
> @@ -3725,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)
> @@ -3859,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)
> @@ -3898,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;
>  }
> @@ -3909,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);
>  }


The rest of this patch looks good to me.

Reinette

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

[Reinette Chatre]

Hi Babu,

On 8/21/2023 4:30 PM, Babu Moger wrote:
> Add "-o debug" option to mount resctrl filesystem in debug mode. This
> option is used for adding extra files to help resctrl debugging.
> 

I think it would help to highlight the new RFTYPE_DEBUG flag.

	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.

Feel free to improve.

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

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

Reinette

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

[Reinette Chatre]

Hi Babu,

On 8/21/2023 4:30 PM, Babu Moger wrote:

...

> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 2fae6d9e24d3..3fa32c1c2677 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -296,9 +296,15 @@ struct rdtgroup {
>   *	--> RFTYPE_BASE (Files common for both MON and CTRL groups)
>   *	    Files: cpus, cpus_list, tasks
>   *
> + *		--> RFTYPE_DEBUG (Files to help resctrl debugging)
> + *		    File: mon_hw_id
> + *

This does not look right. I think mon_hw_id should have RFTYPE_MON
(more below).

>   *		--> 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 94bd69f9964c..e0c2479acf49 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,

I missed this earlier but I think this also needs a RFTYPE_MON.
Perhaps patch 3 can introduce a new RFTYPE_MON_BASE to not
have the flags of the two new files look too different?

> +	},
>  	{
>  		.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,
> +	},
>  
>  };
>  

Reinette

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

[Moger, Babu]

Hi Reinette,

On 8/29/23 15:08, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/21/2023 4:30 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>
> 
> After the typo that Shaopeng pointed out is fixed you
> can add:

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

-- 
Thanks
Babu Moger

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

[Moger, Babu]

Hi Reinette,

On 8/29/23 15:10, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/21/2023 4:30 PM, Babu Moger wrote:
>>  static int rdt_enable_ctx(struct rdt_fs_context *ctx)
>>  {
>>  	int ret = 0;
>>  
>> -	if (ctx->enable_cdpl2)
>> +	if (ctx->enable_cdpl2) {
>>  		ret = resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, true);
>> +		if (ret)
>> +			goto out_done;
>> +	}
>>  
>> -	if (!ret && ctx->enable_cdpl3)
>> +	if (ctx->enable_cdpl3) {
>>  		ret = resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, true);
>> +		if (ret)
>> +			goto out_cdpl2;
>> +	}
>>  
>> -	if (!ret && ctx->enable_mba_mbps)
>> +	if (ctx->enable_mba_mbps) {
>>  		ret = set_mba_sc(true);
>> +		if (ret)
>> +			goto out_cdpl3;
> 
> An error may be encountered here without CDP ever enabled or just
> enabled for L2 or L3. I think that the error unwinding should
> take care to not unwind an action that was not done. Considering
> the information available I think checking either ctx->enable_...
> or the checks used in rdt_disable_ctx() would be ok but for consistency
> the resctrl_arch_get_cdp_enabled() checks may be most appropriate.
> 
>> +	}
>> +
>> +	return 0;
>>  
>> +out_cdpl3:
> 
> So here I think there should be a check. 
> 	if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L3))
> 
>> +	resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
>> +out_cdpl2:
> 
> ... and here a check:
> 	if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L2))


I know it does not hurt to add these checks.  But, it may be unnecessary
considering  cdp_disable() has the check "if (r_hw->cdp_enabled)" already.
Both are same checks. What do you think?
-- 
Thanks
Babu Moger

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

[Reinette Chatre]

Hi Babu,

On 8/30/2023 9:38 AM, Moger, Babu wrote:
> On 8/29/23 15:10, Reinette Chatre wrote:
>> On 8/21/2023 4:30 PM, Babu Moger wrote:
>>>  static int rdt_enable_ctx(struct rdt_fs_context *ctx)
>>>  {
>>>  	int ret = 0;
>>>  
>>> -	if (ctx->enable_cdpl2)
>>> +	if (ctx->enable_cdpl2) {
>>>  		ret = resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, true);
>>> +		if (ret)
>>> +			goto out_done;
>>> +	}
>>>  
>>> -	if (!ret && ctx->enable_cdpl3)
>>> +	if (ctx->enable_cdpl3) {
>>>  		ret = resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, true);
>>> +		if (ret)
>>> +			goto out_cdpl2;
>>> +	}
>>>  
>>> -	if (!ret && ctx->enable_mba_mbps)
>>> +	if (ctx->enable_mba_mbps) {
>>>  		ret = set_mba_sc(true);
>>> +		if (ret)
>>> +			goto out_cdpl3;
>>
>> An error may be encountered here without CDP ever enabled or just
>> enabled for L2 or L3. I think that the error unwinding should
>> take care to not unwind an action that was not done. Considering
>> the information available I think checking either ctx->enable_...
>> or the checks used in rdt_disable_ctx() would be ok but for consistency
>> the resctrl_arch_get_cdp_enabled() checks may be most appropriate.
>>
>>> +	}
>>> +
>>> +	return 0;
>>>  
>>> +out_cdpl3:
>>
>> So here I think there should be a check. 
>> 	if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L3))
>>
>>> +	resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
>>> +out_cdpl2:
>>
>> ... and here a check:
>> 	if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L2))
> 
> 
> I know it does not hurt to add these checks.  But, it may be unnecessary
> considering  cdp_disable() has the check "if (r_hw->cdp_enabled)" already.
> Both are same checks. What do you think?

Yes, good point. Thank you for checking. Considering this it looks like
rdt_disable_ctx() can be simplified also by removing those CDP
enabled checks from it? Also looks like rdt_disable_ctx()-> set_mba_sc(false)
can be called unconditionally.

Reinette

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

[Moger, Babu]

Hi Reinette,

On 8/30/23 12:56, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/30/2023 9:38 AM, Moger, Babu wrote:
>> On 8/29/23 15:10, Reinette Chatre wrote:
>>> On 8/21/2023 4:30 PM, Babu Moger wrote:
>>>>  static int rdt_enable_ctx(struct rdt_fs_context *ctx)
>>>>  {
>>>>  	int ret = 0;
>>>>  
>>>> -	if (ctx->enable_cdpl2)
>>>> +	if (ctx->enable_cdpl2) {
>>>>  		ret = resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, true);
>>>> +		if (ret)
>>>> +			goto out_done;
>>>> +	}
>>>>  
>>>> -	if (!ret && ctx->enable_cdpl3)
>>>> +	if (ctx->enable_cdpl3) {
>>>>  		ret = resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, true);
>>>> +		if (ret)
>>>> +			goto out_cdpl2;
>>>> +	}
>>>>  
>>>> -	if (!ret && ctx->enable_mba_mbps)
>>>> +	if (ctx->enable_mba_mbps) {
>>>>  		ret = set_mba_sc(true);
>>>> +		if (ret)
>>>> +			goto out_cdpl3;
>>>
>>> An error may be encountered here without CDP ever enabled or just
>>> enabled for L2 or L3. I think that the error unwinding should
>>> take care to not unwind an action that was not done. Considering
>>> the information available I think checking either ctx->enable_...
>>> or the checks used in rdt_disable_ctx() would be ok but for consistency
>>> the resctrl_arch_get_cdp_enabled() checks may be most appropriate.
>>>
>>>> +	}
>>>> +
>>>> +	return 0;
>>>>  
>>>> +out_cdpl3:
>>>
>>> So here I think there should be a check. 
>>> 	if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L3))
>>>
>>>> +	resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
>>>> +out_cdpl2:
>>>
>>> ... and here a check:
>>> 	if (resctrl_arch_get_cdp_enabled(RDT_RESOURCE_L2))
>>
>>
>> I know it does not hurt to add these checks.  But, it may be unnecessary
>> considering  cdp_disable() has the check "if (r_hw->cdp_enabled)" already.
>> Both are same checks. What do you think?
> 
> Yes, good point. Thank you for checking. Considering this it looks like
> rdt_disable_ctx() can be simplified also by removing those CDP
> enabled checks from it? Also looks like rdt_disable_ctx()-> set_mba_sc(false)
> can be called unconditionally.

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

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

[Moger, Babu]

Hi Reinette,

On 8/29/23 15:11, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/21/2023 4:30 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>
>> ---
>>  arch/x86/kernel/cpu/resctrl/internal.h |  2 +
>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 55 +++++++++++++++-----------
>>  2 files changed, 33 insertions(+), 24 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index b09e7abd1299..44ad98f8c7af 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -611,5 +611,7 @@ 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);
>> +void rdtgroup_destroy_root(void);
>>  
> 
> From what I can tell these functions are only used in rdtgroup.c.
> Can this export be avoided by just moving these functions within
> rdtgroup.c and making them static?

Yes. It is used only in rdtgroup.c. We can make this static by adding the
prototypes of these function in the beginning of rdtgroup.c file to avoid
implicit declaration compiler errors.

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 189c51c479d3..a97118b6f9f0 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)


> 
>>  #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 80a4f76bc34b..98f9f880c30b 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -2516,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();
>> @@ -2528,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;
>> @@ -2584,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);
>> @@ -2654,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;
>> @@ -2824,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);
>> @@ -3705,10 +3717,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,
>> @@ -3716,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;
>> +}
>> +
>> +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;
>> @@ -3725,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)
>> @@ -3859,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)
>> @@ -3898,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;
>>  }
>> @@ -3909,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);
>>  }
> 
> 
> The rest of this patch looks good to me.-- 
Thanks
Babu Moger

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

[Reinette Chatre]

Hi Babu,

On 8/30/2023 12:50 PM, Moger, Babu wrote:
> Hi Reinette,
> 
> On 8/29/23 15:11, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 8/21/2023 4:30 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>
>>> ---
>>>  arch/x86/kernel/cpu/resctrl/internal.h |  2 +
>>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 55 +++++++++++++++-----------
>>>  2 files changed, 33 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>>> index b09e7abd1299..44ad98f8c7af 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>> @@ -611,5 +611,7 @@ 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);
>>> +void rdtgroup_destroy_root(void);
>>>  
>>
>> From what I can tell these functions are only used in rdtgroup.c.
>> Can this export be avoided by just moving these functions within
>> rdtgroup.c and making them static?
> 
> Yes. It is used only in rdtgroup.c. We can make this static by adding the
> prototypes of these function in the beginning of rdtgroup.c file to avoid
> implicit declaration compiler errors.

Why not just place the functions earlier in rdtgroup.c so that they are
located before all callers? 

Reinette

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

[Moger, Babu]

Hi Reinette,

On 8/30/23 15:00, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/30/2023 12:50 PM, Moger, Babu wrote:
>> Hi Reinette,
>>
>> On 8/29/23 15:11, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 8/21/2023 4:30 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>
>>>> ---
>>>>  arch/x86/kernel/cpu/resctrl/internal.h |  2 +
>>>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 55 +++++++++++++++-----------
>>>>  2 files changed, 33 insertions(+), 24 deletions(-)
>>>>
>>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>>>> index b09e7abd1299..44ad98f8c7af 100644
>>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>>> @@ -611,5 +611,7 @@ 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);
>>>> +void rdtgroup_destroy_root(void);
>>>>  
>>>
>>> From what I can tell these functions are only used in rdtgroup.c.
>>> Can this export be avoided by just moving these functions within
>>> rdtgroup.c and making them static?
>>
>> Yes. It is used only in rdtgroup.c. We can make this static by adding the
>> prototypes of these function in the beginning of rdtgroup.c file to avoid
>> implicit declaration compiler errors.
> 
> Why not just place the functions earlier in rdtgroup.c so that they are
> located before all callers? 

Couple of problems with that.
1.  rdtgroup_setup_root needs the the definition of
rdtgroup_kf_syscall_ops which is defined later in the file.

Static struct kernfs_syscall_ops rdtgroup_kf_syscall_ops = {
         .mkdir          = rdtgroup_mkdir,
         .rmdir          = rdtgroup_rmdir,
         .rename         = rdtgroup_rename,
         .show_options   = rdtgroup_show_options,
};

2. rdtgroup_setup_root is called in rdt_get_tree which is defined earlier
in the file.

So, this needs re-arrange of all these functions. That is reason I made
these functions global. Thought it may be too much a change for this purpose.
-- 
Thanks
Babu Moger

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

[Moger, Babu]

Hi Reinette,

On 8/29/23 15:12, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/21/2023 4:30 PM, Babu Moger wrote:
>> Add "-o debug" option to mount resctrl filesystem in debug mode. This
>> option is used for adding extra files to help resctrl debugging.
>>
> 
> I think it would help to highlight the new RFTYPE_DEBUG flag.
> 
> 	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.

Sure.

> 
> Feel free to improve.
> 
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
> 
> Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>-- 
Thanks
Babu Moger

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

[Reinette Chatre]

Hi Babu,

On 8/30/2023 2:18 PM, Moger, Babu wrote:
> On 8/30/23 15:00, Reinette Chatre wrote:
>> On 8/30/2023 12:50 PM, Moger, Babu wrote:
>>> On 8/29/23 15:11, Reinette Chatre wrote:
>>>> On 8/21/2023 4:30 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>
>>>>> ---
>>>>>  arch/x86/kernel/cpu/resctrl/internal.h |  2 +
>>>>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 55 +++++++++++++++-----------
>>>>>  2 files changed, 33 insertions(+), 24 deletions(-)
>>>>>
>>>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>>>>> index b09e7abd1299..44ad98f8c7af 100644
>>>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>>>> @@ -611,5 +611,7 @@ 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);
>>>>> +void rdtgroup_destroy_root(void);
>>>>>  
>>>>
>>>> From what I can tell these functions are only used in rdtgroup.c.
>>>> Can this export be avoided by just moving these functions within
>>>> rdtgroup.c and making them static?
>>>
>>> Yes. It is used only in rdtgroup.c. We can make this static by adding the
>>> prototypes of these function in the beginning of rdtgroup.c file to avoid
>>> implicit declaration compiler errors.
>>
>> Why not just place the functions earlier in rdtgroup.c so that they are
>> located before all callers? 
> 
> Couple of problems with that.
> 1.  rdtgroup_setup_root needs the the definition of
> rdtgroup_kf_syscall_ops which is defined later in the file.
> 
> Static struct kernfs_syscall_ops rdtgroup_kf_syscall_ops = {
>          .mkdir          = rdtgroup_mkdir,
>          .rmdir          = rdtgroup_rmdir,
>          .rename         = rdtgroup_rename,
>          .show_options   = rdtgroup_show_options,
> };
> 
> 2. rdtgroup_setup_root is called in rdt_get_tree which is defined earlier
> in the file.
> 
> So, this needs re-arrange of all these functions. That is reason I made
> these functions global. Thought it may be too much a change for this purpose.

I see, yes, to accomplish this would trigger a lot of churn and also seem
to cascade into other dependencies needing to be taken into account.
As you suggested the static declaration can be added to the top of rdtgroup.c
as proposal for the next stage.

Reinette

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

[Moger, Babu]

Hi Reinette,

On 8/29/23 15:14, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/21/2023 4:30 PM, Babu Moger wrote:
> 
> ...
> 
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 2fae6d9e24d3..3fa32c1c2677 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -296,9 +296,15 @@ struct rdtgroup {
>>   *	--> RFTYPE_BASE (Files common for both MON and CTRL groups)
>>   *	    Files: cpus, cpus_list, tasks
>>   *
>> + *		--> RFTYPE_DEBUG (Files to help resctrl debugging)
>> + *		    File: mon_hw_id
>> + *
> 
> This does not look right. I think mon_hw_id should have RFTYPE_MON
> (more below).

I am not sure about this (more below).

> 
>>   *		--> 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 94bd69f9964c..e0c2479acf49 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,
> 
> I missed this earlier but I think this also needs a RFTYPE_MON.
> Perhaps patch 3 can introduce a new RFTYPE_MON_BASE to not
> have the flags of the two new files look too different?

We have two types of files in base directory structure.

 if (rtype == RDTCTRL_GROUP)
                files = RFTYPE_BASE | RFTYPE_CTRL;
        else
                files = RFTYPE_BASE | RFTYPE_MON;

1. RFTYPE_BASE | RFTYPE_CTRL;
   Files for the control group only.

2. RFTYPE_BASE | RFTYPE_MON;
   Files for both control and mon groups. However, RFTYPE_MON is not used
for any files. It is only RFTYPE_BASE.

Because of the check in rdtgroup_add_files it all works fine.
For the control group it creates files with RFTYPE_BASE | RFTYPE_CTRL and
RFTYPE_BASE.

For the mon group it creates files with RFTYPE_BASE only.

Adding FTYPE_MON_BASE will be a problem.

-- 
Thanks
Babu Moger

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

[Moger, Babu]

Hi Reinette,

On 8/30/23 17:05, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/30/2023 2:18 PM, Moger, Babu wrote:
>> On 8/30/23 15:00, Reinette Chatre wrote:
>>> On 8/30/2023 12:50 PM, Moger, Babu wrote:
>>>> On 8/29/23 15:11, Reinette Chatre wrote:
>>>>> On 8/21/2023 4:30 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>
>>>>>> ---
>>>>>>  arch/x86/kernel/cpu/resctrl/internal.h |  2 +
>>>>>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 55 +++++++++++++++-----------
>>>>>>  2 files changed, 33 insertions(+), 24 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>>>>>> index b09e7abd1299..44ad98f8c7af 100644
>>>>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>>>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>>>>> @@ -611,5 +611,7 @@ 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);
>>>>>> +void rdtgroup_destroy_root(void);
>>>>>>  
>>>>>
>>>>> From what I can tell these functions are only used in rdtgroup.c.
>>>>> Can this export be avoided by just moving these functions within
>>>>> rdtgroup.c and making them static?
>>>>
>>>> Yes. It is used only in rdtgroup.c. We can make this static by adding the
>>>> prototypes of these function in the beginning of rdtgroup.c file to avoid
>>>> implicit declaration compiler errors.
>>>
>>> Why not just place the functions earlier in rdtgroup.c so that they are
>>> located before all callers? 
>>
>> Couple of problems with that.
>> 1.  rdtgroup_setup_root needs the the definition of
>> rdtgroup_kf_syscall_ops which is defined later in the file.
>>
>> Static struct kernfs_syscall_ops rdtgroup_kf_syscall_ops = {
>>          .mkdir          = rdtgroup_mkdir,
>>          .rmdir          = rdtgroup_rmdir,
>>          .rename         = rdtgroup_rename,
>>          .show_options   = rdtgroup_show_options,
>> };
>>
>> 2. rdtgroup_setup_root is called in rdt_get_tree which is defined earlier
>> in the file.
>>
>> So, this needs re-arrange of all these functions. That is reason I made
>> these functions global. Thought it may be too much a change for this purpose.
> 
> I see, yes, to accomplish this would trigger a lot of churn and also seem
> to cascade into other dependencies needing to be taken into account.
> As you suggested the static declaration can be added to the top of rdtgroup.c
> as proposal for the next stage.
> 

Sure.
Thanks
Babu Moger

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

[Reinette Chatre]

Hi Babu,

On 8/30/2023 4:19 PM, Moger, Babu wrote:
> On 8/29/23 15:14, Reinette Chatre wrote:
>> On 8/21/2023 4:30 PM, Babu Moger wrote:
>>
>> ...
>>
>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>>> index 2fae6d9e24d3..3fa32c1c2677 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>> @@ -296,9 +296,15 @@ struct rdtgroup {
>>>   *	--> RFTYPE_BASE (Files common for both MON and CTRL groups)
>>>   *	    Files: cpus, cpus_list, tasks
>>>   *
>>> + *		--> RFTYPE_DEBUG (Files to help resctrl debugging)
>>> + *		    File: mon_hw_id
>>> + *
>>
>> This does not look right. I think mon_hw_id should have RFTYPE_MON
>> (more below).
> 
> I am not sure about this (more below).
> 
>>
>>>   *		--> 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 94bd69f9964c..e0c2479acf49 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,
>>
>> I missed this earlier but I think this also needs a RFTYPE_MON.
>> Perhaps patch 3 can introduce a new RFTYPE_MON_BASE to not
>> have the flags of the two new files look too different?
> 
> We have two types of files in base directory structure.
> 
>  if (rtype == RDTCTRL_GROUP)
>                 files = RFTYPE_BASE | RFTYPE_CTRL;
>         else
>                 files = RFTYPE_BASE | RFTYPE_MON;
> 
> 1. RFTYPE_BASE | RFTYPE_CTRL;
>    Files for the control group only.
> 
> 2. RFTYPE_BASE | RFTYPE_MON;
>    Files for both control and mon groups. However, RFTYPE_MON is not used
> for any files. It is only RFTYPE_BASE.
> 
> Because of the check in rdtgroup_add_files it all works fine.
> For the control group it creates files with RFTYPE_BASE | RFTYPE_CTRL and
> RFTYPE_BASE.
> 
> For the mon group it creates files with RFTYPE_BASE only.

This describes current behavior because there are no resctrl
files in base that are specific to monitoring, mon_hw_id is the
first.

This does not mean that the new file mon_hw_id should just have
RFTYPE_BASE because that would result in mon_hw_id being created
for all control groups, even those that do not support monitoring
Having mon_hw_id in resctrl for a group that does not support
monitoring is not correct.

You should be able to reproduce this when booting your system
with rdt=!cmt,!mbmlocal,!mbmtotal.

> 
> Adding FTYPE_MON_BASE will be a problem.
> 

Yes, this change does not just involve assigning the RFTYPE_MON
to mon_hw_id. As you describe mkdir_rdt_prepare() does not take
RFTYPE_MON into account when creating the files. Could this not just
be a straightforward change to have it append RFTYPE_MON to the flags
of files needing to be created for a CTRL_MON group? This would
support new resource groups and then the default resource group
would need to be taken into account also. What am I missing?

Reinette

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

[Moger, Babu]

Hi Reinette,

On 8/31/23 12:42, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/30/2023 4:19 PM, Moger, Babu wrote:
>> On 8/29/23 15:14, Reinette Chatre wrote:
>>> On 8/21/2023 4:30 PM, Babu Moger wrote:
>>>
>>> ...
>>>
>>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>>>> index 2fae6d9e24d3..3fa32c1c2677 100644
>>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>>> @@ -296,9 +296,15 @@ struct rdtgroup {
>>>>   *	--> RFTYPE_BASE (Files common for both MON and CTRL groups)
>>>>   *	    Files: cpus, cpus_list, tasks
>>>>   *
>>>> + *		--> RFTYPE_DEBUG (Files to help resctrl debugging)
>>>> + *		    File: mon_hw_id
>>>> + *
>>>
>>> This does not look right. I think mon_hw_id should have RFTYPE_MON
>>> (more below).
>>
>> I am not sure about this (more below).
>>
>>>
>>>>   *		--> 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 94bd69f9964c..e0c2479acf49 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,
>>>
>>> I missed this earlier but I think this also needs a RFTYPE_MON.
>>> Perhaps patch 3 can introduce a new RFTYPE_MON_BASE to not
>>> have the flags of the two new files look too different?
>>
>> We have two types of files in base directory structure.
>>
>>  if (rtype == RDTCTRL_GROUP)
>>                 files = RFTYPE_BASE | RFTYPE_CTRL;
>>         else
>>                 files = RFTYPE_BASE | RFTYPE_MON;
>>
>> 1. RFTYPE_BASE | RFTYPE_CTRL;
>>    Files for the control group only.
>>
>> 2. RFTYPE_BASE | RFTYPE_MON;
>>    Files for both control and mon groups. However, RFTYPE_MON is not used
>> for any files. It is only RFTYPE_BASE.
>>
>> Because of the check in rdtgroup_add_files it all works fine.
>> For the control group it creates files with RFTYPE_BASE | RFTYPE_CTRL and
>> RFTYPE_BASE.
>>
>> For the mon group it creates files with RFTYPE_BASE only.
> 
> This describes current behavior because there are no resctrl
> files in base that are specific to monitoring, mon_hw_id is the
> first.
> 
> This does not mean that the new file mon_hw_id should just have
> RFTYPE_BASE because that would result in mon_hw_id being created
> for all control groups, even those that do not support monitoring
> Having mon_hw_id in resctrl for a group that does not support
> monitoring is not correct.
> 
> You should be able to reproduce this when booting your system
> with rdt=!cmt,!mbmlocal,!mbmtotal.

You are right. I reproduced it.

> 
>>
>> Adding FTYPE_MON_BASE will be a problem.
>>
> 
> Yes, this change does not just involve assigning the RFTYPE_MON
> to mon_hw_id. As you describe mkdir_rdt_prepare() does not take
> RFTYPE_MON into account when creating the files. Could this not just
> be a straightforward change to have it append RFTYPE_MON to the flags
> of files needing to be created for a CTRL_MON group? This would
> support new resource groups and then the default resource group
> would need to be taken into account also. What am I missing?
> 

It is not straight forward. We have have to handle few more things.
1. Base directory creation.
2. Mon directory creation after the base.

I got it working with this patches.  We may be able to clean it little
more or we can split also.

diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
b/arch/x86/kernel/cpu/resctrl/internal.h
index 3fa32c1c2677..e2f3197f1c24 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -318,6 +318,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 e0c2479acf49..1f9abab7b9bd 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1874,7 +1874,7 @@ static struct rftype res_common_files[] = {
                .mode           = 0444,
                .kf_ops         = &rdtgroup_kf_single_ops,
                .seq_show       = rdtgroup_rmid_show,
-               .fflags         = RFTYPE_BASE | RFTYPE_DEBUG,
+               .fflags         = RFTYPE_MON_BASE | RFTYPE_DEBUG,
        },
        {
                .name           = "schemata",
@@ -2558,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);
+       uint flags = RFTYPE_CTRL_BASE;
        struct rdt_domain *dom;
        struct rdt_resource *r;
        int ret;
@@ -2588,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;

@@ -3336,6 +3340,9 @@ static int mkdir_rdt_prepare(struct kernfs_node
*parent_kn,
        else
                files = RFTYPE_BASE | RFTYPE_MON;

+       if (rdt_mon_capable)
+               files |= RFTYPE_MON;
+
        ret = rdtgroup_add_files(kn, files);
        if (ret) {
                rdt_last_cmd_puts("kernfs fill error\n");


-- 
Thanks
Babu Moger

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

[Reinette Chatre]

Hi Babu,

On 8/31/2023 4:58 PM, Moger, Babu wrote:
> On 8/31/23 12:42, Reinette Chatre wrote:
>> On 8/30/2023 4:19 PM, Moger, Babu wrote:
>>> On 8/29/23 15:14, Reinette Chatre wrote:
>>>> On 8/21/2023 4:30 PM, Babu Moger wrote:
>>>>
>>>> ...
>>>>
>>>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>>>>> index 2fae6d9e24d3..3fa32c1c2677 100644
>>>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>>>> @@ -296,9 +296,15 @@ struct rdtgroup {
>>>>>   *	--> RFTYPE_BASE (Files common for both MON and CTRL groups)
>>>>>   *	    Files: cpus, cpus_list, tasks
>>>>>   *
>>>>> + *		--> RFTYPE_DEBUG (Files to help resctrl debugging)
>>>>> + *		    File: mon_hw_id
>>>>> + *
>>>>
>>>> This does not look right. I think mon_hw_id should have RFTYPE_MON
>>>> (more below).
>>>
>>> I am not sure about this (more below).
>>>
>>>>
>>>>>   *		--> 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 94bd69f9964c..e0c2479acf49 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,
>>>>
>>>> I missed this earlier but I think this also needs a RFTYPE_MON.
>>>> Perhaps patch 3 can introduce a new RFTYPE_MON_BASE to not
>>>> have the flags of the two new files look too different?
>>>
>>> We have two types of files in base directory structure.
>>>
>>>  if (rtype == RDTCTRL_GROUP)
>>>                 files = RFTYPE_BASE | RFTYPE_CTRL;
>>>         else
>>>                 files = RFTYPE_BASE | RFTYPE_MON;
>>>
>>> 1. RFTYPE_BASE | RFTYPE_CTRL;
>>>    Files for the control group only.
>>>
>>> 2. RFTYPE_BASE | RFTYPE_MON;
>>>    Files for both control and mon groups. However, RFTYPE_MON is not used
>>> for any files. It is only RFTYPE_BASE.
>>>
>>> Because of the check in rdtgroup_add_files it all works fine.
>>> For the control group it creates files with RFTYPE_BASE | RFTYPE_CTRL and
>>> RFTYPE_BASE.
>>>
>>> For the mon group it creates files with RFTYPE_BASE only.
>>
>> This describes current behavior because there are no resctrl
>> files in base that are specific to monitoring, mon_hw_id is the
>> first.
>>
>> This does not mean that the new file mon_hw_id should just have
>> RFTYPE_BASE because that would result in mon_hw_id being created
>> for all control groups, even those that do not support monitoring
>> Having mon_hw_id in resctrl for a group that does not support
>> monitoring is not correct.
>>
>> You should be able to reproduce this when booting your system
>> with rdt=!cmt,!mbmlocal,!mbmtotal.
> 
> You are right. I reproduced it.
> 
>>
>>>
>>> Adding FTYPE_MON_BASE will be a problem.
>>>
>>
>> Yes, this change does not just involve assigning the RFTYPE_MON
>> to mon_hw_id. As you describe mkdir_rdt_prepare() does not take
>> RFTYPE_MON into account when creating the files. Could this not just
>> be a straightforward change to have it append RFTYPE_MON to the flags
>> of files needing to be created for a CTRL_MON group? This would
>> support new resource groups and then the default resource group
>> would need to be taken into account also. What am I missing?
>>
> 
> It is not straight forward. We have have to handle few more things.
> 1. Base directory creation.
> 2. Mon directory creation after the base.
> 

heh ... these are not a "few more things" ... these are exactly
the items I mentioned: "base directory creation" is taking into account
the default resource group and "mon directory creation after the
base" are the changes needed in mkdir_rdt_prepare() where RFTYPE_MON
is appended to the flags.

> I got it working with this patches.  We may be able to clean it little
> more or we can split also.

I think it would make things easier to understand if there
is a separate patch that adds support for files with
RFTYPE_MON flag.

> 
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
> b/arch/x86/kernel/cpu/resctrl/internal.h
> index 3fa32c1c2677..e2f3197f1c24 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -318,6 +318,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 e0c2479acf49..1f9abab7b9bd 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1874,7 +1874,7 @@ static struct rftype res_common_files[] = {
>                 .mode           = 0444,
>                 .kf_ops         = &rdtgroup_kf_single_ops,
>                 .seq_show       = rdtgroup_rmid_show,
> -               .fflags         = RFTYPE_BASE | RFTYPE_DEBUG,
> +               .fflags         = RFTYPE_MON_BASE | RFTYPE_DEBUG,
>         },
>         {
>                 .name           = "schemata",
> @@ -2558,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);
> +       uint flags = RFTYPE_CTRL_BASE;

I assume that you may have just copied this from mkdir_rdt_prepare() but
I think this should rather match the type as this is used (unsigned long).

>         struct rdt_domain *dom;
>         struct rdt_resource *r;
>         int ret;
> @@ -2588,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;
> 
> @@ -3336,6 +3340,9 @@ static int mkdir_rdt_prepare(struct kernfs_node
> *parent_kn,
>         else
>                 files = RFTYPE_BASE | RFTYPE_MON;
> 
> +       if (rdt_mon_capable)
> +               files |= RFTYPE_MON;
> +

Is this not redundant considering what just happened a few lines above?

>         ret = rdtgroup_add_files(kn, files);
>         if (ret) {
>                 rdt_last_cmd_puts("kernfs fill error\n");
> 
> 

Reinette

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

[Moger, Babu]

Hi Reinette,

On 8/31/23 19:43, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/31/2023 4:58 PM, Moger, Babu wrote:
>> On 8/31/23 12:42, Reinette Chatre wrote:
>>> On 8/30/2023 4:19 PM, Moger, Babu wrote:
>>>> On 8/29/23 15:14, Reinette Chatre wrote:
>>>>> On 8/21/2023 4:30 PM, Babu Moger wrote:
>>>>>
>>>>> ...
>>>>>
>>>>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>>>>>> index 2fae6d9e24d3..3fa32c1c2677 100644
>>>>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>>>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>>>>> @@ -296,9 +296,15 @@ struct rdtgroup {
>>>>>>   *	--> RFTYPE_BASE (Files common for both MON and CTRL groups)
>>>>>>   *	    Files: cpus, cpus_list, tasks
>>>>>>   *
>>>>>> + *		--> RFTYPE_DEBUG (Files to help resctrl debugging)
>>>>>> + *		    File: mon_hw_id
>>>>>> + *
>>>>>
>>>>> This does not look right. I think mon_hw_id should have RFTYPE_MON
>>>>> (more below).
>>>>
>>>> I am not sure about this (more below).
>>>>
>>>>>
>>>>>>   *		--> 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 94bd69f9964c..e0c2479acf49 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,
>>>>>
>>>>> I missed this earlier but I think this also needs a RFTYPE_MON.
>>>>> Perhaps patch 3 can introduce a new RFTYPE_MON_BASE to not
>>>>> have the flags of the two new files look too different?
>>>>
>>>> We have two types of files in base directory structure.
>>>>
>>>>  if (rtype == RDTCTRL_GROUP)
>>>>                 files = RFTYPE_BASE | RFTYPE_CTRL;
>>>>         else
>>>>                 files = RFTYPE_BASE | RFTYPE_MON;
>>>>
>>>> 1. RFTYPE_BASE | RFTYPE_CTRL;
>>>>    Files for the control group only.
>>>>
>>>> 2. RFTYPE_BASE | RFTYPE_MON;
>>>>    Files for both control and mon groups. However, RFTYPE_MON is not used
>>>> for any files. It is only RFTYPE_BASE.
>>>>
>>>> Because of the check in rdtgroup_add_files it all works fine.
>>>> For the control group it creates files with RFTYPE_BASE | RFTYPE_CTRL and
>>>> RFTYPE_BASE.
>>>>
>>>> For the mon group it creates files with RFTYPE_BASE only.
>>>
>>> This describes current behavior because there are no resctrl
>>> files in base that are specific to monitoring, mon_hw_id is the
>>> first.
>>>
>>> This does not mean that the new file mon_hw_id should just have
>>> RFTYPE_BASE because that would result in mon_hw_id being created
>>> for all control groups, even those that do not support monitoring
>>> Having mon_hw_id in resctrl for a group that does not support
>>> monitoring is not correct.
>>>
>>> You should be able to reproduce this when booting your system
>>> with rdt=!cmt,!mbmlocal,!mbmtotal.
>>
>> You are right. I reproduced it.
>>
>>>
>>>>
>>>> Adding FTYPE_MON_BASE will be a problem.
>>>>
>>>
>>> Yes, this change does not just involve assigning the RFTYPE_MON
>>> to mon_hw_id. As you describe mkdir_rdt_prepare() does not take
>>> RFTYPE_MON into account when creating the files. Could this not just
>>> be a straightforward change to have it append RFTYPE_MON to the flags
>>> of files needing to be created for a CTRL_MON group? This would
>>> support new resource groups and then the default resource group
>>> would need to be taken into account also. What am I missing?
>>>
>>
>> It is not straight forward. We have have to handle few more things.
>> 1. Base directory creation.
>> 2. Mon directory creation after the base.
>>
> 
> heh ... these are not a "few more things" ... these are exactly
> the items I mentioned: "base directory creation" is taking into account
> the default resource group and "mon directory creation after the
> base" are the changes needed in mkdir_rdt_prepare() where RFTYPE_MON
> is appended to the flags.

Ok. Got it.
> 
>> I got it working with this patches.  We may be able to clean it little
>> more or we can split also.
> 
> I think it would make things easier to understand if there
> is a separate patch that adds support for files with
> RFTYPE_MON flag.

Ok. Sure,

> 
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
>> b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 3fa32c1c2677..e2f3197f1c24 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -318,6 +318,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 e0c2479acf49..1f9abab7b9bd 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -1874,7 +1874,7 @@ static struct rftype res_common_files[] = {
>>                 .mode           = 0444,
>>                 .kf_ops         = &rdtgroup_kf_single_ops,
>>                 .seq_show       = rdtgroup_rmid_show,
>> -               .fflags         = RFTYPE_BASE | RFTYPE_DEBUG,
>> +               .fflags         = RFTYPE_MON_BASE | RFTYPE_DEBUG,
>>         },
>>         {
>>                 .name           = "schemata",
>> @@ -2558,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);
>> +       uint flags = RFTYPE_CTRL_BASE;
> 
> I assume that you may have just copied this from mkdir_rdt_prepare() but
> I think this should rather match the type as this is used (unsigned long).

Yes. Will correct it.

> 
>>         struct rdt_domain *dom;
>>         struct rdt_resource *r;
>>         int ret;
>> @@ -2588,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;
>>
>> @@ -3336,6 +3340,9 @@ static int mkdir_rdt_prepare(struct kernfs_node
>> *parent_kn,
>>         else
>>                 files = RFTYPE_BASE | RFTYPE_MON;
>>
>> +       if (rdt_mon_capable)
>> +               files |= RFTYPE_MON;
>> +
> 
> Is this not redundant considering what just happened a few lines above?

Yea. Right. I will change the previous line to

files = RFTYPE_BASE;

Thanks
Babu Moger

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

[Reinette Chatre]

Hi Babu,

On 9/1/2023 10:28 AM, Moger, Babu wrote:
> On 8/31/23 19:43, Reinette Chatre wrote:
>> On 8/31/2023 4:58 PM, Moger, Babu wrote:
>>> @@ -3336,6 +3340,9 @@ static int mkdir_rdt_prepare(struct kernfs_node
>>> *parent_kn,
>>>         else
>>>                 files = RFTYPE_BASE | RFTYPE_MON;
>>>
>>> +       if (rdt_mon_capable)
>>> +               files |= RFTYPE_MON;
>>> +
>>
>> Is this not redundant considering what just happened a few lines above?
> 
> Yea. Right. I will change the previous line to
> 
> files = RFTYPE_BASE;
> 

This is not clear to me. If I understand correctly this means that
when rtype == RDTMON_GROUP then files = RFTYPE_BASE?
This does not sound right to me. I think it would be awkward to to set
files = RFTYPE_BASE if rtype == RDTMON_GROUP and then later do another
test using rdt_mon_capable to set the correct flag. It should be possible
to integrate this better.

What is needed is:
When group is a control group:
	files = RFTYPE_BASE | RFTYPE_CTRL;
When group is a monitor group:
	files = RFTYPE_BASE | RFTYPE_MON;
When group is a monitor and control group then:
	files = RFTYPE_BASE | RFTYPE_CTRL | RFTYPE_MON;

How about just moving the "if (rdt_mon_capable)" check into the 
snippet that sets the flag for a control group?

Reinette

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

[Fenghua Yu]

Hi, Babu,

On 8/21/23 16:30, 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>
> Reviewed-by: Reinette Chatre <reinette.chatre@intel.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.

What happens to the already moved tasks when "abort"?

Could you please add add more details on "abort"?

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

It would be better to show the failed pid string in the failure report:
+			rdt_last_cmd_puts("Task list parsing error pid %s\n", pid_str);

So user will know which pid string causes the failure?

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

Thanks.

-Fenghua

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

[Fenghua Yu]

On 8/21/23 16:30, Babu Moger wrote:
> 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>

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

Thanks.

-Fenghua

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

[Fenghua Yu]

On 8/21/23 16:30, Babu Moger wrote:
> 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>

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

Thanks.

-Fenghua

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

[Fenghua Yu]

Hi, Babu,

On 8/21/23 16:30, 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 | 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..b09e7abd1299 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

L2 CDP and L3 CDP have different dirs. So need to add them here:

  + *		    Directories: L2, L3, MB, SMBA, L2CODE, L2DATA, L3CODE, L3DATA 
(L*CODE and L*DATA only available when CDP is enabled)

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

 > + *			    Directories: L2, L3

+ *			    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)

Thanks.

-Fenghua

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

[Fenghua Yu]

On 8/21/23 16:30, Babu Moger wrote:
> 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>

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

Thanks.

-Fenghua

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

[Fenghua Yu]

On 8/21/23 16:30, 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 to "CLOSID" and "RMID" respectively in
> an effort to keep the naming generic.
> 
> For example:
>   $cat /sys/fs/resctrl/ctrl_grp1/ctrl_hw_id
>   1
>   $cat /sys/fs/resctrl/mon_groups/mon_grp1/mon_hw_id
>   3
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>

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

Thanks.

-Fenghua

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

[Fenghua Yu]

Hi, Babu,

On 8/21/23 16:30, 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>
> ---
>   arch/x86/kernel/cpu/resctrl/internal.h |  2 +
>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 55 +++++++++++++++-----------
>   2 files changed, 33 insertions(+), 24 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index b09e7abd1299..44ad98f8c7af 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -611,5 +611,7 @@ 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);
> +void rdtgroup_destroy_root(void);

These two functions are called only in rdtgroup.c. Why are they exposed 
here?

>   
>   #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 80a4f76bc34b..98f9f880c30b 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -2516,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();
> @@ -2528,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;
> @@ -2584,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);
> @@ -2654,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;
> @@ -2824,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);
> @@ -3705,10 +3717,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)

Since rdtgroup_setup_root() is called only in this file, need to add 
"static".

>   {
> -	int ret;
> -
>   	rdt_root = kernfs_create_root(&rdtgroup_kf_syscall_ops,
>   				      KERNFS_ROOT_CREATE_DEACTIVATED |
>   				      KERNFS_ROOT_EXTRA_OPEN_PERM_CHECK,
> @@ -3716,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;
> +}
> +
> +void rdtgroup_destroy_root(void)

Ditto.

> +{
> +	kernfs_destroy_root(rdt_root);
> +	rdtgroup_default.kn = NULL;
> +}
> +
> +static void __init rdtgroup_setup_default(void)
> +{
>   	mutex_lock(&rdtgroup_mutex);
>   
>   	rdtgroup_default.closid = 0;
> @@ -3725,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)
> @@ -3859,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)
> @@ -3898,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;
>   }
> @@ -3909,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);
>   }

Thanks.

-Fenghua

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

[Fenghua Yu]

On 8/21/23 16:30, Babu Moger wrote:
> rdt_enable_ctx() enables the features provided during resctrl mount.
> 
> Additions to rdt_enable_ctx() are required to also modify error paths
> of rdt_enable_ctx() callers to ensure correct unwinding if errors
> are encountered after calling rdt_enable_ctx(). This is error prone.
> 
> Introduce rdt_disable_ctx() to refactor the error unwinding of
> rdt_enable_ctx() to simplify future additions. This also simplifies
> cleanup in rdt_kill_sb().
> 
> Remove cdp_disable_all() as it is not used anymore after the refactor.
> 
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Babu Moger <babu.moger@amd.com>

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

Thanks.

-Fenghua

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

[Reinette Chatre]

Hi Fenghua,

On 9/1/2023 4:21 PM, Fenghua Yu wrote:
> On 8/21/23 16:30, 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>
>> ---
>>   arch/x86/kernel/cpu/resctrl/internal.h |  2 +
>>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 55 +++++++++++++++-----------
>>   2 files changed, 33 insertions(+), 24 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index b09e7abd1299..44ad98f8c7af 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -611,5 +611,7 @@ 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);
>> +void rdtgroup_destroy_root(void);
> 
> These two functions are called only in rdtgroup.c. Why are they exposed here?
> 

Could you please take a look at the email thread [1] that
discusses this? We reached a compromise but would appreciate
if you have any guidance on the right solution.

Reinette

[1] https://lore.kernel.org/lkml/972db626-1d74-679d-72f2-3e122f95c314@intel.com/

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

[Fenghua Yu]

Hi, Reinette,

On 9/1/23 16:36, Reinette Chatre wrote:
> Hi Fenghua,
> 
> On 9/1/2023 4:21 PM, Fenghua Yu wrote:
>> On 8/21/23 16:30, 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>
>>> ---
>>>    arch/x86/kernel/cpu/resctrl/internal.h |  2 +
>>>    arch/x86/kernel/cpu/resctrl/rdtgroup.c | 55 +++++++++++++++-----------
>>>    2 files changed, 33 insertions(+), 24 deletions(-)
>>>
>>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>>> index b09e7abd1299..44ad98f8c7af 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>>> @@ -611,5 +611,7 @@ 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);
>>> +void rdtgroup_destroy_root(void);
>>
>> These two functions are called only in rdtgroup.c. Why are they exposed here?
>>
> 
> Could you please take a look at the email thread [1] that
> discusses this? We reached a compromise but would appreciate
> if you have any guidance on the right solution.

Yes, putting the static declarations earlier in rdtgroup.c is right AFAICT.

> 
> Reinette
> 
> [1] https://lore.kernel.org/lkml/972db626-1d74-679d-72f2-3e122f95c314@intel.com/
> 

Thanks.

-Fenghua

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

[Reinette Chatre]

On 9/1/2023 4:46 PM, Fenghua Yu wrote:
> 
> Yes, putting the static declarations earlier in rdtgroup.c is right AFAICT.
> 

Thank you very much Fenghua.

Reinette

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

[Moger, Babu]

Hi Reinette,

On 9/1/23 12:57, Reinette Chatre wrote:
> Hi Babu,
> 
> On 9/1/2023 10:28 AM, Moger, Babu wrote:
>> On 8/31/23 19:43, Reinette Chatre wrote:
>>> On 8/31/2023 4:58 PM, Moger, Babu wrote:
>>>> @@ -3336,6 +3340,9 @@ static int mkdir_rdt_prepare(struct kernfs_node
>>>> *parent_kn,
>>>>         else
>>>>                 files = RFTYPE_BASE | RFTYPE_MON;
>>>>
>>>> +       if (rdt_mon_capable)
>>>> +               files |= RFTYPE_MON;
>>>> +
>>>
>>> Is this not redundant considering what just happened a few lines above?
>>
>> Yea. Right. I will change the previous line to
>>
>> files = RFTYPE_BASE;
>>
> 
> This is not clear to me. If I understand correctly this means that
> when rtype == RDTMON_GROUP then files = RFTYPE_BASE?
> This does not sound right to me. I think it would be awkward to to set
> files = RFTYPE_BASE if rtype == RDTMON_GROUP and then later do another
> test using rdt_mon_capable to set the correct flag. It should be possible
> to integrate this better.
> 
> What is needed is:
> When group is a control group:
> 	files = RFTYPE_BASE | RFTYPE_CTRL;
> When group is a monitor group:
> 	files = RFTYPE_BASE | RFTYPE_MON;
> When group is a monitor and control group then:
> 	files = RFTYPE_BASE | RFTYPE_CTRL | RFTYPE_MON;
> 
> How about just moving the "if (rdt_mon_capable)" check into the 
> snippet that sets the flag for a control group?
> 
Sure. Will change it to.

   if (rtype == RDTCTRL_GROUP) {
                files = RFTYPE_BASE | RFTYPE_CTRL;
                if (rdt_mon_capable)
                        files |= RFTYPE_MON;
   } else {
                files = RFTYPE_BASE | RFTYPE_MON;
   }


thanks
Babu

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

[Moger, Babu]

Hi Fenghua,

On 9/1/2023 5:13 PM, Fenghua Yu wrote:
> Hi, Babu,
>
> On 8/21/23 16:30, 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>
>> Reviewed-by: Reinette Chatre <reinette.chatre@intel.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.
>
> What happens to the already moved tasks when "abort"?
>
> Could you please add add more details on "abort"?
>
> "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."
Sure.
>
>> +    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");
>
> It would be better to show the failed pid string in the failure report:
> +            rdt_last_cmd_puts("Task list parsing error pid %s\n", 
> pid_str);
>
> So user will know which pid string causes the failure?

It was already discussed. Printing the characters during parsing error 
may not be much useful.

Thanks

Babu

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

[Moger, Babu]

Hi Fenghua,

On 9/1/2023 5:31 PM, Fenghua Yu wrote:
> Hi, Babu,
>
> On 8/21/23 16:30, 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 | 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..b09e7abd1299 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
>
> L2 CDP and L3 CDP have different dirs. So need to add them here:
>
>  + *            Directories: L2, L3, MB, SMBA, L2CODE, L2DATA, L3CODE, 
> L3DATA (L*CODE and L*DATA only available when CDP is enabled)
>
Sure.


>> + *                   File: num_closids
>> + *
>> + *            --> RFTYPE_RES_CACHE (Files for cache control resources)
>> + *                Directories: L2, L3
>
> > + *                Directories: L2, L3
>
> + *                Directories: L2, L3, L2CODE, L2DATA, L3CODE, L3DATA
>
Yes. Sure.

Thank you

Babu

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

[Moger, Babu]

Hi Fenghua,

On 9/1/2023 6:21 PM, Fenghua Yu wrote:
> Hi, Babu,
>
> On 8/21/23 16:30, 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>
>> ---
>>   arch/x86/kernel/cpu/resctrl/internal.h |  2 +
>>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 55 +++++++++++++++-----------
>>   2 files changed, 33 insertions(+), 24 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h 
>> b/arch/x86/kernel/cpu/resctrl/internal.h
>> index b09e7abd1299..44ad98f8c7af 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -611,5 +611,7 @@ 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);
>> +void rdtgroup_destroy_root(void);
>
> These two functions are called only in rdtgroup.c. Why are they 
> exposed here?
Yes. Removed it now.
>
>>     #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 80a4f76bc34b..98f9f880c30b 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -2516,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();
>> @@ -2528,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;
>> @@ -2584,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);
>> @@ -2654,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;
>> @@ -2824,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);
>> @@ -3705,10 +3717,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)
>
> Since rdtgroup_setup_root() is called only in this file, need to add 
> "static".

Yes. Taken care of it now.


>
>>   {
>> -    int ret;
>> -
>>       rdt_root = kernfs_create_root(&rdtgroup_kf_syscall_ops,
>>                         KERNFS_ROOT_CREATE_DEACTIVATED |
>>                         KERNFS_ROOT_EXTRA_OPEN_PERM_CHECK,
>> @@ -3716,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;
>> +}
>> +
>> +void rdtgroup_destroy_root(void)
>
> Ditto.

Thanks

Babu

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

[Fenghua Yu]

Hi, Babu,

On 9/6/23 07:56, Moger, Babu wrote:
>>> @@ -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");
>>
>> It would be better to show the failed pid string in the failure report:
>> +            rdt_last_cmd_puts("Task list parsing error pid %s\n", 
>> pid_str);
>>
>> So user will know which pid string causes the failure?
> 
> It was already discussed. Printing the characters during parsing error 
> may not be much useful.

Could you please let me know where printing "pid_str" is discussed?

My understanding is a similar thing is discussed in v3:
https://lore.kernel.org/all/167778866506.1053859.2329229096484796501.stgit@bmoger-ubuntu/

Then v4 has this code without printing pid_str. In v4, there is a 
similar discussion of printing pid, but not pid_str.

But I cannot find a discussion of why "pid_str" is not printed.

If kstritoint(pid_str, 0, &pid) fails, without printing pid_str, how can 
user know which pid string fails? e.g. user tries to move 100 pids and 
the 51st pid parsing fails. It's hard for user to know the 51st pid 
fails without showing pid_str in the error info and then it's hard for 
the user to decide to re-do moving or aborting moving etc.

Thanks.

-Fenghua
then

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

[Moger, Babu]

Hi Fenghua,

On 9/6/2023 3:42 PM, Fenghua Yu wrote:
> Hi, Babu,
>
> On 9/6/23 07:56, Moger, Babu wrote:
>>>> @@ -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");
>>>
>>> It would be better to show the failed pid string in the failure report:
>>> +            rdt_last_cmd_puts("Task list parsing error pid %s\n", 
>>> pid_str);
>>>
>>> So user will know which pid string causes the failure?
>>
>> It was already discussed. Printing the characters during parsing 
>> error may not be much useful.
>
> Could you please let me know where printing "pid_str" is discussed?

My bad.  Should have read your comments more carefully.


>
> My understanding is a similar thing is discussed in v3:
> https://lore.kernel.org/all/167778866506.1053859.2329229096484796501.stgit@bmoger-ubuntu/ 
>
>
> Then v4 has this code without printing pid_str. In v4, there is a 
> similar discussion of printing pid, but not pid_str.
>
> But I cannot find a discussion of why "pid_str" is not printed.
>
> If kstritoint(pid_str, 0, &pid) fails, without printing pid_str, how 
> can user know which pid string fails? e.g. user tries to move 100 pids 
> and the 51st pid parsing fails. It's hard for user to know the 51st 
> pid fails without showing pid_str in the error info and then it's hard 
> for the user to decide to re-do moving or aborting moving etc.

That is correct.  Will add following print statement o print the pid_str.

rdt_last_cmd_printf("Task list parsing error pid %s\n", pid_str);

Thanks

Babu

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

[Fenghua Yu]

Hi, Babu,

On 9/7/23 07:44, Moger, Babu wrote:
> Hi Fenghua,
> 
> On 9/6/2023 3:42 PM, Fenghua Yu wrote:
>> Hi, Babu,
>>
>> On 9/6/23 07:56, Moger, Babu wrote:
>>>>> @@ -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");
>>>>
>>>> It would be better to show the failed pid string in the failure report:
>>>> +            rdt_last_cmd_puts("Task list parsing error pid %s\n", 
>>>> pid_str);
>>>>
>>>> So user will know which pid string causes the failure?
>>>
>>> It was already discussed. Printing the characters during parsing 
>>> error may not be much useful.
>>
>> Could you please let me know where printing "pid_str" is discussed?
> 
> My bad.  Should have read your comments more carefully.

Thank you for your clarification.

> 
> 
>>
>> My understanding is a similar thing is discussed in v3:
>> https://lore.kernel.org/all/167778866506.1053859.2329229096484796501.stgit@bmoger-ubuntu/ 
>>
>>
>> Then v4 has this code without printing pid_str. In v4, there is a 
>> similar discussion of printing pid, but not pid_str.
>>
>> But I cannot find a discussion of why "pid_str" is not printed.
>>
>> If kstritoint(pid_str, 0, &pid) fails, without printing pid_str, how 
>> can user know which pid string fails? e.g. user tries to move 100 pids 
>> and the 51st pid parsing fails. It's hard for user to know the 51st 
>> pid fails without showing pid_str in the error info and then it's hard 
>> for the user to decide to re-do moving or aborting moving etc.
> 
> That is correct.  Will add following print statement o print the pid_str.
> 
> rdt_last_cmd_printf("Task list parsing error pid %s\n", pid_str);

Great!

Thanks.

-Fenghua