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

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

---
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. 

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: Introduce "-o debug" mount option
      x86/resctrl: Display CLOSID and RMID for the resctrl groups
      x86/resctrl: Move default control group creation during mount
      x86/resctrl: Introduce RFTYPE_DEBUG flag


 Documentation/arch/x86/resctrl.rst     |  19 ++-
 arch/x86/kernel/cpu/resctrl/internal.h |  66 ++++++--
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 199 +++++++++++++++++--------
 3 files changed, 212 insertions(+), 72 deletions(-)

--

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

[Babu Moger]

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

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

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

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

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

diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
index 387ccbcb558f..290f01aa3002 100644
--- a/Documentation/arch/x86/resctrl.rst
+++ b/Documentation/arch/x86/resctrl.rst
@@ -292,7 +292,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 6ad33f355861..504137a5d31f 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);
@@ -708,16 +707,27 @@ static ssize_t rdtgroup_tasks_write(struct kernfs_open_file *of,
 	}
 	rdt_last_cmd_clear();
 
-	if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED ||
-	    rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
-		ret = -EINVAL;
-		rdt_last_cmd_puts("Pseudo-locking in progress\n");
-		goto unlock;
-	}
+	while (buf && buf[0] != '\0') {
+		pid_str = strim(strsep(&buf, ","));
 
-	ret = rdtgroup_move_task(pid, rdtgrp, of);
+		if (kstrtoint(pid_str, 0, &pid)) {
+			rdt_last_cmd_puts("Task list parsing error\n");
+			ret = -EINVAL;
+			break;
+		}
 
-unlock:
+		if (pid < 0) {
+			rdt_last_cmd_printf("Invalid pid %d value\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;
+		}
+	}
 	rdtgroup_kn_unlock(of->kn);
 
 	return ret ?: nbytes;

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

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 504137a5d31f..181612d2c84b 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -3220,7 +3220,11 @@ static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
 		goto out_destroy;
 	}
 
-	files = RFTYPE_BASE | BIT(RF_CTRLSHIFT + rtype);
+	if (rtype == RDTCTRL_GROUP)
+		files = RFTYPE_BASE | RFTYPE_CTRL;
+	else
+		files = RFTYPE_BASE | RFTYPE_MON;
+
 	ret = rdtgroup_add_files(kn, files);
 	if (ret) {
 		rdt_last_cmd_puts("kernfs fill error\n");

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

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 181612d2c84b..baed20b2d788 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1692,77 +1692,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
@@ -1781,7 +1781,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",
@@ -1828,7 +1828,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",
@@ -1836,14 +1836,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,
 	},
 
 };
@@ -1900,7 +1900,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)
@@ -1909,7 +1909,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;
 }
 
 /**
@@ -2044,21 +2044,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)
@@ -3559,7 +3559,7 @@ static int __init rdtgroup_setup_root(void)
 
 	list_add(&rdtgroup_default.rdtgroup_list, &rdt_all_groups);
 
-	ret = rdtgroup_add_files(kernfs_root_to_node(rdt_root), RF_CTRL_BASE);
+	ret = rdtgroup_add_files(kernfs_root_to_node(rdt_root), RFTYPE_CTRL_BASE);
 	if (ret) {
 		kernfs_destroy_root(rdt_root);
 		goto out;

[PATCH v5 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 |   45 ++++++++++++++++++++++++++++++++
 1 file changed, 45 insertions(+)

diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index 2051179a3b91..c20cd6acb7a3 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -240,6 +240,51 @@ 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 (directory and provides details on control
+ *     |         and monitoring resources)
+ *     |
+ *     --> base (Lists the files and information to interact with control
+ *               or monitor groups. Provides details on default control
+ *               group when filesystem is created. There is no directory
+ *               with name base)
+ *
+ *     info structure
+ *    -------------------------------------------------------------
+ *    --> RFTYPE_INFO
+ *        --> <info> directory
+ *            --> RFTYPE_TOP_INFO
+ *                Files: last_cmd_status
+ *
+ *        --> RFTYPE_MON_INFO
+ *            --> <L3_MON> directory
+ *                 Files: max_threshold_occupancy, mbm_local_bytes_config,
+ *                        mbm_total_bytes_config, mon_features, num_rmids
+ *
+ *        --> RFTYPE_CTRL_INFO
+ *            --> RFTYPE_RES_CACHE
+ *                --> <L2/L3> directory
+ *                     Files: bit_usage, cbm_mask, min_cbm_bits,
+ *                            num_closids, shareable_bits
+ *
+ *            --> RFTYPE_RES_MB
+ *                --> <MB/SMBA> directory
+ *                     Files: bandwidth_gran, delay_linear, min_bandwidth,
+ *                            num_closids
+ *
+ *     base structure
+ *     -----------------------------------------------------------
+ *     --> RFTYPE_BASE (Files common for both MON and CTRL groups)
+ *         Files: cpus, cpus_list, tasks
+ *
+ *     --> RFTYPE_CTRL_BASE (Files only for CTRL group)
+ *         Files: mode, schemata, size
  */
 #define RFTYPE_INFO			BIT(0)
 #define RFTYPE_BASE			BIT(1)

[PATCH v5 5/8] x86/resctrl: Introduce "-o debug" mount option

[Babu Moger]

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

For example, hardware uses CLOSID and RMIDs to control and monitor
resctrl resources. These numbers are not visible to the user. These
details can help to debug issues. Debug option provides a method to
add these files to resctrl.

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

diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
index 290f01aa3002..afdee4d1f691 100644
--- a/Documentation/arch/x86/resctrl.rst
+++ b/Documentation/arch/x86/resctrl.rst
@@ -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 c20cd6acb7a3..c07c6517d856 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)
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index baed20b2d788..be91dea5f927 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);
@@ -2368,6 +2370,9 @@ static int rdt_enable_ctx(struct rdt_fs_context *ctx)
 	if (!ret && ctx->enable_mba_mbps)
 		ret = set_mba_sc(true);
 
+	if (!ret && ctx->enable_debug)
+		resctrl_debug = true;
+
 	return ret;
 }
 
@@ -2556,6 +2561,7 @@ enum rdt_param {
 	Opt_cdp,
 	Opt_cdpl2,
 	Opt_mba_mbps,
+	Opt_debug,
 	nr__rdt_params
 };
 
@@ -2563,6 +2569,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),
 	{}
 };
 
@@ -2588,6 +2595,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;
@@ -2778,6 +2788,8 @@ static void rdt_kill_sb(struct super_block *sb)
 
 	set_mba_sc(false);
 
+	resctrl_debug = false;
+
 	/*Put everything back to default values. */
 	for_each_alloc_capable_rdt_resource(r)
 		reset_all_ctrls(r);
@@ -3530,6 +3542,9 @@ static int rdtgroup_show_options(struct seq_file *seq, struct kernfs_root *kf)
 	if (is_mba_sc(&rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl))
 		seq_puts(seq, ",mba_MBps");
 
+	if (resctrl_debug)
+		seq_puts(seq, ",debug");
+
 	return 0;
 }
 

[PATCH v5 6/8] x86/resctrl: Display CLOSID and RMID for the resctrl 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
to debug the issues.

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 mounted
with "-o debug" option.

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

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

diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
index afdee4d1f691..1baf857ad8c6 100644
--- a/Documentation/arch/x86/resctrl.rst
+++ b/Documentation/arch/x86/resctrl.rst
@@ -344,6 +344,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":
@@ -357,6 +361,10 @@ When monitoring is enabled all MON groups will also contain:
 	the sum for all tasks in the CTRL_MON group and all tasks in
 	MON groups. Please see example section for more details on usage.
 
+"mon_hw_id":
+	Available only with debug option. The identifier used by hardware
+	for the monitor group. On x86 this is the RMID.
+
 Resource allocation rules
 -------------------------
 
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index be91dea5f927..2f5cdc638607 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -763,6 +763,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
 
 /*
@@ -1824,6 +1856,12 @@ 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,
+	},
 	{
 		.name		= "schemata",
 		.mode		= 0644,
@@ -1847,6 +1885,12 @@ 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,
+	},
 
 };
 

[PATCH v5 7/8] x86/resctrl: Move default control group creation during mount

[Babu Moger]

Currently, the resctrl default control group is created during kernel
init time and rest of the files are added during mount. If the new
files are to be added to the default group during the mount then it
has to be done separately again.

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

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

diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 2f5cdc638607..e03cb01c4742 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -57,6 +57,7 @@ static char last_cmd_status_buf[512];
 struct dentry *debugfs_resctrl;
 
 static bool resctrl_debug;
+static int rdtgroup_setup_root(void);
 
 void rdt_last_cmd_clear(void)
 {
@@ -2515,13 +2516,6 @@ static int rdt_get_tree(struct fs_context *fc)
 
 	cpus_read_lock();
 	mutex_lock(&rdtgroup_mutex);
-	/*
-	 * resctrl file system can only be mounted once.
-	 */
-	if (static_branch_unlikely(&rdt_enable_key)) {
-		ret = -EBUSY;
-		goto out;
-	}
 
 	ret = rdt_enable_ctx(ctx);
 	if (ret < 0)
@@ -2535,9 +2529,15 @@ 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;
+		goto out_default;
 
 	if (rdt_mon_capable) {
 		ret = mongroup_create_dir(rdtgroup_default.kn,
@@ -2587,6 +2587,8 @@ static int rdt_get_tree(struct fs_context *fc)
 		kernfs_remove(kn_mongrp);
 out_info:
 	kernfs_remove(kn_info);
+out_default:
+	kernfs_remove(rdtgroup_default.kn);
 out_schemata_free:
 	schemata_list_destroy();
 out_mba:
@@ -2664,10 +2666,23 @@ static const struct fs_context_operations rdt_fs_context_ops = {
 static int rdt_init_fs_context(struct fs_context *fc)
 {
 	struct rdt_fs_context *ctx;
+	int ret;
+
+	/*
+	 * resctrl file system can only be mounted once.
+	 */
+	if (static_branch_unlikely(&rdt_enable_key))
+		return -EBUSY;
+
+	ret = rdtgroup_setup_root();
+	if (ret)
+		return ret;
 
 	ctx = kzalloc(sizeof(struct rdt_fs_context), GFP_KERNEL);
-	if (!ctx)
+	if (!ctx) {
+		kernfs_destroy_root(rdt_root);
 		return -ENOMEM;
+	}
 
 	ctx->kfc.root = rdt_root;
 	ctx->kfc.magic = RDTGROUP_SUPER_MAGIC;
@@ -2845,6 +2860,9 @@ static void rdt_kill_sb(struct super_block *sb)
 	static_branch_disable_cpuslocked(&rdt_alloc_enable_key);
 	static_branch_disable_cpuslocked(&rdt_mon_enable_key);
 	static_branch_disable_cpuslocked(&rdt_enable_key);
+	/* Remove the default group and cleanup the root */
+	list_del(&rdtgroup_default.rdtgroup_list);
+	kernfs_destroy_root(rdt_root);
 	kernfs_kill_sb(sb);
 	mutex_unlock(&rdtgroup_mutex);
 	cpus_read_unlock();
@@ -3598,10 +3616,8 @@ static struct kernfs_syscall_ops rdtgroup_kf_syscall_ops = {
 	.show_options	= rdtgroup_show_options,
 };
 
-static int __init rdtgroup_setup_root(void)
+static int rdtgroup_setup_root(void)
 {
-	int ret;
-
 	rdt_root = kernfs_create_root(&rdtgroup_kf_syscall_ops,
 				      KERNFS_ROOT_CREATE_DEACTIVATED |
 				      KERNFS_ROOT_EXTRA_OPEN_PERM_CHECK,
@@ -3618,19 +3634,11 @@ 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;
+	return 0;
 }
 
 static void domain_destroy_mon_state(struct rdt_domain *d)
@@ -3752,13 +3760,9 @@ int __init rdtgroup_init(void)
 	seq_buf_init(&last_cmd_status, last_cmd_status_buf,
 		     sizeof(last_cmd_status_buf));
 
-	ret = rdtgroup_setup_root();
-	if (ret)
-		return ret;
-
 	ret = sysfs_create_mount_point(fs_kobj, "resctrl");
 	if (ret)
-		goto cleanup_root;
+		return ret;
 
 	ret = register_filesystem(&rdt_fs_type);
 	if (ret)
@@ -3791,8 +3795,6 @@ int __init rdtgroup_init(void)
 
 cleanup_mountpoint:
 	sysfs_remove_mount_point(fs_kobj, "resctrl");
-cleanup_root:
-	kernfs_destroy_root(rdt_root);
 
 	return ret;
 }
@@ -3802,5 +3804,4 @@ void __exit rdtgroup_exit(void)
 	debugfs_remove_recursive(debugfs_resctrl);
 	unregister_filesystem(&rdt_fs_type);
 	sysfs_remove_mount_point(fs_kobj, "resctrl");
-	kernfs_destroy_root(rdt_root);
 }

[PATCH v5 8/8] x86/resctrl: Introduce RFTYPE_DEBUG flag

[Babu Moger]

Introduce RFTYPE_DEBUG flag which can be used to add files when
resctrl is mounted with "-o debug" option. RFTYPE_DEBUG is OR'd
with other RFTYPE_ flags. These other flags decide where in resctrl
structure these files should be created.

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

diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index c07c6517d856..5bc8d371fc3e 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -294,6 +294,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 e03cb01c4742..9e42ecbb9063 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1862,6 +1862,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,
 	},
 	{
 		.name		= "schemata",
@@ -1891,6 +1892,7 @@ static struct rftype res_common_files[] = {
 		.mode		= 0444,
 		.kf_ops		= &rdtgroup_kf_single_ops,
 		.seq_show	= rdtgroup_closid_show,
+		.fflags		= RFTYPE_CTRL_BASE | RFTYPE_DEBUG,
 	},
 
 };
@@ -1905,6 +1907,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);

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

[Moger, Babu]

Gentle ping, Any comments on this series.
Thanks
Babu

> -----Original Message-----
> From: Moger, Babu <Babu.Moger@amd.com>
> Sent: Thursday, June 1, 2023 2:01 PM
> To: corbet@lwn.net; reinette.chatre@intel.com; tglx@linutronix.de;
> mingo@redhat.com; bp@alien8.de
> Cc: fenghua.yu@intel.com; dave.hansen@linux.intel.com; x86@kernel.org;
> hpa@zytor.com; paulmck@kernel.org; akpm@linux-foundation.org;
> quic_neeraju@quicinc.com; rdunlap@infradead.org;
> damien.lemoal@opensource.wdc.com; songmuchun@bytedance.com;
> peterz@infradead.org; jpoimboe@kernel.org; pbonzini@redhat.com; Moger,
> Babu <Babu.Moger@amd.com>; chang.seok.bae@intel.com;
> pawan.kumar.gupta@linux.intel.com; jmattson@google.com;
> daniel.sneddon@linux.intel.com; Das1, Sandipan <Sandipan.Das@amd.com>;
> tony.luck@intel.com; james.morse@arm.com; linux-doc@vger.kernel.org; linux-
> kernel@vger.kernel.org; bagasdotme@gmail.com; eranian@google.com;
> christophe.leroy@csgroup.eu; pawan.kumar.gupta@linux.intel.com;
> jarkko@kernel.org; adrian.hunter@intel.com; quic_jiles@quicinc.com;
> peternewman@google.com; Moger, Babu <Babu.Moger@amd.com>
> Subject: [PATCH v5 0/8] x86/resctrl: Miscellaneous resctrl features
> 
> 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.
> 
> ---
> 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.
> 
> v4:
> https://lore.kernel.org/lkml/168177435378.1758847.8317743523931859131.stg
> it@bmoger-ubuntu/
> v3:
> https://lore.kernel.org/lkml/167778850105.1053859.14596357862185564029.st
> git@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: Introduce "-o debug" mount option
>       x86/resctrl: Display CLOSID and RMID for the resctrl groups
>       x86/resctrl: Move default control group creation during mount
>       x86/resctrl: Introduce RFTYPE_DEBUG flag
> 
> 
>  Documentation/arch/x86/resctrl.rst     |  19 ++-
>  arch/x86/kernel/cpu/resctrl/internal.h |  66 ++++++--
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 199 +++++++++++++++++--------
>  3 files changed, 212 insertions(+), 72 deletions(-)
> 
> --

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

[Shaopeng Tan (Fujitsu)]

Hi Babu,

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

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

Best regards,
Shaopeng TAN

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

[Reinette Chatre]

Hi Babu,

On 6/1/2023 12:00 PM, Babu Moger wrote:
> The resctrl task assignment for monitor or control group needs to be
> done one at a time. For example:
> 
>   $mount -t resctrl resctrl /sys/fs/resctrl/
>   $mkdir /sys/fs/resctrl/ctrl_grp1
>   $echo 123 > /sys/fs/resctrl/ctrl_grp1/tasks
>   $echo 456 > /sys/fs/resctrl/ctrl_grp1/tasks
>   $echo 789 > /sys/fs/resctrl/ctrl_grp1/tasks
> 
> This is not user-friendly when dealing with hundreds of tasks.
> 
> Support multiple task assignment in one command with tasks ids separated
> by commas. For example:
>   $echo 123,456,789 > /sys/fs/resctrl/ctrl_grp1/tasks
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 6ad33f355861..504137a5d31f 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);
> @@ -708,16 +707,27 @@ static ssize_t rdtgroup_tasks_write(struct kernfs_open_file *of,
>  	}
>  	rdt_last_cmd_clear();
>  
> -	if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED ||
> -	    rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
> -		ret = -EINVAL;
> -		rdt_last_cmd_puts("Pseudo-locking in progress\n");
> -		goto unlock;
> -	}

Please do not drop this snippet. I think there may have been misunderstanding
during previous comments - this snippet is required, it just does not need
to be run for every pid the user provides since it depends on the resource
group, not the pid.

> +	while (buf && buf[0] != '\0') {

I think it may help to add a check for '\n' here also. It looks to me
that a user (shell) that provides "pid,", which is "pid,\n" would encounter
error and this may not actually be an error.

> +		pid_str = strim(strsep(&buf, ","));
>  
> -	ret = rdtgroup_move_task(pid, rdtgrp, of);
> +		if (kstrtoint(pid_str, 0, &pid)) {
> +			rdt_last_cmd_puts("Task list parsing error\n");
> +			ret = -EINVAL;
> +			break;
> +		}
>  
> -unlock:
> +		if (pid < 0) {
> +			rdt_last_cmd_printf("Invalid pid %d value\n", pid);
> +			ret = -EINVAL;
> +			break;
> +		}

I'm trying to image a possible error and it does not look right. For example,
the above could be "Invalid pid 123 value". How about just "Invalid pid %d".

> +
> +		ret = rdtgroup_move_task(pid, rdtgrp, of);
> +		if (ret) {
> +			rdt_last_cmd_printf("Error while processing task %d\n", pid);
> +			break;
> +		}
> +	}
>  	rdtgroup_kn_unlock(of->kn);
>  
>  	return ret ?: nbytes;
> 
> 

Reinette

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

[Reinette Chatre]

Hi Babu,

On 6/1/2023 12:01 PM, 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.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---

Thank you.

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

Reinette

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

[Reinette Chatre]

Hi Babu,

On 6/1/2023 12:01 PM, 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.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---

Thank you.

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

Reinette

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

[Reinette Chatre]

Hi Babu,

On 6/1/2023 12:01 PM, Babu Moger wrote:
> resctrl uses RFTYPE flags for creating resctrl directory structure.
> 
> Definitions and directory structures are not documented. Add
> comments to improve the readability and help future additions.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  arch/x86/kernel/cpu/resctrl/internal.h |   45 ++++++++++++++++++++++++++++++++
>  1 file changed, 45 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 2051179a3b91..c20cd6acb7a3 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -240,6 +240,51 @@ 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 (directory and provides details on control
> + *     |         and monitoring resources)
> + *     |
> + *     --> base (Lists the files and information to interact with control
> + *               or monitor groups. Provides details on default control
> + *               group when filesystem is created. There is no directory
> + *               with name base)
> + *

In the above I think it may help to split the comment into two parts:
(a) which directory/directories are referred to, and (b) which files can be
found in the directory/directories.

For example,

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

Please feel free to improve.

> + *     info structure
> + *    -------------------------------------------------------------
> + *    --> RFTYPE_INFO
> + *        --> <info> directory
> + *            --> RFTYPE_TOP_INFO
> + *                Files: last_cmd_status
> + *
> + *        --> RFTYPE_MON_INFO
> + *            --> <L3_MON> directory
> + *                 Files: max_threshold_occupancy, mbm_local_bytes_config,
> + *                        mbm_total_bytes_config, mon_features, num_rmids
> + *
> + *        --> RFTYPE_CTRL_INFO
> + *            --> RFTYPE_RES_CACHE
> + *                --> <L2/L3> directory

Maybe a nitpick but I wonder if it should be "L2,L3" to not create impression
that it is either/or?

> + *                     Files: bit_usage, cbm_mask, min_cbm_bits,
> + *                            num_closids, shareable_bits
> + *
> + *            --> RFTYPE_RES_MB
> + *                --> <MB/SMBA> directory

Same here ... perhaps "MB,SMBA"

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

Missing thread_throttle_mode

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

I think this is a helpful addition. Thanks for doing this.

Reinette

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

[Reinette Chatre]

Hi Babu,

On 6/1/2023 12:01 PM, Babu Moger wrote:
> Add "-o debug" option to mount resctrl filesystem in debug mode.
> This option can be used for adding extra files to help debug
> resctrl issues.

Please avoid uncertainty in the changelog (re. "can be used"). I
think it will help to be more specific if the first and last
hunks of patch 8 is merged into this patch, making it clear
that the debug mount option is in support of debug files with
this changelog written to support that.

> For example, hardware uses CLOSID and RMIDs to control and monitor
> resctrl resources. These numbers are not visible to the user. These
> details can help to debug issues. Debug option provides a method to
> add these files to resctrl.

With the debug file support added here this extra motivation should
not be necessary (remaining hunks of patch 8 can be moved to where
these files are introduced).
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  Documentation/arch/x86/resctrl.rst     |    3 +++
>  arch/x86/kernel/cpu/resctrl/internal.h |    1 +
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |   15 +++++++++++++++
>  3 files changed, 19 insertions(+)
> 
> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
> index 290f01aa3002..afdee4d1f691 100644
> --- a/Documentation/arch/x86/resctrl.rst
> +++ b/Documentation/arch/x86/resctrl.rst
> @@ -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".
>  

At the risk of becoming unreadable, the earlier documentation of the mount
command should also change.

>  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 c20cd6acb7a3..c07c6517d856 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)
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index baed20b2d788..be91dea5f927 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);
> @@ -2368,6 +2370,9 @@ static int rdt_enable_ctx(struct rdt_fs_context *ctx)
>  	if (!ret && ctx->enable_mba_mbps)
>  		ret = set_mba_sc(true);
>  
> +	if (!ret && ctx->enable_debug)
> +		resctrl_debug = true;
> +
>  	return ret;
>  }

Looks like unwinding of rdt_enable_ctx() errors is done in the caller, this is
not ideal and additions like above cause some error unwinding to be omitted.
I cannot see why rdt_enable_ctx() cannot do its own error unwinding. This
may be better as a separate patch.

Reinette

Re: [PATCH v5 6/8] x86/resctrl: Display CLOSID and RMID for the resctrl groups

[Reinette Chatre]

Hi Babu,

On 6/1/2023 12:01 PM, Babu Moger wrote:
> In x86, hardware uses CLOSID and an RMID to identify a control group and
> a monitoring group respectively. When a user creates a control or monitor
> group these details are not visible to the user. These details can help
> to debug the issues.

"to debug the issues" -> "to debug issues"? Even so "issues" is vague,
perhaps just "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 mounted

"when mounted with" -> "when resctrl is mounted with"?

> with "-o debug" option.

Could you please add a snippet to explain the file naming choice? Just a
mention that this is done in support of other architectures that do not use
"CLOSID" and "RMID". 

Considering that, "x86/resctrl: Display hardware ids of resource groups"
may be a more appropriate subject.


Reinette

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

[Reinette Chatre]

Hi Babu,

On 6/1/2023 12:02 PM, Babu Moger wrote:
> Currently, the resctrl default control group is created during kernel
> init time and rest of the files are added during mount. If the new

Please drop the word "Currently"

> files are to be added to the default group during the mount then it
> has to be done separately again.
> 
> This can avoided if all the files are created during the mount and
> destroyed during the umount. Move the default group creation in

"creation in" -> "creation to"?

> rdt_get_tree and removal in rdt_kill_sb.

I think it would be simpler if this patch is moved earlier in series
then patch 8 can more easily be squashed where appropriate.

> 
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |   59 ++++++++++++++++----------------
>  1 file changed, 30 insertions(+), 29 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 2f5cdc638607..e03cb01c4742 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -57,6 +57,7 @@ static char last_cmd_status_buf[512];
>  struct dentry *debugfs_resctrl;
>  
>  static bool resctrl_debug;
> +static int rdtgroup_setup_root(void);
>  
>  void rdt_last_cmd_clear(void)
>  {
> @@ -2515,13 +2516,6 @@ static int rdt_get_tree(struct fs_context *fc)
>  
>  	cpus_read_lock();
>  	mutex_lock(&rdtgroup_mutex);
> -	/*
> -	 * resctrl file system can only be mounted once.
> -	 */
> -	if (static_branch_unlikely(&rdt_enable_key)) {
> -		ret = -EBUSY;
> -		goto out;
> -	}
>  

This change is unexpected.

>  	ret = rdt_enable_ctx(ctx);
>  	if (ret < 0)
> @@ -2535,9 +2529,15 @@ 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;
> +		goto out_default;
>  
>  	if (rdt_mon_capable) {
>  		ret = mongroup_create_dir(rdtgroup_default.kn,
> @@ -2587,6 +2587,8 @@ static int rdt_get_tree(struct fs_context *fc)
>  		kernfs_remove(kn_mongrp);
>  out_info:
>  	kernfs_remove(kn_info);
> +out_default:
> +	kernfs_remove(rdtgroup_default.kn);
>  out_schemata_free:
>  	schemata_list_destroy();
>  out_mba:
> @@ -2664,10 +2666,23 @@ static const struct fs_context_operations rdt_fs_context_ops = {
>  static int rdt_init_fs_context(struct fs_context *fc)
>  {
>  	struct rdt_fs_context *ctx;
> +	int ret;
> +
> +	/*
> +	 * resctrl file system can only be mounted once.
> +	 */
> +	if (static_branch_unlikely(&rdt_enable_key))
> +		return -EBUSY;
> +
> +	ret = rdtgroup_setup_root();
> +	if (ret)
> +		return ret;
>  

Why was it necessary to move this code?

>  	ctx = kzalloc(sizeof(struct rdt_fs_context), GFP_KERNEL);
> -	if (!ctx)
> +	if (!ctx) {
> +		kernfs_destroy_root(rdt_root);
>  		return -ENOMEM;
> +	}
>  
>  	ctx->kfc.root = rdt_root;
>  	ctx->kfc.magic = RDTGROUP_SUPER_MAGIC;
> @@ -2845,6 +2860,9 @@ static void rdt_kill_sb(struct super_block *sb)
>  	static_branch_disable_cpuslocked(&rdt_alloc_enable_key);
>  	static_branch_disable_cpuslocked(&rdt_mon_enable_key);
>  	static_branch_disable_cpuslocked(&rdt_enable_key);
> +	/* Remove the default group and cleanup the root */
> +	list_del(&rdtgroup_default.rdtgroup_list);
> +	kernfs_destroy_root(rdt_root);

Why not just add kernfs_remove(rdtgroup_default.kn) to rmdir_all_sub()?

>  	kernfs_kill_sb(sb);
>  	mutex_unlock(&rdtgroup_mutex);
>  	cpus_read_unlock();
> @@ -3598,10 +3616,8 @@ static struct kernfs_syscall_ops rdtgroup_kf_syscall_ops = {
>  	.show_options	= rdtgroup_show_options,
>  };
>  
> -static int __init rdtgroup_setup_root(void)
> +static int rdtgroup_setup_root(void)
>  {
> -	int ret;
> -
>  	rdt_root = kernfs_create_root(&rdtgroup_kf_syscall_ops,
>  				      KERNFS_ROOT_CREATE_DEACTIVATED |
>  				      KERNFS_ROOT_EXTRA_OPEN_PERM_CHECK,
> @@ -3618,19 +3634,11 @@ 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;
> +	return 0;
>  }
>  
>  static void domain_destroy_mon_state(struct rdt_domain *d)
> @@ -3752,13 +3760,9 @@ int __init rdtgroup_init(void)
>  	seq_buf_init(&last_cmd_status, last_cmd_status_buf,
>  		     sizeof(last_cmd_status_buf));
>  
> -	ret = rdtgroup_setup_root();
> -	if (ret)
> -		return ret;
> -
>  	ret = sysfs_create_mount_point(fs_kobj, "resctrl");
>  	if (ret)
> -		goto cleanup_root;
> +		return ret;
>  

It is not clear to me why this change is required, could you
please elaborate? It seems that all that is needed is for 
rdtgroup_add_files() to move to rdt_get_tree() (which you have done)
and then an additional call to kernfs_remove() in rmdir_all_sub().
I must be missing something, could you please help me understand?

>  	ret = register_filesystem(&rdt_fs_type);
>  	if (ret)
> @@ -3791,8 +3795,6 @@ int __init rdtgroup_init(void)
>  
>  cleanup_mountpoint:
>  	sysfs_remove_mount_point(fs_kobj, "resctrl");
> -cleanup_root:
> -	kernfs_destroy_root(rdt_root);
>  
>  	return ret;
>  }
> @@ -3802,5 +3804,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);
>  }
> 
> 

Reinette

Re: [PATCH v5 8/8] x86/resctrl: Introduce RFTYPE_DEBUG flag

[Reinette Chatre]

Hi Babu,

On 6/1/2023 12:02 PM, Babu Moger wrote:
> Introduce RFTYPE_DEBUG flag which can be used to add files when
> resctrl is mounted with "-o debug" option. RFTYPE_DEBUG is OR'd
> with other RFTYPE_ flags. These other flags decide where in resctrl
> structure these files should be created.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  arch/x86/kernel/cpu/resctrl/internal.h |    1 +
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |    5 +++++
>  2 files changed, 6 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index c07c6517d856..5bc8d371fc3e 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -294,6 +294,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 e03cb01c4742..9e42ecbb9063 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1862,6 +1862,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,
>  	},
>  	{
>  		.name		= "schemata",
> @@ -1891,6 +1892,7 @@ static struct rftype res_common_files[] = {
>  		.mode		= 0444,
>  		.kf_ops		= &rdtgroup_kf_single_ops,
>  		.seq_show	= rdtgroup_closid_show,
> +		.fflags		= RFTYPE_CTRL_BASE | RFTYPE_DEBUG,
>  	},
>  
>  };
> @@ -1905,6 +1907,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);
> 
> 

I think that the first and last hunk of this patch can be
squashed with patch 5. From what I can tell it would help
the motivation of patch 5 and fit nicely with what its
changelog aims to describe. The remaining hunks can be
moved to patch 6.

Reinette

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

[Moger, Babu]

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

Thanks Tan. I will add Reviewed and Tested by to the patches that are final.
-- 
Thanks
Babu Moger

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

[Moger, Babu]

Hi Reinette,

On 7/7/23 16:38, Reinette Chatre wrote:
> Hi Babu,
> 
> On 6/1/2023 12:00 PM, Babu Moger wrote:
>> The resctrl task assignment for monitor or control group needs to be
>> done one at a time. For example:
>>
>>   $mount -t resctrl resctrl /sys/fs/resctrl/
>>   $mkdir /sys/fs/resctrl/ctrl_grp1
>>   $echo 123 > /sys/fs/resctrl/ctrl_grp1/tasks
>>   $echo 456 > /sys/fs/resctrl/ctrl_grp1/tasks
>>   $echo 789 > /sys/fs/resctrl/ctrl_grp1/tasks
>>
>> This is not user-friendly when dealing with hundreds of tasks.
>>
>> Support multiple task assignment in one command with tasks ids separated
>> by commas. For example:
>>   $echo 123,456,789 > /sys/fs/resctrl/ctrl_grp1/tasks
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 6ad33f355861..504137a5d31f 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);
>> @@ -708,16 +707,27 @@ static ssize_t rdtgroup_tasks_write(struct kernfs_open_file *of,
>>  	}
>>  	rdt_last_cmd_clear();
>>  
>> -	if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED ||
>> -	    rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
>> -		ret = -EINVAL;
>> -		rdt_last_cmd_puts("Pseudo-locking in progress\n");
>> -		goto unlock;
>> -	}
> 
> Please do not drop this snippet. I think there may have been misunderstanding
> during previous comments - this snippet is required, it just does not need
> to be run for every pid the user provides since it depends on the resource
> group, not the pid.

Ok. Got it.

> 
>> +	while (buf && buf[0] != '\0') {
> 
> I think it may help to add a check for '\n' here also. It looks to me
> that a user (shell) that provides "pid,", which is "pid,\n" would encounter
> error and this may not actually be an error.

Ok Sounds good. I have verified it. New check will look like this below.

while (buf && buf[0] != '\0' && buf[0] != '\n') {

> 
>> +		pid_str = strim(strsep(&buf, ","));
>>  
>> -	ret = rdtgroup_move_task(pid, rdtgrp, of);
>> +		if (kstrtoint(pid_str, 0, &pid)) {
>> +			rdt_last_cmd_puts("Task list parsing error\n");
>> +			ret = -EINVAL;
>> +			break;
>> +		}
>>  
>> -unlock:
>> +		if (pid < 0) {
>> +			rdt_last_cmd_printf("Invalid pid %d value\n", pid);
>> +			ret = -EINVAL;
>> +			break;
>> +		}
> 
> I'm trying to image a possible error and it does not look right. For example,
> the above could be "Invalid pid 123 value". How about just "Invalid pid %d".

Sure.

Thanks
Babu

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

[Moger, Babu]

Hi Reinette,

On 7/7/23 16:39, Reinette Chatre wrote:
> Hi Babu,
> 
> On 6/1/2023 12:01 PM, Babu Moger wrote:
>> resctrl uses RFTYPE flags for creating resctrl directory structure.
>>
>> Definitions and directory structures are not documented. Add
>> comments to improve the readability and help future additions.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>  arch/x86/kernel/cpu/resctrl/internal.h |   45 ++++++++++++++++++++++++++++++++
>>  1 file changed, 45 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 2051179a3b91..c20cd6acb7a3 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -240,6 +240,51 @@ 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 (directory and provides details on control
>> + *     |         and monitoring resources)
>> + *     |
>> + *     --> base (Lists the files and information to interact with control
>> + *               or monitor groups. Provides details on default control
>> + *               group when filesystem is created. There is no directory
>> + *               with name base)
>> + *
> 
> In the above I think it may help to split the comment into two parts:
> (a) which directory/directories are referred to, and (b) which files can be
> found in the directory/directories.
> 
> For example,
> 
> --> 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.)
> 
> Please feel free to improve.

Looks good.

> 
>> + *     info structure
>> + *    -------------------------------------------------------------
>> + *    --> RFTYPE_INFO
>> + *        --> <info> directory
>> + *            --> RFTYPE_TOP_INFO
>> + *                Files: last_cmd_status
>> + *
>> + *        --> RFTYPE_MON_INFO
>> + *            --> <L3_MON> directory
>> + *                 Files: max_threshold_occupancy, mbm_local_bytes_config,
>> + *                        mbm_total_bytes_config, mon_features, num_rmids
>> + *
>> + *        --> RFTYPE_CTRL_INFO
>> + *            --> RFTYPE_RES_CACHE
>> + *                --> <L2/L3> directory
> 
> Maybe a nitpick but I wonder if it should be "L2,L3" to not create impression
> that it is either/or?

Sure.
> 
>> + *                     Files: bit_usage, cbm_mask, min_cbm_bits,
>> + *                            num_closids, shareable_bits
>> + *
>> + *            --> RFTYPE_RES_MB
>> + *                --> <MB/SMBA> directory
> 
> Same here ... perhaps "MB,SMBA"

Sure.
> 
>> + *                     Files: bandwidth_gran, delay_linear, min_bandwidth,
>> + *                            num_closids
> 
> Missing thread_throttle_mode

Will add it.
> 
>> + *
>> + *     base structure
>> + *     -----------------------------------------------------------
>> + *     --> RFTYPE_BASE (Files common for both MON and CTRL groups)
>> + *         Files: cpus, cpus_list, tasks
>> + *
>> + *     --> RFTYPE_CTRL_BASE (Files only for CTRL group)
>> + *         Files: mode, schemata, size
>>   */
>>  #define RFTYPE_INFO			BIT(0)
>>  #define RFTYPE_BASE			BIT(1)
>>
>>
> 
> I think this is a helpful addition. Thanks for doing this.

Sure. Welcome.Thanks
Babu Moger

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

[Moger, Babu]

Hi Reinette,

On 7/7/23 16:42, Reinette Chatre wrote:
> Hi Babu,
> 
> On 6/1/2023 12:01 PM, Babu Moger wrote:
>> Add "-o debug" option to mount resctrl filesystem in debug mode.
>> This option can be used for adding extra files to help debug
>> resctrl issues.
> 
> Please avoid uncertainty in the changelog (re. "can be used"). I

ok Sure.

> think it will help to be more specific if the first and last
> hunks of patch 8 is merged into this patch, making it clear
> that the debug mount option is in support of debug files with
> this changelog written to support that.

Sure. Will do that.

> 
>> For example, hardware uses CLOSID and RMIDs to control and monitor
>> resctrl resources. These numbers are not visible to the user. These
>> details can help to debug issues. Debug option provides a method to
>> add these files to resctrl.
> 
> With the debug file support added here this extra motivation should
> not be necessary (remaining hunks of patch 8 can be moved to where
> these files are introduced).

Sure.

>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>  Documentation/arch/x86/resctrl.rst     |    3 +++
>>  arch/x86/kernel/cpu/resctrl/internal.h |    1 +
>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |   15 +++++++++++++++
>>  3 files changed, 19 insertions(+)
>>
>> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
>> index 290f01aa3002..afdee4d1f691 100644
>> --- a/Documentation/arch/x86/resctrl.rst
>> +++ b/Documentation/arch/x86/resctrl.rst
>> @@ -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".
>>  
> 
> At the risk of becoming unreadable, the earlier documentation of the mount
> command should also change.

Ok. Sure

> 
>>  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 c20cd6acb7a3..c07c6517d856 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)
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index baed20b2d788..be91dea5f927 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);
>> @@ -2368,6 +2370,9 @@ static int rdt_enable_ctx(struct rdt_fs_context *ctx)
>>  	if (!ret && ctx->enable_mba_mbps)
>>  		ret = set_mba_sc(true);
>>  
>> +	if (!ret && ctx->enable_debug)
>> +		resctrl_debug = true;
>> +
>>  	return ret;
>>  }
> 
> Looks like unwinding of rdt_enable_ctx() errors is done in the caller, this is
> not ideal and additions like above cause some error unwinding to be omitted.
> I cannot see why rdt_enable_ctx() cannot do its own error unwinding. This
> may be better as a separate patch.
> 

Sure. Will do error unwinding of rdt_enable_ctx as a separate patch before
this patch. That seems more appropriate.
-- 
Thanks
Babu Moger

Re: [PATCH v5 6/8] x86/resctrl: Display CLOSID and RMID for the resctrl groups

[Moger, Babu]

Hi Reinette,

On 7/7/23 16:45, Reinette Chatre wrote:
> Hi Babu,
> 
> On 6/1/2023 12:01 PM, Babu Moger wrote:
>> In x86, hardware uses CLOSID and an RMID to identify a control group and
>> a monitoring group respectively. When a user creates a control or monitor
>> group these details are not visible to the user. These details can help
>> to debug the issues.
> 
> "to debug the issues" -> "to debug issues"? Even so "issues" is vague,
> perhaps just "These details can help debugging."

Sure.
> 
>>
>> 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 mounted
> 
> "when mounted with" -> "when resctrl is mounted with"?

Sure.

> 
>> with "-o debug" option.
> 
> Could you please add a snippet to explain the file naming choice? Just a
> mention that this is done in support of other architectures that do not use
> "CLOSID" and "RMID". 
Adding this.

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

> 
> Considering that, "x86/resctrl: Display hardware ids of resource groups"
> may be a more appropriate subject.
Sure.
-- 
Thanks
Babu Moger

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

[Moger, Babu]

Hi Reinette,
Sorry.. Took a while to respond. I had to recreate the issue to refresh my
memory.

On 7/7/23 16:46, Reinette Chatre wrote:
> Hi Babu,
> 
> On 6/1/2023 12:02 PM, Babu Moger wrote:
>> Currently, the resctrl default control group is created during kernel
>> init time and rest of the files are added during mount. If the new
> 
> Please drop the word "Currently"

Sure
> 
>> files are to be added to the default group during the mount then it
>> has to be done separately again.
>>
>> This can avoided if all the files are created during the mount and
>> destroyed during the umount. Move the default group creation in
> 
> "creation in" -> "creation to"?

Sure
> 
>> rdt_get_tree and removal in rdt_kill_sb.
> 
> I think it would be simpler if this patch is moved earlier in series
> then patch 8 can more easily be squashed where appropriate.

Yes, I was thinking about that.
> 
>>
>> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |   59 ++++++++++++++++----------------
>>  1 file changed, 30 insertions(+), 29 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 2f5cdc638607..e03cb01c4742 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -57,6 +57,7 @@ static char last_cmd_status_buf[512];
>>  struct dentry *debugfs_resctrl;
>>  
>>  static bool resctrl_debug;
>> +static int rdtgroup_setup_root(void);
>>  
>>  void rdt_last_cmd_clear(void)
>>  {
>> @@ -2515,13 +2516,6 @@ static int rdt_get_tree(struct fs_context *fc)
>>  
>>  	cpus_read_lock();
>>  	mutex_lock(&rdtgroup_mutex);
>> -	/*
>> -	 * resctrl file system can only be mounted once.
>> -	 */
>> -	if (static_branch_unlikely(&rdt_enable_key)) {
>> -		ret = -EBUSY;
>> -		goto out;
>> -	}
>>  
> 
> This change is unexpected.

Please see my comments below.

> 
>>  	ret = rdt_enable_ctx(ctx);
>>  	if (ret < 0)
>> @@ -2535,9 +2529,15 @@ 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;
>> +		goto out_default;
>>  
>>  	if (rdt_mon_capable) {
>>  		ret = mongroup_create_dir(rdtgroup_default.kn,
>> @@ -2587,6 +2587,8 @@ static int rdt_get_tree(struct fs_context *fc)
>>  		kernfs_remove(kn_mongrp);
>>  out_info:
>>  	kernfs_remove(kn_info);
>> +out_default:
>> +	kernfs_remove(rdtgroup_default.kn);
>>  out_schemata_free:
>>  	schemata_list_destroy();
>>  out_mba:
>> @@ -2664,10 +2666,23 @@ static const struct fs_context_operations rdt_fs_context_ops = {
>>  static int rdt_init_fs_context(struct fs_context *fc)
>>  {
>>  	struct rdt_fs_context *ctx;
>> +	int ret;
>> +
>> +	/*
>> +	 * resctrl file system can only be mounted once.
>> +	 */
>> +	if (static_branch_unlikely(&rdt_enable_key))
>> +		return -EBUSY;
>> +
>> +	ret = rdtgroup_setup_root();
>> +	if (ret)
>> +		return ret;
>>  
> 
> Why was it necessary to move this code?

Please see my comments below..
> 
>>  	ctx = kzalloc(sizeof(struct rdt_fs_context), GFP_KERNEL);
>> -	if (!ctx)
>> +	if (!ctx) {
>> +		kernfs_destroy_root(rdt_root);
>>  		return -ENOMEM;
>> +	}
>>  
>>  	ctx->kfc.root = rdt_root;
>>  	ctx->kfc.magic = RDTGROUP_SUPER_MAGIC;
>> @@ -2845,6 +2860,9 @@ static void rdt_kill_sb(struct super_block *sb)
>>  	static_branch_disable_cpuslocked(&rdt_alloc_enable_key);
>>  	static_branch_disable_cpuslocked(&rdt_mon_enable_key);
>>  	static_branch_disable_cpuslocked(&rdt_enable_key);
>> +	/* Remove the default group and cleanup the root */
>> +	list_del(&rdtgroup_default.rdtgroup_list);
>> +	kernfs_destroy_root(rdt_root);
> 
> Why not just add kernfs_remove(rdtgroup_default.kn) to rmdir_all_sub()?

List rdtgroup_default.rdtgroup_list is added during the mount and had to
be removed during umount and rdt_root is destroyed here.
Please see more comments below.

> 
>>  	kernfs_kill_sb(sb);
>>  	mutex_unlock(&rdtgroup_mutex);
>>  	cpus_read_unlock();
>> @@ -3598,10 +3616,8 @@ static struct kernfs_syscall_ops rdtgroup_kf_syscall_ops = {
>>  	.show_options	= rdtgroup_show_options,
>>  };
>>  
>> -static int __init rdtgroup_setup_root(void)
>> +static int rdtgroup_setup_root(void)
>>  {
>> -	int ret;
>> -
>>  	rdt_root = kernfs_create_root(&rdtgroup_kf_syscall_ops,
>>  				      KERNFS_ROOT_CREATE_DEACTIVATED |
>>  				      KERNFS_ROOT_EXTRA_OPEN_PERM_CHECK,
>> @@ -3618,19 +3634,11 @@ 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;
>> +	return 0;
>>  }
>>  
>>  static void domain_destroy_mon_state(struct rdt_domain *d)
>> @@ -3752,13 +3760,9 @@ int __init rdtgroup_init(void)
>>  	seq_buf_init(&last_cmd_status, last_cmd_status_buf,
>>  		     sizeof(last_cmd_status_buf));
>>  
>> -	ret = rdtgroup_setup_root();
>> -	if (ret)
>> -		return ret;
>> -
>>  	ret = sysfs_create_mount_point(fs_kobj, "resctrl");
>>  	if (ret)
>> -		goto cleanup_root;
>> +		return ret;
>>  
> 
> It is not clear to me why this change is required, could you
> please elaborate? It seems that all that is needed is for 
> rdtgroup_add_files() to move to rdt_get_tree() (which you have done)
> and then an additional call to kernfs_remove() in rmdir_all_sub().
> I must be missing something, could you please help me understand?
> 

Yes. I started with that approach. But there are issues with that approach.

Currently, rdt_root(which is rdtgroup_default.kn) is created during
rdtgroup_init. At the same time the root files are created. Also, default
group is added to rdt_all_groups. Basically, the root files and
rdtgroup_default group is always there even though filesystem is never
mounted. Also mbm_over and cqm_limbo workqueues are always running even
though filesystem is not mounted.

I changed rdtgroup_add_files() to move to rdt_get_tree() and added
kernfs_remove() in rmdir_all_sub(). This caused problems. The
kernfs_remove(rdtgroup_default.kn) removes all the reference counts and
releases the root. When we mount again, we hit this this problem below.

[  404.558461] ------------[ cut here ]------------
[  404.563631] WARNING: CPU: 35 PID: 7728 at fs/kernfs/dir.c:522
kernfs_new_node+0x63/0x70

404.778793]  ? __warn+0x81/0x140
[  404.782535]  ? kernfs_new_node+0x63/0x70
[  404.787036]  ? report_bug+0x102/0x200
[  404.791247]  ? handle_bug+0x3f/0x70
[  404.795269]  ? exc_invalid_op+0x13/0x60
[  404.799671]  ? asm_exc_invalid_op+0x16/0x20
[  404.804461]  ? kernfs_new_node+0x63/0x70
[  404.808954]  ? snprintf+0x49/0x70
[  404.812762]  __kernfs_create_file+0x30/0xc0
[  404.817534]  rdtgroup_add_files+0x6c/0x100

Basically kernel says your rdt_root is not initialized. That is the reason
I had to move everything to mount time. The rdt_root is created and
initialized during the mount and also destroyed during the umount.
And I had to move rdt_enable_key check during rdt_root creation.

-- 
Thanks
Babu Moger

Re: [PATCH v5 8/8] x86/resctrl: Introduce RFTYPE_DEBUG flag

[Moger, Babu]

Hi Reinette,

On 7/7/23 16:47, Reinette Chatre wrote:
> Hi Babu,
> 
> On 6/1/2023 12:02 PM, Babu Moger wrote:
>> Introduce RFTYPE_DEBUG flag which can be used to add files when
>> resctrl is mounted with "-o debug" option. RFTYPE_DEBUG is OR'd
>> with other RFTYPE_ flags. These other flags decide where in resctrl
>> structure these files should be created.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>  arch/x86/kernel/cpu/resctrl/internal.h |    1 +
>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |    5 +++++
>>  2 files changed, 6 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index c07c6517d856..5bc8d371fc3e 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -294,6 +294,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 e03cb01c4742..9e42ecbb9063 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -1862,6 +1862,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,
>>  	},
>>  	{
>>  		.name		= "schemata",
>> @@ -1891,6 +1892,7 @@ static struct rftype res_common_files[] = {
>>  		.mode		= 0444,
>>  		.kf_ops		= &rdtgroup_kf_single_ops,
>>  		.seq_show	= rdtgroup_closid_show,
>> +		.fflags		= RFTYPE_CTRL_BASE | RFTYPE_DEBUG,
>>  	},
>>  
>>  };
>> @@ -1905,6 +1907,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);
>>
>>
> 
> I think that the first and last hunk of this patch can be
> squashed with patch 5. From what I can tell it would help
> the motivation of patch 5 and fit nicely with what its
> changelog aims to describe. The remaining hunks can be
> moved to patch 6.
Sure.. Will do.
Thanks
Babu Moger

Re: [PATCH v5 6/8] x86/resctrl: Display CLOSID and RMID for the resctrl groups

[Reinette Chatre]

Hi Babu,

On 7/12/2023 12:36 PM, Moger, Babu wrote:
> On 7/7/23 16:45, Reinette Chatre wrote:
>> On 6/1/2023 12:01 PM, Babu Moger wrote:

>>
>> Could you please add a snippet to explain the file naming choice? Just a
>> mention that this is done in support of other architectures that do not use
>> "CLOSID" and "RMID". 
> Adding this.
> 
> Other architectures do not use "CLOSID" and "RMID". Kept the names
> ctrl_hw_id and mon_hw_id to refer "CLOSID" and "RMID" respectively in an
> effort to keep the naming generic.
> 

Looks good. I would just replace "Kept" with "Use" since this
change introduces these terms.

Reinette

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

[Reinette Chatre]

Hi Babu,

On 7/14/2023 9:26 AM, Moger, Babu wrote:
> Hi Reinette,
> Sorry.. Took a while to respond. I had to recreate the issue to refresh my
> memory.

No problem!

> On 7/7/23 16:46, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 6/1/2023 12:02 PM, Babu Moger wrote:


>>>  	ctx = kzalloc(sizeof(struct rdt_fs_context), GFP_KERNEL);
>>> -	if (!ctx)
>>> +	if (!ctx) {
>>> +		kernfs_destroy_root(rdt_root);
>>>  		return -ENOMEM;
>>> +	}
>>>  
>>>  	ctx->kfc.root = rdt_root;
>>>  	ctx->kfc.magic = RDTGROUP_SUPER_MAGIC;
>>> @@ -2845,6 +2860,9 @@ static void rdt_kill_sb(struct super_block *sb)
>>>  	static_branch_disable_cpuslocked(&rdt_alloc_enable_key);
>>>  	static_branch_disable_cpuslocked(&rdt_mon_enable_key);
>>>  	static_branch_disable_cpuslocked(&rdt_enable_key);
>>> +	/* Remove the default group and cleanup the root */
>>> +	list_del(&rdtgroup_default.rdtgroup_list);
>>> +	kernfs_destroy_root(rdt_root);
>>
>> Why not just add kernfs_remove(rdtgroup_default.kn) to rmdir_all_sub()?
> 
> List rdtgroup_default.rdtgroup_list is added during the mount and had to
> be removed during umount and rdt_root is destroyed here.

I do not think it is required for default resource group management to
be tied with the resctrl files associated with default resource group.

I think rdtgroup_setup_root can be split in two, one for all the
resctrl files that should be done at mount/unmount and one for the
default group init done at __init.

>>>  	kernfs_kill_sb(sb);
>>>  	mutex_unlock(&rdtgroup_mutex);
>>>  	cpus_read_unlock();
>>> @@ -3598,10 +3616,8 @@ static struct kernfs_syscall_ops rdtgroup_kf_syscall_ops = {
>>>  	.show_options	= rdtgroup_show_options,
>>>  };
>>>  
>>> -static int __init rdtgroup_setup_root(void)
>>> +static int rdtgroup_setup_root(void)
>>>  {
>>> -	int ret;
>>> -
>>>  	rdt_root = kernfs_create_root(&rdtgroup_kf_syscall_ops,
>>>  				      KERNFS_ROOT_CREATE_DEACTIVATED |
>>>  				      KERNFS_ROOT_EXTRA_OPEN_PERM_CHECK,
>>> @@ -3618,19 +3634,11 @@ 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;
>>> +	return 0;
>>>  }
>>>  
>>>  static void domain_destroy_mon_state(struct rdt_domain *d)
>>> @@ -3752,13 +3760,9 @@ int __init rdtgroup_init(void)
>>>  	seq_buf_init(&last_cmd_status, last_cmd_status_buf,
>>>  		     sizeof(last_cmd_status_buf));
>>>  
>>> -	ret = rdtgroup_setup_root();
>>> -	if (ret)
>>> -		return ret;
>>> -
>>>  	ret = sysfs_create_mount_point(fs_kobj, "resctrl");
>>>  	if (ret)
>>> -		goto cleanup_root;
>>> +		return ret;
>>>  
>>
>> It is not clear to me why this change is required, could you
>> please elaborate? It seems that all that is needed is for 
>> rdtgroup_add_files() to move to rdt_get_tree() (which you have done)
>> and then an additional call to kernfs_remove() in rmdir_all_sub().
>> I must be missing something, could you please help me understand?
>>
> 
> Yes. I started with that approach. But there are issues with that approach.
> 
> Currently, rdt_root(which is rdtgroup_default.kn) is created during
> rdtgroup_init. At the same time the root files are created. Also, default
> group is added to rdt_all_groups. Basically, the root files and
> rdtgroup_default group is always there even though filesystem is never
> mounted. Also mbm_over and cqm_limbo workqueues are always running even
> though filesystem is not mounted.
> 
> I changed rdtgroup_add_files() to move to rdt_get_tree() and added
> kernfs_remove() in rmdir_all_sub(). This caused problems. The
> kernfs_remove(rdtgroup_default.kn) removes all the reference counts and
> releases the root. When we mount again, we hit this this problem below.
> 
> [  404.558461] ------------[ cut here ]------------
> [  404.563631] WARNING: CPU: 35 PID: 7728 at fs/kernfs/dir.c:522
> kernfs_new_node+0x63/0x70
> 
> 404.778793]  ? __warn+0x81/0x140
> [  404.782535]  ? kernfs_new_node+0x63/0x70
> [  404.787036]  ? report_bug+0x102/0x200
> [  404.791247]  ? handle_bug+0x3f/0x70
> [  404.795269]  ? exc_invalid_op+0x13/0x60
> [  404.799671]  ? asm_exc_invalid_op+0x16/0x20
> [  404.804461]  ? kernfs_new_node+0x63/0x70
> [  404.808954]  ? snprintf+0x49/0x70
> [  404.812762]  __kernfs_create_file+0x30/0xc0
> [  404.817534]  rdtgroup_add_files+0x6c/0x100
> 
> Basically kernel says your rdt_root is not initialized. That is the reason
> I had to move everything to mount time. The rdt_root is created and
> initialized during the mount and also destroyed during the umount.
> And I had to move rdt_enable_key check during rdt_root creation.
> 

ok, thank you for the additional details. I see now how this patch evolved.
I understand how rdt_root needs to be created/destroyed
during mount/unmount. If I understand correctly the changes to
rdt_init_fs_context() was motivated by this line:

	ctx->kfc.root = rdt_root;

... that prompted you to move rdt_root creation there in order to have
it present for this assignment and that prompted the
rdt_enable_key check to follow. Is this correct?

I am concerned about the changes to rdt_init_fs_context() since it further
separates the resctrl file management, it breaks the symmetry of the
key checked and set, and finally these new actions seem unrelated to a function
named "init_fs_context". I looked at other examples and from what I can tell
it is not required that ctx->kfc.root be initialized within
rdt_init_fs_context(). Looks like the value is required by kernfs_get_tree()
that is called from rdt_get_tree(). For comparison I found cgroup_do_get_tree().
Note how cgroup_do_get_tree(), within the .get_tree callback,
initializes kernfs_fs_context.root and then call kernfs_get_tree()? 

It thus looks to me as though things can be simplified significantly
if the kernfs_fs_context.root assignment is moved from rdt_init_fs_context()
to rdt_get_tree(). rdt_get_tree() can then create rdt_root (and add all needed
files), assign it to kernfs_fs_context.root and call kernfs_get_tree().

What do you think?

Reinette

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

[Moger, Babu]

Hi Reinette,

On 7/14/23 16:54, Reinette Chatre wrote:
> Hi Babu,
> 
> On 7/14/2023 9:26 AM, Moger, Babu wrote:
>> Hi Reinette,
>> Sorry.. Took a while to respond. I had to recreate the issue to refresh my
>> memory.
> 
> No problem!
> 
>> On 7/7/23 16:46, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 6/1/2023 12:02 PM, Babu Moger wrote:
> 
> 
>>>>  	ctx = kzalloc(sizeof(struct rdt_fs_context), GFP_KERNEL);
>>>> -	if (!ctx)
>>>> +	if (!ctx) {
>>>> +		kernfs_destroy_root(rdt_root);
>>>>  		return -ENOMEM;
>>>> +	}
>>>>  
>>>>  	ctx->kfc.root = rdt_root;
>>>>  	ctx->kfc.magic = RDTGROUP_SUPER_MAGIC;
>>>> @@ -2845,6 +2860,9 @@ static void rdt_kill_sb(struct super_block *sb)
>>>>  	static_branch_disable_cpuslocked(&rdt_alloc_enable_key);
>>>>  	static_branch_disable_cpuslocked(&rdt_mon_enable_key);
>>>>  	static_branch_disable_cpuslocked(&rdt_enable_key);
>>>> +	/* Remove the default group and cleanup the root */
>>>> +	list_del(&rdtgroup_default.rdtgroup_list);
>>>> +	kernfs_destroy_root(rdt_root);
>>>
>>> Why not just add kernfs_remove(rdtgroup_default.kn) to rmdir_all_sub()?
>>
>> List rdtgroup_default.rdtgroup_list is added during the mount and had to
>> be removed during umount and rdt_root is destroyed here.
> 
> I do not think it is required for default resource group management to
> be tied with the resctrl files associated with default resource group.
> 
> I think rdtgroup_setup_root can be split in two, one for all the
> resctrl files that should be done at mount/unmount and one for the
> default group init done at __init.

Ok.
> 
>>>>  	kernfs_kill_sb(sb);
>>>>  	mutex_unlock(&rdtgroup_mutex);
>>>>  	cpus_read_unlock();
>>>> @@ -3598,10 +3616,8 @@ static struct kernfs_syscall_ops rdtgroup_kf_syscall_ops = {
>>>>  	.show_options	= rdtgroup_show_options,
>>>>  };
>>>>  
>>>> -static int __init rdtgroup_setup_root(void)
>>>> +static int rdtgroup_setup_root(void)
>>>>  {
>>>> -	int ret;
>>>> -
>>>>  	rdt_root = kernfs_create_root(&rdtgroup_kf_syscall_ops,
>>>>  				      KERNFS_ROOT_CREATE_DEACTIVATED |
>>>>  				      KERNFS_ROOT_EXTRA_OPEN_PERM_CHECK,
>>>> @@ -3618,19 +3634,11 @@ 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;
>>>> +	return 0;
>>>>  }
>>>>  
>>>>  static void domain_destroy_mon_state(struct rdt_domain *d)
>>>> @@ -3752,13 +3760,9 @@ int __init rdtgroup_init(void)
>>>>  	seq_buf_init(&last_cmd_status, last_cmd_status_buf,
>>>>  		     sizeof(last_cmd_status_buf));
>>>>  
>>>> -	ret = rdtgroup_setup_root();
>>>> -	if (ret)
>>>> -		return ret;
>>>> -
>>>>  	ret = sysfs_create_mount_point(fs_kobj, "resctrl");
>>>>  	if (ret)
>>>> -		goto cleanup_root;
>>>> +		return ret;
>>>>  
>>>
>>> It is not clear to me why this change is required, could you
>>> please elaborate? It seems that all that is needed is for 
>>> rdtgroup_add_files() to move to rdt_get_tree() (which you have done)
>>> and then an additional call to kernfs_remove() in rmdir_all_sub().
>>> I must be missing something, could you please help me understand?
>>>
>>
>> Yes. I started with that approach. But there are issues with that approach.
>>
>> Currently, rdt_root(which is rdtgroup_default.kn) is created during
>> rdtgroup_init. At the same time the root files are created. Also, default
>> group is added to rdt_all_groups. Basically, the root files and
>> rdtgroup_default group is always there even though filesystem is never
>> mounted. Also mbm_over and cqm_limbo workqueues are always running even
>> though filesystem is not mounted.
>>
>> I changed rdtgroup_add_files() to move to rdt_get_tree() and added
>> kernfs_remove() in rmdir_all_sub(). This caused problems. The
>> kernfs_remove(rdtgroup_default.kn) removes all the reference counts and
>> releases the root. When we mount again, we hit this this problem below.
>>
>> [  404.558461] ------------[ cut here ]------------
>> [  404.563631] WARNING: CPU: 35 PID: 7728 at fs/kernfs/dir.c:522
>> kernfs_new_node+0x63/0x70
>>
>> 404.778793]  ? __warn+0x81/0x140
>> [  404.782535]  ? kernfs_new_node+0x63/0x70
>> [  404.787036]  ? report_bug+0x102/0x200
>> [  404.791247]  ? handle_bug+0x3f/0x70
>> [  404.795269]  ? exc_invalid_op+0x13/0x60
>> [  404.799671]  ? asm_exc_invalid_op+0x16/0x20
>> [  404.804461]  ? kernfs_new_node+0x63/0x70
>> [  404.808954]  ? snprintf+0x49/0x70
>> [  404.812762]  __kernfs_create_file+0x30/0xc0
>> [  404.817534]  rdtgroup_add_files+0x6c/0x100
>>
>> Basically kernel says your rdt_root is not initialized. That is the reason
>> I had to move everything to mount time. The rdt_root is created and
>> initialized during the mount and also destroyed during the umount.
>> And I had to move rdt_enable_key check during rdt_root creation.
>>
> 
> ok, thank you for the additional details. I see now how this patch evolved.
> I understand how rdt_root needs to be created/destroyed
> during mount/unmount. If I understand correctly the changes to
> rdt_init_fs_context() was motivated by this line:
> 
> 	ctx->kfc.root = rdt_root;
> 
> ... that prompted you to move rdt_root creation there in order to have
> it present for this assignment and that prompted the
> rdt_enable_key check to follow. Is this correct?

That is correct.

> 
> I am concerned about the changes to rdt_init_fs_context() since it further
> separates the resctrl file management, it breaks the symmetry of the
> key checked and set, and finally these new actions seem unrelated to a function
> named "init_fs_context". I looked at other examples and from what I can tell
> it is not required that ctx->kfc.root be initialized within
> rdt_init_fs_context(). Looks like the value is required by kernfs_get_tree()
> that is called from rdt_get_tree(). For comparison I found cgroup_do_get_tree().
> Note how cgroup_do_get_tree(), within the .get_tree callback,
> initializes kernfs_fs_context.root and then call kernfs_get_tree()? 

Yes. I see that. Thanks for pointing.

> 
> It thus looks to me as though things can be simplified significantly
> if the kernfs_fs_context.root assignment is moved from rdt_init_fs_context()
> to rdt_get_tree(). rdt_get_tree() can then create rdt_root (and add all needed
> files), assign it to kernfs_fs_context.root and call kernfs_get_tree().
> 
> What do you think?

Yes. I think we can do that. Let me try it. Will let you know how it goes.
Thanks for the suggestion.
-- 
Thanks
Babu Moger

Re: [PATCH v5 6/8] x86/resctrl: Display CLOSID and RMID for the resctrl groups

[Moger, Babu]

On 7/14/23 16:53, Reinette Chatre wrote:
> Hi Babu,
> 
> On 7/12/2023 12:36 PM, Moger, Babu wrote:
>> On 7/7/23 16:45, Reinette Chatre wrote:
>>> On 6/1/2023 12:01 PM, Babu Moger wrote:
> 
>>>
>>> Could you please add a snippet to explain the file naming choice? Just a
>>> mention that this is done in support of other architectures that do not use
>>> "CLOSID" and "RMID". 
>> Adding this.
>>
>> Other architectures do not use "CLOSID" and "RMID". Kept the names
>> ctrl_hw_id and mon_hw_id to refer "CLOSID" and "RMID" respectively in an
>> effort to keep the naming generic.
>>
> 
> Looks good. I would just replace "Kept" with "Use" since this
> change introduces these terms.

Sure. Will do that.

-- 
Thanks
Babu Moger