[RFC PATCH 0/7] Add driver registration i/f to resctrl

[RFC PATCH 0/7] Add driver registration i/f to resctrl

[Tony Luck]

This is very much proof of concept code at this stage. I have a few
quality of service features that are hard to intergrate into the core
resctrl code because they are model specific, or have input parameters
that do not fit neatly into the existing schemata model.

Also, as AMD, ARM, and now RISC-V are looking to share the core resctrl
code, it might be helpful to have "driver" as a software layer for
per-CPU architectural code to avoid cluttering the core.

None of my drivers are ready to post, so this series has a simple example
driver that would meet the same debug requirements of Babu Moger's
patch to expose the CLOSID/RMID in files in each directory:

  https://lore.kernel.org/all/168177449635.1758847.13040588638888054027.stgit@bmoger-ubuntu/

Doing this debug with a driver that can be loaded unloaded without
having to unmount and remount the resctrl file system appears slightly
more convenient that a "-o debug" option. But this example driver is
really intended just as a toy example of what can be done.

The series is broken into steps that add callback functions into various
different parts of the resctrl hierarchy. That list of parts has been
driven by the needs of the drivers that I want to write. The
registration interface could be extended if there are additional
hooks need for other drivers.

I'm looking for high level comments on the desireability of this approach
at this time. I don't expect any of this to be merged until I have some
real drivers that use this to offer to upstream.

Tony Luck (7):
  x86/resctrl: Add register/unregister functions for driver to hook into
    resctrl
  x86/resctrl: Add an interface to add/remove a new info/directory
  x86/resctrl: Add driver callback when directories are removed
  x86/resctrl: Add capability to driver registration to create control
    files
  x86/resctrl: Enhance driver registration to hook into schemata files
  x86/resctrl: Allow a device to override an existing schemata entry
  x86/resctrl: Example resctrl driver

 include/linux/resctrl.h                       |  37 +++
 arch/x86/kernel/cpu/resctrl/ctrlmondata.c     |  17 +-
 .../cpu/resctrl/drivers/resctrl_example.c     |  77 ++++++
 arch/x86/kernel/cpu/resctrl/rdtgroup.c        | 227 ++++++++++++++++++
 arch/x86/Kconfig                              |  11 +
 arch/x86/kernel/cpu/resctrl/Makefile          |   1 +
 arch/x86/kernel/cpu/resctrl/drivers/Makefile  |   1 +
 7 files changed, 368 insertions(+), 3 deletions(-)
 create mode 100644 arch/x86/kernel/cpu/resctrl/drivers/resctrl_example.c
 create mode 100644 arch/x86/kernel/cpu/resctrl/drivers/Makefile


base-commit: 6a8f57ae2eb07ab39a6f0ccad60c760743051026
-- 
2.39.2

[RFC PATCH 1/7] x86/resctrl: Add register/unregister functions for driver to hook into resctrl

[Tony Luck]

Just one callback at the point for the driver to be notified when the
resctrl filesystem is mounted or unmounted. Virtually all drivers
will need this hook to enable/disable their feature(s) as part of
mount/unmount.

Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 include/linux/resctrl.h                | 13 +++++++
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 50 ++++++++++++++++++++++++++
 2 files changed, 63 insertions(+)

diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index 8334eeacfec5..78513edddca0 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -204,6 +204,19 @@ struct resctrl_schema {
 	u32				num_closid;
 };
 
+/**
+ * struct resctrl_driver - interface for driver to attach to resctrl
+ * @list:	List of registered drivers
+ * @mount:	Callback for mount/unmount
+ */
+struct resctrl_driver {
+	struct list_head	list;
+	void			(*mount)(bool mount);
+};
+
+int resctrl_register_driver(struct resctrl_driver *d);
+void resctrl_unregister_driver(struct resctrl_driver *d);
+
 /* The number of closid supported by this resource regardless of CDP */
 u32 resctrl_arch_get_num_closid(struct rdt_resource *r);
 int resctrl_arch_update_domains(struct rdt_resource *r, u32 closid);
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 6ad33f355861..3e6778bde427 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -51,6 +51,9 @@ static struct kernfs_node *kn_mongrp;
 /* Kernel fs node for "mon_data" directory under root */
 static struct kernfs_node *kn_mondata;
 
+static LIST_HEAD(drivers);
+static bool resctrl_is_mounted;
+
 static struct seq_buf last_cmd_status;
 static char last_cmd_status_buf[512];
 
@@ -2437,6 +2440,42 @@ static int schemata_list_create(void)
 	return ret;
 }
 
+static void driver_up(struct resctrl_driver *d)
+{
+	if (d->mount)
+		d->mount(true);
+}
+
+static void driver_down(struct resctrl_driver *d)
+{
+	if (d->mount)
+		d->mount(false);
+}
+
+int resctrl_register_driver(struct resctrl_driver *d)
+{
+	mutex_lock(&rdtgroup_mutex);
+	list_add(&d->list, &drivers);
+
+	if (resctrl_is_mounted)
+		driver_up(d);
+	mutex_unlock(&rdtgroup_mutex);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(resctrl_register_driver);
+
+void resctrl_unregister_driver(struct resctrl_driver *d)
+{
+	mutex_lock(&rdtgroup_mutex);
+	list_del(&d->list);
+
+	if (resctrl_is_mounted)
+		driver_down(d);
+	mutex_unlock(&rdtgroup_mutex);
+}
+EXPORT_SYMBOL_GPL(resctrl_unregister_driver);
+
 static void schemata_list_destroy(void)
 {
 	struct resctrl_schema *s, *tmp;
@@ -2450,6 +2489,7 @@ static void schemata_list_destroy(void)
 static int rdt_get_tree(struct fs_context *fc)
 {
 	struct rdt_fs_context *ctx = rdt_fc2context(fc);
+	struct resctrl_driver *d;
 	struct rdt_domain *dom;
 	struct rdt_resource *r;
 	int ret;
@@ -2516,6 +2556,10 @@ static int rdt_get_tree(struct fs_context *fc)
 			mbm_setup_overflow_handler(dom, MBM_OVERFLOW_INTERVAL);
 	}
 
+	list_for_each_entry(d, &drivers, list)
+		driver_up(d);
+	resctrl_is_mounted = true;
+
 	goto out;
 
 out_psl:
@@ -2761,6 +2805,7 @@ static void rmdir_all_sub(void)
 
 static void rdt_kill_sb(struct super_block *sb)
 {
+	struct resctrl_driver *d;
 	struct rdt_resource *r;
 
 	cpus_read_lock();
@@ -2780,6 +2825,11 @@ static void rdt_kill_sb(struct super_block *sb)
 	static_branch_disable_cpuslocked(&rdt_mon_enable_key);
 	static_branch_disable_cpuslocked(&rdt_enable_key);
 	kernfs_kill_sb(sb);
+
+	list_for_each_entry(d, &drivers, list)
+		driver_down(d);
+	resctrl_is_mounted = false;
+
 	mutex_unlock(&rdtgroup_mutex);
 	cpus_read_unlock();
 }
-- 
2.39.2

[RFC PATCH 2/7] x86/resctrl: Add an interface to add/remove a new info/directory

[Tony Luck]

Extend the resctrl_driver structure so that a driver may request
creation of a directory in the info/ hierarchy and populate it
with files.

Remove that info directory and contents when the resctrl file system is
unmounted, or the driver unregisters.

Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 include/linux/resctrl.h                | 10 +++++
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 59 ++++++++++++++++++++++++++
 2 files changed, 69 insertions(+)

diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index 78513edddca0..7847be48edae 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -204,14 +204,24 @@ struct resctrl_schema {
 	u32				num_closid;
 };
 
+struct resctrl_fileinfo {
+	char			*name;
+	struct kernfs_ops	*ops;
+	void			*priv;
+};
+
 /**
  * struct resctrl_driver - interface for driver to attach to resctrl
  * @list:	List of registered drivers
  * @mount:	Callback for mount/unmount
+ * @infodir:	Name of directory to create in resctrl/info.
+ * @infofiles:	Array of files to create under infodir.
  */
 struct resctrl_driver {
 	struct list_head	list;
 	void			(*mount)(bool mount);
+	char			*infodir;
+	struct resctrl_fileinfo	*infofiles;
 };
 
 int resctrl_register_driver(struct resctrl_driver *d);
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 3e6778bde427..4c662e827097 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2440,16 +2440,75 @@ static int schemata_list_create(void)
 	return ret;
 }
 
+#define F_ACTIVATE	BIT(0)
+#define F_PRIV		BIT(1)
+
+static int addfiles(struct kernfs_node *parent, struct resctrl_fileinfo *files,
+		    int flags, void *priv)
+{
+	struct resctrl_fileinfo *f;
+	struct kernfs_node *kn;
+	umode_t mode;
+	void *p;
+
+	for (f = files; f->name; f++) {
+		mode = (f->ops->write) ? 0644 : 0444;
+		p = (flags & F_PRIV) ? priv : f->priv;
+		kn = __kernfs_create_file(parent, f->name, mode,
+					  GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, 0,
+					  f->ops, p, NULL, NULL);
+		if (IS_ERR(kn))
+			return PTR_ERR(kn);
+		if (flags & F_ACTIVATE)
+			kernfs_activate(kn);
+	}
+
+	return 0;
+}
+
+/*
+ * Add or remove a directory from the resctrl/info directory
+ */
+static int rdtgroup_update_info_dir(char *name, struct resctrl_fileinfo *files)
+{
+	struct kernfs_node *kn_subdir;
+	int ret;
+
+	if (!files) {
+		kn_subdir = kernfs_find_and_get_ns(kn_info, name, NULL);
+		if (kn_subdir)
+			kernfs_remove(kn_subdir);
+		return 0;
+	}
+
+	kn_subdir = kernfs_create_dir(kn_info, name, 0755, NULL);
+	if (IS_ERR(kn_subdir))
+		return PTR_ERR(kn_subdir);
+
+	ret = addfiles(kn_subdir, files, 0, 0);
+
+	if (ret)
+		kernfs_remove(kn_subdir);
+	else
+		kernfs_activate(kn_subdir);
+
+	return ret;
+}
+
 static void driver_up(struct resctrl_driver *d)
 {
 	if (d->mount)
 		d->mount(true);
+	if (d->infodir)
+		rdtgroup_update_info_dir(d->infodir, d->infofiles);
 }
 
 static void driver_down(struct resctrl_driver *d)
 {
 	if (d->mount)
 		d->mount(false);
+	if (d->infodir)
+		rdtgroup_update_info_dir(d->infodir, NULL);
 }
 
 int resctrl_register_driver(struct resctrl_driver *d)
-- 
2.39.2

[RFC PATCH 3/7] x86/resctrl: Add driver callback when directories are removed

[Tony Luck]

When a resctrl directory is removed, entities attached to that
directory are reassigned with the CLOSID and RMID of the parent
directory.

Add a callback function so a driver can reset the CLOSID and RMID
of any resources attached to the removed directory.

Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 include/linux/resctrl.h                |  2 ++
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 13 +++++++++++++
 2 files changed, 15 insertions(+)

diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index 7847be48edae..44dd811cb552 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -216,12 +216,14 @@ struct resctrl_fileinfo {
  * @mount:	Callback for mount/unmount
  * @infodir:	Name of directory to create in resctrl/info.
  * @infofiles:	Array of files to create under infodir.
+ * @rmdir:	Callback when a resctrl directory is removed.
  */
 struct resctrl_driver {
 	struct list_head	list;
 	void			(*mount)(bool mount);
 	char			*infodir;
 	struct resctrl_fileinfo	*infofiles;
+	int			(*rmdir)(int oclos, int ormid, int nclos, int nrmid);
 };
 
 int resctrl_register_driver(struct resctrl_driver *d);
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 4c662e827097..8ca3b17bd671 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -3495,11 +3495,18 @@ static int rdtgroup_mkdir(struct kernfs_node *parent_kn, const char *name,
 static int rdtgroup_rmdir_mon(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
 {
 	struct rdtgroup *prdtgrp = rdtgrp->mon.parent;
+	struct resctrl_driver *d;
 	int cpu;
 
 	/* Give any tasks back to the parent group */
 	rdt_move_group_tasks(rdtgrp, prdtgrp, tmpmask);
 
+	list_for_each_entry(d, &drivers, list) {
+		if (d->rmdir)
+			d->rmdir(rdtgrp->closid, rdtgrp->mon.rmid,
+				 prdtgrp->closid, prdtgrp->mon.rmid);
+	}
+
 	/* Update per cpu rmid of the moved CPUs first */
 	for_each_cpu(cpu, &rdtgrp->cpu_mask)
 		per_cpu(pqr_state.default_rmid, cpu) = prdtgrp->mon.rmid;
@@ -3535,6 +3542,7 @@ static int rdtgroup_ctrl_remove(struct rdtgroup *rdtgrp)
 
 static int rdtgroup_rmdir_ctrl(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
 {
+	struct resctrl_driver *d;
 	int cpu;
 
 	/* Give any tasks back to the default group */
@@ -3544,6 +3552,11 @@ static int rdtgroup_rmdir_ctrl(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
 	cpumask_or(&rdtgroup_default.cpu_mask,
 		   &rdtgroup_default.cpu_mask, &rdtgrp->cpu_mask);
 
+	list_for_each_entry(d, &drivers, list) {
+		if (d->rmdir)
+			d->rmdir(rdtgrp->closid, rdtgrp->mon.rmid, 0, 0);
+	}
+
 	/* Update per cpu closid and rmid of the moved CPUs first */
 	for_each_cpu(cpu, &rdtgrp->cpu_mask) {
 		per_cpu(pqr_state.default_closid, cpu) = rdtgroup_default.closid;
-- 
2.39.2

[RFC PATCH 4/7] x86/resctrl: Add capability to driver registration to create control files

[Tony Luck]

Add a control file to each ctrlmon directory. Hook into the mkdir()
functions to add the control file to any new directories that are
created.

Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 include/linux/resctrl.h                |  2 +
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 70 ++++++++++++++++++++++++++
 2 files changed, 72 insertions(+)

diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index 44dd811cb552..8668480cea51 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -217,6 +217,7 @@ struct resctrl_fileinfo {
  * @infodir:	Name of directory to create in resctrl/info.
  * @infofiles:	Array of files to create under infodir.
  * @rmdir:	Callback when a resctrl directory is removed.
+ * @ctrlfiles:	Array of files to create in ctrlmon directories.
  */
 struct resctrl_driver {
 	struct list_head	list;
@@ -224,6 +225,7 @@ struct resctrl_driver {
 	char			*infodir;
 	struct resctrl_fileinfo	*infofiles;
 	int			(*rmdir)(int oclos, int ormid, int nclos, int nrmid);
+	struct resctrl_fileinfo	*ctrlfiles;
 };
 
 int resctrl_register_driver(struct resctrl_driver *d);
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 8ca3b17bd671..e2fdd5819336 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2466,6 +2466,18 @@ static int addfiles(struct kernfs_node *parent, struct resctrl_fileinfo *files,
 	return 0;
 }
 
+static void rmfiles(struct kernfs_node *parent, struct resctrl_fileinfo *files)
+{
+	struct resctrl_fileinfo *f;
+	struct kernfs_node *kn;
+
+	for (f = files; f->name; f++) {
+		kn = kernfs_find_and_get_ns(parent, f->name, NULL);
+		if (kn)
+			kernfs_remove(kn);
+	}
+}
+
 /*
  * Add or remove a directory from the resctrl/info directory
  */
@@ -2495,12 +2507,49 @@ static int rdtgroup_update_info_dir(char *name, struct resctrl_fileinfo *files)
 	return ret;
 }
 
+/*
+ * Create/remove a new control file in all current and future control and
+ * monitor groups.
+ */
+static void rdtgroup_update_ctrl_dir(struct resctrl_fileinfo *files, bool add)
+{
+	struct rdtgroup *rdtgrp;
+
+	if (!add) {
+		list_for_each_entry(rdtgrp, &rdt_all_groups, rdtgroup_list) {
+			struct rdtgroup *crg;
+
+			rmfiles(rdtgrp->kn, files);
+
+			list_for_each_entry(crg, &rdtgrp->mon.crdtgrp_list, mon.crdtgrp_list) {
+				rmfiles(crg->kn, files);
+			}
+		}
+
+		return;
+	}
+
+	list_for_each_entry(rdtgrp, &rdt_all_groups, rdtgroup_list) {
+		unsigned long priv = (rdtgrp->closid << 16) | rdtgrp->mon.rmid;
+		struct rdtgroup *crg;
+
+		addfiles(rdtgrp->kn, files, F_PRIV | F_ACTIVATE, (void *)priv);
+
+		list_for_each_entry(crg, &rdtgrp->mon.crdtgrp_list, mon.crdtgrp_list) {
+			priv = (crg->closid << 16) | crg->mon.rmid;
+			addfiles(crg->kn, files, F_PRIV | F_ACTIVATE, (void *)priv);
+		}
+	}
+}
+
 static void driver_up(struct resctrl_driver *d)
 {
 	if (d->mount)
 		d->mount(true);
 	if (d->infodir)
 		rdtgroup_update_info_dir(d->infodir, d->infofiles);
+	if (d->ctrlfiles)
+		rdtgroup_update_ctrl_dir(d->ctrlfiles, true);
 }
 
 static void driver_down(struct resctrl_driver *d)
@@ -2509,6 +2558,8 @@ static void driver_down(struct resctrl_driver *d)
 		d->mount(false);
 	if (d->infodir)
 		rdtgroup_update_info_dir(d->infodir, NULL);
+	if (d->ctrlfiles)
+		rdtgroup_update_ctrl_dir(d->ctrlfiles, false);
 }
 
 int resctrl_register_driver(struct resctrl_driver *d)
@@ -3375,6 +3426,7 @@ static int rdtgroup_mkdir_mon(struct kernfs_node *parent_kn,
 			      const char *name, umode_t mode)
 {
 	struct rdtgroup *rdtgrp, *prgrp;
+	struct resctrl_driver *d;
 	int ret;
 
 	ret = mkdir_rdt_prepare(parent_kn, name, mode, RDTMON_GROUP, &rdtgrp);
@@ -3384,6 +3436,14 @@ static int rdtgroup_mkdir_mon(struct kernfs_node *parent_kn,
 	prgrp = rdtgrp->mon.parent;
 	rdtgrp->closid = prgrp->closid;
 
+	list_for_each_entry(d, &drivers, list) {
+		unsigned long priv = (rdtgrp->closid << 16) | rdtgrp->mon.rmid;
+
+		if (!d->ctrlfiles)
+			continue;
+		addfiles(rdtgrp->kn, d->ctrlfiles, F_PRIV | F_ACTIVATE, (void *)priv);
+	}
+
 	/*
 	 * Add the rdtgrp to the list of rdtgrps the parent
 	 * ctrl_mon group has to track.
@@ -3401,6 +3461,7 @@ static int rdtgroup_mkdir_mon(struct kernfs_node *parent_kn,
 static int rdtgroup_mkdir_ctrl_mon(struct kernfs_node *parent_kn,
 				   const char *name, umode_t mode)
 {
+	struct resctrl_driver *d;
 	struct rdtgroup *rdtgrp;
 	struct kernfs_node *kn;
 	u32 closid;
@@ -3426,6 +3487,15 @@ static int rdtgroup_mkdir_ctrl_mon(struct kernfs_node *parent_kn,
 
 	list_add(&rdtgrp->rdtgroup_list, &rdt_all_groups);
 
+	list_for_each_entry(d, &drivers, list) {
+		unsigned long priv = (rdtgrp->closid << 16) | rdtgrp->mon.rmid;
+
+		if (!d->ctrlfiles)
+			continue;
+
+		addfiles(kn, d->ctrlfiles, F_PRIV | F_ACTIVATE, (void *)priv);
+	}
+
 	if (rdt_mon_capable) {
 		/*
 		 * Create an empty mon_groups directory to hold the subset
-- 
2.39.2

[RFC PATCH 5/7] x86/resctrl: Enhance driver registration to hook into schemata files

[Tony Luck]

Add a new configuration type "DRIVER" for lines in the schemata files
with a show() and update() callback functions to the driver to maintain
these lines.

Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 include/linux/resctrl.h                   |  6 ++++++
 arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 17 ++++++++++++++---
 arch/x86/kernel/cpu/resctrl/rdtgroup.c    | 16 ++++++++++++++++
 3 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index 8668480cea51..691805214f41 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -23,11 +23,13 @@ int proc_resctrl_show(struct seq_file *m,
  * @CDP_NONE:	No prioritisation, both code and data are controlled or monitored.
  * @CDP_CODE:	Configuration applies to instruction fetches.
  * @CDP_DATA:	Configuration applies to reads and writes.
+ * @DRIVER:	Resource managed by driver.
  */
 enum resctrl_conf_type {
 	CDP_NONE,
 	CDP_CODE,
 	CDP_DATA,
+	DRIVER,
 };
 
 #define CDP_NUM_TYPES	(CDP_DATA + 1)
@@ -201,6 +203,8 @@ struct resctrl_schema {
 	char				name[8];
 	enum resctrl_conf_type		conf_type;
 	struct rdt_resource		*res;
+	void				(*show)(struct seq_file *s, int closid);
+	int				(*update)(char *s, int closid);
 	u32				num_closid;
 };
 
@@ -218,6 +222,7 @@ struct resctrl_fileinfo {
  * @infofiles:	Array of files to create under infodir.
  * @rmdir:	Callback when a resctrl directory is removed.
  * @ctrlfiles:	Array of files to create in ctrlmon directories.
+ * @schema:	Driver supplied to manage a line in schemata file.
  */
 struct resctrl_driver {
 	struct list_head	list;
@@ -226,6 +231,7 @@ struct resctrl_driver {
 	struct resctrl_fileinfo	*infofiles;
 	int			(*rmdir)(int oclos, int ormid, int nclos, int nrmid);
 	struct resctrl_fileinfo	*ctrlfiles;
+	struct resctrl_schema	schema;
 };
 
 int resctrl_register_driver(struct resctrl_driver *d);
diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index b44c487727d4..7a59d6eab576 100644
--- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
@@ -356,8 +356,12 @@ static int rdtgroup_parse_resource(char *resname, char *tok,
 	struct resctrl_schema *s;
 
 	list_for_each_entry(s, &resctrl_schema_all, list) {
-		if (!strcmp(resname, s->name) && rdtgrp->closid < s->num_closid)
-			return parse_line(tok, s, rdtgrp);
+		if (!strcmp(resname, s->name) && rdtgrp->closid < s->num_closid) {
+			if (s->conf_type == DRIVER)
+				return s->update(tok, rdtgrp->closid);
+			else
+				return parse_line(tok, s, rdtgrp);
+		}
 	}
 	rdt_last_cmd_printf("Unknown or unsupported resource name '%s'\n", resname);
 	return -EINVAL;
@@ -419,10 +423,11 @@ ssize_t rdtgroup_schemata_write(struct kernfs_open_file *of,
 		r = s->res;
 
 		/*
+		 * Resources controlled by a driver are updated by that driver.
 		 * Writes to mba_sc resources update the software controller,
 		 * not the control MSR.
 		 */
-		if (is_mba_sc(r))
+		if (s->conf_type == DRIVER || is_mba_sc(r))
 			continue;
 
 		ret = resctrl_arch_update_domains(r, rdtgrp->closid);
@@ -464,6 +469,12 @@ static void show_doms(struct seq_file *s, struct resctrl_schema *schema, int clo
 	u32 ctrl_val;
 
 	seq_printf(s, "%*s:", max_name_width, schema->name);
+
+	if (schema->conf_type == DRIVER) {
+		schema->show(s, closid);
+		return;
+	}
+
 	list_for_each_entry(dom, &r->domains, list) {
 		if (sep)
 			seq_puts(s, ";");
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index e2fdd5819336..cc2292a7435b 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2043,6 +2043,8 @@ static int rdtgroup_create_info_dir(struct kernfs_node *parent_kn)
 
 	/* loop over enabled controls, these are all alloc_capable */
 	list_for_each_entry(s, &resctrl_schema_all, list) {
+		if (s->conf_type == DRIVER)
+			continue;
 		r = s->res;
 		fflags =  r->fflags | RF_CTRL_INFO;
 		ret = rdtgroup_mkdir_info_resdir(s, s->name, fflags);
@@ -2390,6 +2392,9 @@ static int schemata_list_add(struct rdt_resource *r, enum resctrl_conf_type type
 	case CDP_NONE:
 		suffix = "";
 		break;
+	case DRIVER:
+		kfree(s);
+		return -EINVAL;
 	}
 
 	ret = snprintf(s->name, sizeof(s->name), "%s%s", r->name, suffix);
@@ -2567,6 +2572,9 @@ int resctrl_register_driver(struct resctrl_driver *d)
 	mutex_lock(&rdtgroup_mutex);
 	list_add(&d->list, &drivers);
 
+	if (d->schema.name[0])
+		list_add(&d->schema.list, &resctrl_schema_all);
+
 	if (resctrl_is_mounted)
 		driver_up(d);
 	mutex_unlock(&rdtgroup_mutex);
@@ -2578,8 +2586,12 @@ EXPORT_SYMBOL_GPL(resctrl_register_driver);
 void resctrl_unregister_driver(struct resctrl_driver *d)
 {
 	mutex_lock(&rdtgroup_mutex);
+
 	list_del(&d->list);
 
+	if (d->schema.name[0])
+		list_del(&d->schema.list);
+
 	if (resctrl_is_mounted)
 		driver_down(d);
 	mutex_unlock(&rdtgroup_mutex);
@@ -2591,6 +2603,8 @@ static void schemata_list_destroy(void)
 	struct resctrl_schema *s, *tmp;
 
 	list_for_each_entry_safe(s, tmp, &resctrl_schema_all, list) {
+		if (s->conf_type == DRIVER)
+			continue;
 		list_del(&s->list);
 		kfree(s);
 	}
@@ -3285,6 +3299,8 @@ static int rdtgroup_init_alloc(struct rdtgroup *rdtgrp)
 	rdt_staged_configs_clear();
 
 	list_for_each_entry(s, &resctrl_schema_all, list) {
+		if (s->conf_type == DRIVER)
+			continue;
 		r = s->res;
 		if (r->rid == RDT_RESOURCE_MBA ||
 		    r->rid == RDT_RESOURCE_SMBA) {
-- 
2.39.2

[RFC PATCH 6/7] x86/resctrl: Allow a device to override an existing schemata entry

[Tony Luck]

Remove that entry from the resctrl_schema_all list when driver
is loaded. Put it back again when driver is unloaded.

Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 include/linux/resctrl.h                |  4 ++++
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 19 +++++++++++++++++++
 2 files changed, 23 insertions(+)

diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index 691805214f41..246644f53bde 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -223,6 +223,8 @@ struct resctrl_fileinfo {
  * @rmdir:	Callback when a resctrl directory is removed.
  * @ctrlfiles:	Array of files to create in ctrlmon directories.
  * @schema:	Driver supplied to manage a line in schemata file.
+ * @schema_override: Name of resource in schemata to override.
+ * @save_schema: List to save overridden schema while driver loaded
  */
 struct resctrl_driver {
 	struct list_head	list;
@@ -232,6 +234,8 @@ struct resctrl_driver {
 	int			(*rmdir)(int oclos, int ormid, int nclos, int nrmid);
 	struct resctrl_fileinfo	*ctrlfiles;
 	struct resctrl_schema	schema;
+	char			*schema_override;
+	struct list_head	save_schema;
 };
 
 int resctrl_register_driver(struct resctrl_driver *d);
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index cc2292a7435b..4fc12ad56843 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -2547,6 +2547,19 @@ static void rdtgroup_update_ctrl_dir(struct resctrl_fileinfo *files, bool add)
 	}
 }
 
+static void driver_override(struct resctrl_driver *d)
+{
+	struct resctrl_schema *rs;
+
+	list_for_each_entry(rs, &resctrl_schema_all, list) {
+		if (strncmp(d->schema_override, rs->name, sizeof(rs->name)))
+			continue;
+		INIT_LIST_HEAD(&d->save_schema);
+		list_move(&rs->list, &d->save_schema);
+		break;
+	}
+}
+
 static void driver_up(struct resctrl_driver *d)
 {
 	if (d->mount)
@@ -2575,6 +2588,9 @@ int resctrl_register_driver(struct resctrl_driver *d)
 	if (d->schema.name[0])
 		list_add(&d->schema.list, &resctrl_schema_all);
 
+	if (d->schema_override)
+		driver_override(d);
+
 	if (resctrl_is_mounted)
 		driver_up(d);
 	mutex_unlock(&rdtgroup_mutex);
@@ -2592,6 +2608,9 @@ void resctrl_unregister_driver(struct resctrl_driver *d)
 	if (d->schema.name[0])
 		list_del(&d->schema.list);
 
+	if (d->schema_override && !list_empty(&d->save_schema))
+		list_move(d->save_schema.next, &resctrl_schema_all);
+
 	if (resctrl_is_mounted)
 		driver_down(d);
 	mutex_unlock(&rdtgroup_mutex);
-- 
2.39.2

[RFC PATCH 7/7] x86/resctrl: Example resctrl driver

[Tony Luck]

Simply add two files to each ctrlmon directory that show the closid and
rmid respectively.

Signed-off-by: Tony Luck <tony.luck@intel.com>
---
 .../cpu/resctrl/drivers/resctrl_example.c     | 77 +++++++++++++++++++
 arch/x86/Kconfig                              | 11 +++
 arch/x86/kernel/cpu/resctrl/Makefile          |  1 +
 arch/x86/kernel/cpu/resctrl/drivers/Makefile  |  1 +
 4 files changed, 90 insertions(+)
 create mode 100644 arch/x86/kernel/cpu/resctrl/drivers/resctrl_example.c
 create mode 100644 arch/x86/kernel/cpu/resctrl/drivers/Makefile

diff --git a/arch/x86/kernel/cpu/resctrl/drivers/resctrl_example.c b/arch/x86/kernel/cpu/resctrl/drivers/resctrl_example.c
new file mode 100644
index 000000000000..24998e0dc3c2
--- /dev/null
+++ b/arch/x86/kernel/cpu/resctrl/drivers/resctrl_example.c
@@ -0,0 +1,77 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright(c) 2023 Intel Corporation. */
+
+/*
+ *  Example resctrl driver
+ */
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/acpi.h>
+#include <linux/seq_file.h>
+#include <linux/slab.h>
+#include <linux/resctrl.h>
+
+#undef pr_fmt
+#define pr_fmt(fmt) "exampleresctrl: " fmt
+
+static int closid_show(struct seq_file *sf, void *arg)
+{
+	struct kernfs_open_file *of = sf->private;
+	unsigned long priv = (unsigned long)of->kn->priv;
+	u32 closid = priv >> 16;
+
+	seq_printf(sf, "%d\n", closid);
+
+	return 0;
+}
+
+static int rmid_show(struct seq_file *sf, void *arg)
+{
+	struct kernfs_open_file *of = sf->private;
+	unsigned long priv = (unsigned long)of->kn->priv;
+	u32 rmid = priv & 0xffff;
+
+	seq_printf(sf, "%d\n", rmid);
+
+	return 0;
+}
+
+static struct kernfs_ops closid_ops = {
+	.seq_show	= closid_show,
+};
+
+static struct kernfs_ops rmid_ops = {
+	.seq_show	= rmid_show,
+};
+
+static struct resctrl_fileinfo files[] = {
+	{
+		.name = "closid", .ops = &closid_ops
+	},
+	{
+		.name = "rmid", .ops = &rmid_ops
+	},
+	{
+	}
+};
+
+static struct resctrl_driver ops = {
+	.ctrlfiles	= files,
+};
+
+static int __init init_example(void)
+{
+	resctrl_register_driver(&ops);
+
+	return 0;
+}
+
+static void __exit cleanup_example(void)
+{
+	resctrl_unregister_driver(&ops);
+}
+
+module_init(init_example);
+module_exit(cleanup_example);
+
+MODULE_LICENSE("GPL");
diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index a825bf031f49..7f2faec17365 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -500,6 +500,17 @@ config X86_CPU_RESCTRL
 
 	  Say N if unsure.
 
+config X86_CPU_RESCTRL_DRIVERS
+	bool "resctrl driver"
+
+config X86_CPU_RESCTRL_EXAMPLE_DRIVER
+	tristate "example x86 resctrl driver"
+	depends on X86_CPU_RESCTRL
+	select X86_CPU_RESCTRL_DRIVERS
+	help
+	  Example driver to show one possible use case for
+	  resctrl driver registration.
+
 if X86_32
 config X86_BIGSMP
 	bool "Support for big SMP systems with more than 8 CPUs"
diff --git a/arch/x86/kernel/cpu/resctrl/Makefile b/arch/x86/kernel/cpu/resctrl/Makefile
index 4a06c37b9cf1..7db4d729afbc 100644
--- a/arch/x86/kernel/cpu/resctrl/Makefile
+++ b/arch/x86/kernel/cpu/resctrl/Makefile
@@ -1,4 +1,5 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_X86_CPU_RESCTRL)	+= core.o rdtgroup.o monitor.o
 obj-$(CONFIG_X86_CPU_RESCTRL)	+= ctrlmondata.o pseudo_lock.o
+obj-$(CONFIG_X86_CPU_RESCTRL_DRIVERS) += drivers/
 CFLAGS_pseudo_lock.o = -I$(src)
diff --git a/arch/x86/kernel/cpu/resctrl/drivers/Makefile b/arch/x86/kernel/cpu/resctrl/drivers/Makefile
new file mode 100644
index 000000000000..27db936eb947
--- /dev/null
+++ b/arch/x86/kernel/cpu/resctrl/drivers/Makefile
@@ -0,0 +1 @@
+obj-$(CONFIG_X86_CPU_RESCTRL_EXAMPLE_DRIVER) += resctrl_example.o
-- 
2.39.2

Re: [RFC PATCH 0/7] Add driver registration i/f to resctrl

[Reinette Chatre]

Hi Tony,

On 4/20/2023 3:06 PM, Tony Luck wrote:
> This is very much proof of concept code at this stage. I have a few
> quality of service features that are hard to intergrate into the core

(integrate?)

> resctrl code because they are model specific, or have input parameters
> that do not fit neatly into the existing schemata model.

I would like to understand these two motivations better.

Regarding "because they are model specific": the features would remain
model specific no matter from where they are supported so it sounds to
me that you would like to move/contain the model specific logic to
the drivers? Why would this be a motivation since it seems that this
would still make things model specific. I do not think resctrl is
averse to model specific code when I consider the code like
cache_alloc_hsw_probe() and __check_quirks_intel().

Regarding "do not fit neatly into the existing schemata model": could
you please elaborate? If I understand correctly a driver would like
to take ownership of a line in the schemata file, I then look at
a driver as providing support for a resource. It looks like 
these new resources may not be "domain aware" so would require unique
parsing, but that is something we can do in resctrl, no? Something
like a unique "parse_line()" associated with each resctrl resource?

Considering the above it is not clear to me at this point why this
driver interface is needed. Why could each new driver not instead be
a resctrl resource?

> Also, as AMD, ARM, and now RISC-V are looking to share the core resctrl
> code, it might be helpful to have "driver" as a software layer for
> per-CPU architectural code to avoid cluttering the core.
> 
> None of my drivers are ready to post, so this series has a simple example
> driver that would meet the same debug requirements of Babu Moger's
> patch to expose the CLOSID/RMID in files in each directory:
> 
>   https://lore.kernel.org/all/168177449635.1758847.13040588638888054027.stgit@bmoger-ubuntu/
> 
> Doing this debug with a driver that can be loaded unloaded without
> having to unmount and remount the resctrl file system appears slightly
> more convenient that a "-o debug" option. But this example driver is
> really intended just as a toy example of what can be done.

The driver seems simple but I think it already shows that this can get
complicated with many sharp corners. If I understand correctly this driver
will add the "closid" and "rmid" files in every control and monitor group. This
driver thus assumes a system that supports both control and monitoring, but
that is not guaranteed. For robustness the "rmid" file should not appear
in a control group that does not support monitoring.

> 
> The series is broken into steps that add callback functions into various
> different parts of the resctrl hierarchy. That list of parts has been
> driven by the needs of the drivers that I want to write. The
> registration interface could be extended if there are additional
> hooks need for other drivers.

The boundaries of the resctrl and driver interface are not clear to me.
Looking at where the new driver API is created and how it is used in the
example code I see that this occurs in include/linux/resctrl.h. This is
the API that an architecture using resctrl is intended to use and
thus provides much more to the drivers that I'd expect it to want to or
be able to use based on this description. 

> 
> I'm looking for high level comments on the desireability of this approach

(desirability?)

> at this time. I don't expect any of this to be merged until I have some
> real drivers that use this to offer to upstream.

Some hints about scenarios that require this driver interface would be
helpful.

Apart from the high level comments above I looked briefly at the patches
and responded there where I have some high level comments/questions.

Reinette

Re: [RFC PATCH 1/7] x86/resctrl: Add register/unregister functions for driver to hook into resctrl

[Reinette Chatre]

Hi Tony,

On 4/20/2023 3:06 PM, Tony Luck wrote:
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 6ad33f355861..3e6778bde427 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -51,6 +51,9 @@ static struct kernfs_node *kn_mongrp;
>  /* Kernel fs node for "mon_data" directory under root */
>  static struct kernfs_node *kn_mondata;
>  
> +static LIST_HEAD(drivers);
> +static bool resctrl_is_mounted;
> +

fyi ...
https://lore.kernel.org/lkml/20230320172620.18254-13-james.morse@arm.com/

> +int resctrl_register_driver(struct resctrl_driver *d)
> +{
> +	mutex_lock(&rdtgroup_mutex);
> +	list_add(&d->list, &drivers);
> +
> +	if (resctrl_is_mounted)
> +		driver_up(d);
> +	mutex_unlock(&rdtgroup_mutex);
> +
> +	return 0;
> +}

Do you expect that the drivers may at any time
need to call into resctrl as opposed to relying entirely
on callbacks from resctrl? I am wondering about any potential
lock ordering issues between rdtgroup_mutex and locks
used internally by the drivers.

Reinette

Re: [RFC PATCH 3/7] x86/resctrl: Add driver callback when directories are removed

[Reinette Chatre]

Hi Tony,

On 4/20/2023 3:06 PM, Tony Luck wrote:
> When a resctrl directory is removed, entities attached to that
> directory are reassigned with the CLOSID and RMID of the parent
> directory.

Could you please elaborate what you mean with "entities"? In
resctrl there are tasks needing to move but that is done in resctrl.
Would drivers be doing any manipulation of the tasks' closid/rmid?

> 
> Add a callback function so a driver can reset the CLOSID and RMID
> of any resources attached to the removed directory.

I expect this behavior to be different between the removal of
a monitoring group vs a control group (more later) ...


> @@ -3495,11 +3495,18 @@ static int rdtgroup_mkdir(struct kernfs_node *parent_kn, const char *name,
>  static int rdtgroup_rmdir_mon(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
>  {
>  	struct rdtgroup *prdtgrp = rdtgrp->mon.parent;
> +	struct resctrl_driver *d;
>  	int cpu;
>  
>  	/* Give any tasks back to the parent group */
>  	rdt_move_group_tasks(rdtgrp, prdtgrp, tmpmask);
>  
> +	list_for_each_entry(d, &drivers, list) {
> +		if (d->rmdir)
> +			d->rmdir(rdtgrp->closid, rdtgrp->mon.rmid,
> +				 prdtgrp->closid, prdtgrp->mon.rmid);
> +	}
> +

This seems tricky ... if I understand correctly the API provides
limited properties directly to driver to keep things safe but at the
same time the properties need to accommodate all driver usages. All drivers
would need an update if something new is needed.

For example, this seems to ignore whether a resource group is exclusive
or not - could group removal trigger behavior that can circumvent this
restriction?

>  	/* Update per cpu rmid of the moved CPUs first */
>  	for_each_cpu(cpu, &rdtgrp->cpu_mask)
>  		per_cpu(pqr_state.default_rmid, cpu) = prdtgrp->mon.rmid;
> @@ -3535,6 +3542,7 @@ static int rdtgroup_ctrl_remove(struct rdtgroup *rdtgrp)
>  
>  static int rdtgroup_rmdir_ctrl(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
>  {
> +	struct resctrl_driver *d;
>  	int cpu;
>  
>  	/* Give any tasks back to the default group */
> @@ -3544,6 +3552,11 @@ static int rdtgroup_rmdir_ctrl(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
>  	cpumask_or(&rdtgroup_default.cpu_mask,
>  		   &rdtgroup_default.cpu_mask, &rdtgrp->cpu_mask);
>  
> +	list_for_each_entry(d, &drivers, list) {
> +		if (d->rmdir)
> +			d->rmdir(rdtgrp->closid, rdtgrp->mon.rmid, 0, 0);
> +	}
> +
>  	/* Update per cpu closid and rmid of the moved CPUs first */
>  	for_each_cpu(cpu, &rdtgrp->cpu_mask) {
>  		per_cpu(pqr_state.default_closid, cpu) = rdtgroup_default.closid;

... is the expectation that the driver would look at the latter parameters to
determine if a monitor or control group is removed? I think that may be troublesome
since API like this relies on driver needing to have a lot of insight into
internal implementation choices of resctrl (which could potentially change?).
If I understand correctly the driver makes assumptions like: (a) default control group
always is assigned closid 0 and rmid 0 and (b) default control group is never removed.
I think that it should not be required for drivers to have knowledge like this
(and then also risk that these assumptions may change).

Reinette

Re: [RFC PATCH 5/7] x86/resctrl: Enhance driver registration to hook into schemata files

[Reinette Chatre]

Hi Tony,

On 4/20/2023 3:06 PM, Tony Luck wrote:
> Add a new configuration type "DRIVER" for lines in the schemata files
> with a show() and update() callback functions to the driver to maintain
> these lines.

If the goal is to have unique APIs for resource, could an alternative
be the have show() and update() callbacks associated with resources in
resctrl self?

...

> @@ -2390,6 +2392,9 @@ static int schemata_list_add(struct rdt_resource *r, enum resctrl_conf_type type
>  	case CDP_NONE:
>  		suffix = "";
>  		break;
> +	case DRIVER:
> +		kfree(s);
> +		return -EINVAL;
>  	}
>  

(This could perhaps be simplified to not allocate the memory in the first place?)

Reinette

Re: [RFC PATCH 4/7] x86/resctrl: Add capability to driver registration to create control files

[Reinette Chatre]

Hi Tony,

On 4/20/2023 3:06 PM, Tony Luck wrote:
> Add a control file to each ctrlmon directory. Hook into the mkdir()
> functions to add the control file to any new directories that are
> created.

I assume you mean CTRL_MON as defined in the docs. If this is correct
the then the implementation appears to be behave different in that it
adds files to CTRL_MON as well as MON groups. It adds the files
irrespective of the group type. Is this intended? Please do correct
me where I am wrong.

Reinette

Re: [RFC PATCH 6/7] x86/resctrl: Allow a device to override an existing schemata entry

[Reinette Chatre]

Hi Tony,

On 4/20/2023 3:06 PM, Tony Luck wrote:
> Remove that entry from the resctrl_schema_all list when driver
> is loaded. Put it back again when driver is unloaded.

This is unexpected. It sounds like the system would advertise
a supported resource with appropriate properties but for some reason this
is not correct, optional in some way, or perhaps resources are 
conflicting? Where would it be decided whether the overriding driver
should be loaded and why can that logic not be in enumeration
within resctrl?

Reinette

RE: [RFC PATCH 0/7] Add driver registration i/f to resctrl

[Luck, Tony]

Reinette,

Thanks for the review. Your comments on individual patches are all good, but I'll try to address
them here to keep all the pieces together.

You sum up everything in your concise:

  "I would like to understand these two motivations better."

I'll try to give some additional details. But the specific features of the drivers I
want to add are still under wraps, so I will have to be somewhat vague at times.
If that means that a final decision can't be made until more details are forthcoming,
that's fine. At least the discussion has begun now so people have some time
to think about this well before I need to get code upstream. Or if the answer
is "No. We will never create s/w layers in the resctrl code." Then I can go
back to puzzling some other solutions.

Intel has always had some model specific "RDT" features that we have
not attempted to push to upstream. Rationale is that architectural features
have lasting value across CPU generations. Once the code is upstream it
"just works" (TM) for all time. Cluttering up core code with features that are
only applicable to one or two CPU generations seemed like a recipe for
disaster from a long-term maintenance perspective.

But things are changing. Previously the end user industries that wanted
the model specific features were content for out-of-tree patches for their
specific systems. We now have some features that may be useful to a
wider audience, ones that requires a solution integrated into upstream
so that they don't take on technical debt to move the solution to new
kernel versions.

I'm also guessing that with other architectures (AMD, ARM, RISC-V)
all building on the resctrl foundation, some of them may also have some
features that don't fit neatly into the core of resctrl.

My RFC patches were just to show that creating a s/w layer in resctrl
is possible. If there is a better dividing line between core code and 
architecture/model specific code I'm happy to discuss cleaner ways to
draw the line. E.g. you make the point here and in one of your comments
to the individual patches that making new resctrl resources may be a 
cleaner solution. I didn't do it that way partly because the existing code
has a static array of resources. But it might be relatively simple to transform
that into a list to make dynamically adding new resources easier. But see
Q6 below about tracking domains in a resource for other challenges.

Specific questions you raise:

Q1) Will there be a need for drivers to "call into" resctrl rather than rely
on call backs? May make locking complex.

A1) So far I haven't found a case. But I only have three drivers (in addition
to the example one) so it is possible that more complex things may be needed.
I agree that this will raise many locking challenges. Exporting the resctrl
mutex seems like a terrible idea.

Q2) What about exclusive groups?
A2) I didn’t try to handle in this RFC. Additional bits will be needed.

Q3) How to make visible to the driver other resctrl assumptions (e.g. default
group is CLOSID=0, RMID=0).
A3) I think this specific example is unlikely to ever change (it is somewhat tied
to the power-on/reset state of the IA32_PQR_ASSOC register. But the general
point is true that assumptions by drivers may create challenges to refactor core
code in ways that break those assumptions.

Q4) Suppressing schemata resources from a driver surprised you.
A4) This is, as you guessed, about conflicting resources. There are other ways it
could be handled. E.g. use existing mount options to suppress the resource from
the schemata. To be safe that might also need some way to fail to load of a driver
that needs other access to a resource unless the correct mount options are in
force.

Q5) Boundaries are not clear. Too much of resctrl internals made visible to drivers.
A5) Can work on more abstract interfaces if we move forward with some sort of
layered approach.

Q6) Domain awareness of drivers.
A6) This is a challenge. Especially as the domain for a driver may not match up
with any existing resource scope (e.g. driver may be socket scoped, which may
not be the same as "L3 cache" scoped). After I posted this series I added
an entry in the resource table with socket scope to handle this. Dynamically adding
a new resource with a custom scope has challenges (because the domain lists
attached to that resource are maintained by the resctrl cpu hot plug callbacks as
CPUs come online and go offline.

-Tony

Re: [RFC PATCH 0/7] Add driver registration i/f to resctrl

[Reinette Chatre]

Hi Tony,

On 5/8/2023 11:32 AM, Luck, Tony wrote:
> Reinette,
> 
> Thanks for the review. Your comments on individual patches are all good, but I'll try to address
> them here to keep all the pieces together.
> 
> You sum up everything in your concise:
> 
>   "I would like to understand these two motivations better."
> 
> I'll try to give some additional details. But the specific features of the drivers I
> want to add are still under wraps, so I will have to be somewhat vague at times.
> If that means that a final decision can't be made until more details are forthcoming,
> that's fine. At least the discussion has begun now so people have some time
> to think about this well before I need to get code upstream. Or if the answer
> is "No. We will never create s/w layers in the resctrl code." Then I can go
> back to puzzling some other solutions.
> 
> Intel has always had some model specific "RDT" features that we have
> not attempted to push to upstream. Rationale is that architectural features
> have lasting value across CPU generations. Once the code is upstream it
> "just works" (TM) for all time. Cluttering up core code with features that are
> only applicable to one or two CPU generations seemed like a recipe for
> disaster from a long-term maintenance perspective.

Could you please elaborate how this seems like a "recipe for disaster"? I
can certainly see how removing a driver is easy when it is decided that
something is "end of life". I rarely see "end of life" in practice
though and viewing removal of obsolete code from a driver as a "disaster"
is not clear to me. Re-factoring code occurs frequently.

> But things are changing. Previously the end user industries that wanted
> the model specific features were content for out-of-tree patches for their
> specific systems. We now have some features that may be useful to a
> wider audience, ones that requires a solution integrated into upstream
> so that they don't take on technical debt to move the solution to new
> kernel versions.
> 
> I'm also guessing that with other architectures (AMD, ARM, RISC-V)
> all building on the resctrl foundation, some of them may also have some
> features that don't fit neatly into the core of resctrl.

They absolutely have features that don't fit neatly into the core of
resctrl. I am not able to tell whether this style of interface would
solve this.

The second motivation from your original email was that these new features
have "input parameters that do not fit neatly into the existing schemata
model". It is not obvious to me where you address that, but maybe it is
related to "Q6".

> My RFC patches were just to show that creating a s/w layer in resctrl
> is possible. If there is a better dividing line between core code and 
> architecture/model specific code I'm happy to discuss cleaner ways to
> draw the line. E.g. you make the point here and in one of your comments
> to the individual patches that making new resctrl resources may be a 
> cleaner solution. I didn't do it that way partly because the existing code
> has a static array of resources. But it might be relatively simple to transform
> that into a list to make dynamically adding new resources easier. But see
> Q6 below about tracking domains in a resource for other challenges.
> 
> Specific questions you raise:
> 
> Q1) Will there be a need for drivers to "call into" resctrl rather than rely
> on call backs? May make locking complex.
> 
> A1) So far I haven't found a case. But I only have three drivers (in addition
> to the example one) so it is possible that more complex things may be needed.
> I agree that this will raise many locking challenges. Exporting the resctrl
> mutex seems like a terrible idea.

I agree that we should not export the mutex. You may be interested in the work
that James is working on separating the locks. 
https://lore.kernel.org/lkml/20230320172620.18254-1-james.morse@arm.com/

It would be great if you could join these discussions to see if there are
some common requirements that can be solved together.

> Q2) What about exclusive groups?
> A2) I didn’t try to handle in this RFC. Additional bits will be needed.

It seems like every time a driver needs "additional bits" it would impact
all the other drivers.

> 
> Q3) How to make visible to the driver other resctrl assumptions (e.g. default
> group is CLOSID=0, RMID=0).

Actually I think resctrl assumptions should be _invisible_ to drivers.

> A3) I think this specific example is unlikely to ever change (it is somewhat tied
> to the power-on/reset state of the IA32_PQR_ASSOC register. But the general
> point is true that assumptions by drivers may create challenges to refactor core
> code in ways that break those assumptions.
> 
> Q4) Suppressing schemata resources from a driver surprised you.
> A4) This is, as you guessed, about conflicting resources. There are other ways it
> could be handled. E.g. use existing mount options to suppress the resource from
> the schemata. To be safe that might also need some way to fail to load of a driver
> that needs other access to a resource unless the correct mount options are in
> force.

From above it sounds like there may be scenarios where a driver layer would still
be accompanied by core changes (like mount options added to the core that will 
allow/deny certain drivers). If there was no driver layer it could just be handled
in a single spot.

The second part of my original question was "Where would it be decided whether
the overriding driver should be loaded and why can that logic not be in
enumeration within resctrl?" It is the user that needs to determine that there are
conflicting resources?

> Q5) Boundaries are not clear. Too much of resctrl internals made visible to drivers.
> A5) Can work on more abstract interfaces if we move forward with some sort of
> layered approach.
> 
> Q6) Domain awareness of drivers.
> A6) This is a challenge. Especially as the domain for a driver may not match up
> with any existing resource scope (e.g. driver may be socket scoped, which may
> not be the same as "L3 cache" scoped). After I posted this series I added
> an entry in the resource table with socket scope to handle this. Dynamically adding
> a new resource with a custom scope has challenges (because the domain lists
> attached to that resource are maintained by the resctrl cpu hot plug callbacks as
> CPUs come online and go offline.

My comment was not about a need to make drivers "domain aware". My assumption was that
drivers are not domain aware since I did not see any related information shared
with the drivers and since the drivers override the schemata entries I thus assumed
that the schemata entries use some driver specific scope. 
The challenge to add a resource with a custom scope seems like the biggest problem
raised thus far. Is this perhaps what started the venture down this driver interface?

Reinette

RE: [RFC PATCH 0/7] Add driver registration i/f to resctrl

[Luck, Tony]

>> Intel has always had some model specific "RDT" features that we have
>> not attempted to push to upstream. Rationale is that architectural features
>> have lasting value across CPU generations. Once the code is upstream it
>> "just works" (TM) for all time. Cluttering up core code with features that are
>> only applicable to one or two CPU generations seemed like a recipe for
>> disaster from a long-term maintenance perspective.
>
> Could you please elaborate how this seems like a "recipe for disaster"? I
> can certainly see how removing a driver is easy when it is decided that
> something is "end of life". I rarely see "end of life" in practice
> though and viewing removal of obsolete code from a driver as a "disaster"
> is not clear to me. Re-factoring code occurs frequently.

I'm thinking of the amount of code under arch/x86/kernel/cpu/resctrl. In
v6.4-rc1 it looks like:

$ wc -l $resctrl/*.[ch]
   996 arch/x86/kernel/cpu/resctrl/core.c
   581 arch/x86/kernel/cpu/resctrl/ctrlmondata.c
   560 arch/x86/kernel/cpu/resctrl/internal.h
   845 arch/x86/kernel/cpu/resctrl/monitor.c
  1600 arch/x86/kernel/cpu/resctrl/pseudo_lock.c
    43 arch/x86/kernel/cpu/resctrl/pseudo_lock_event.h
  3733 arch/x86/kernel/cpu/resctrl/rdtgroup.c
  8358 total

Fenghua did a built-in implementation for one of the features that I'd
like to implement as a driver and the bottom line of "git diff --stat" for
his series of patches was:

9 files changed, 1300 insertions(+), 10 deletions(-)

Projecting forward a few CPU generations there may be 2-3 different
versions of that code. Plus all the other model specific features that
we'd like to support. The core resctrl architectural code is going to
disappear in the maze of "do this for CPU models X & Y, but do that
for CPU model Z". 


>> I'm also guessing that with other architectures (AMD, ARM, RISC-V)
>> all building on the resctrl foundation, some of them may also have some
>> features that don't fit neatly into the core of resctrl.
>
> They absolutely have features that don't fit neatly into the core of
> resctrl. I am not able to tell whether this style of interface would
> solve this.

I'm hoping James and others copied on this thread can chime in to
answer that.

> The second motivation from your original email was that these new features
> have "input parameters that do not fit neatly into the existing schemata
> model". It is not obvious to me where you address that, but maybe it is
> related to "Q6".

That's really tied into the specific features that aren't up for public discussion
at this point.

> I agree that we should not export the mutex. You may be interested in the work
> that James is working on separating the locks. 
> https://lore.kernel.org/lkml/20230320172620.18254-1-james.morse@arm.com/
>
> It would be great if you could join these discussions to see if there are
> some common requirements that can be solved together.

Thanks for the link. I'll go off and read that thread.

>> Q2) What about exclusive groups?
>> A2) I didn’t try to handle in this RFC. Additional bits will be needed.
>
> It seems like every time a driver needs "additional bits" it would impact
> all the other drivers.

It depends. If the new hook is just some additional callback function, then
existing drivers would have an implied ".newfunc = NULL," in the registration
structure. So wouldn't need any changes.

The hooks I implemented in my RFC series are the union of the requirements
of each driver. But each driver just sets up the hooks that it needs. E.g. my
silly example driver only used the "add files to the ctrlmon directories" hook.

>> Q3) How to make visible to the driver other resctrl assumptions (e.g. default
>> group is CLOSID=0, RMID=0).
>
> Actually I think resctrl assumptions should be _invisible_ to drivers.

That's a worthy goal. I'm not sure whether it can always be met. E.g. for
the default CLOSID/RMID case the driver will want to initialize to some
useful default because receiving callbacks from the core resctrl code as
the user writes to files to set up a specific configuration.

>> Q4) Suppressing schemata resources from a driver surprised you.
>> A4) This is, as you guessed, about conflicting resources. There are other ways it
>> could be handled. E.g. use existing mount options to suppress the resource from
>> the schemata. To be safe that might also need some way to fail to load of a driver
>> that needs other access to a resource unless the correct mount options are in
>> force.
>
>From above it sounds like there may be scenarios where a driver layer would still
>be accompanied by core changes (like mount options added to the core that will
>allow/deny certain drivers). If there was no driver layer it could just be handled
>in a single spot.

I threw out mount options as an alternative to suppressing features. Having
thought about it, I'm not fond of it at all.

> The second part of my original question was "Where would it be decided whether
> the overriding driver should be loaded and why can that logic not be in
> enumeration within resctrl?" It is the user that needs to determine that there are
> conflicting resources?

The horror of model specific features is appalling, or non-existent, enumeration.
In the dim and distant past of resctrl there was once a point where it did
sting compares of model strings against a list of specific SKUs that supported
early RDT features.

>> Q6) Domain awareness of drivers.
>> A6) This is a challenge. Especially as the domain for a driver may not match up
>> with any existing resource scope (e.g. driver may be socket scoped, which may
>> not be the same as "L3 cache" scoped). After I posted this series I added
>> an entry in the resource table with socket scope to handle this. Dynamically adding
>> a new resource with a custom scope has challenges (because the domain lists
>> attached to that resource are maintained by the resctrl cpu hot plug callbacks as
>> CPUs come online and go offline.
>
>My comment was not about a need to make drivers "domain aware". My assumption was that
>drivers are not domain aware since I did not see any related information shared
>with the drivers and since the drivers override the schemata entries I thus assumed
>that the schemata entries use some driver specific scope.
>The challenge to add a resource with a custom scope seems like the biggest problem
>raised thus far. Is this perhaps what started the venture down this driver interface?

New domain scopes wasn't a driving motivation, just a thing that was found along
the implementation journey. After playing with some ways to have each driver keep
track of scope I found that I'd replicated some of the core domain tracking cpuhp
code and decided that juat making the core keep track of a socket scoped resource
with call backs to the driver(s) for socket add/delete was the cleanest way to go.

That might mean asking the core to track other scopes (like "tile") in future if some
control/measure feature has that scope. Having created a "node" scope in my
patch series for SNC[1], it then is quite trivial to add additional resources
with any scope needed.

-Tony

[1] https://lore.kernel.org/all/20230126184157.27626-1-tony.luck@intel.com/ ... that
series is not forgotten. Just needs a slightly different approach which needs an internal
approval before I can post version 2 of the series.

Re: [RFC PATCH 0/7] Add driver registration i/f to resctrl

[Reinette Chatre]

Hi Tony,

On 5/9/2023 4:35 PM, Luck, Tony wrote:
>>> Intel has always had some model specific "RDT" features that we have
>>> not attempted to push to upstream. Rationale is that architectural features
>>> have lasting value across CPU generations. Once the code is upstream it
>>> "just works" (TM) for all time. Cluttering up core code with features that are
>>> only applicable to one or two CPU generations seemed like a recipe for
>>> disaster from a long-term maintenance perspective.
>>
>> Could you please elaborate how this seems like a "recipe for disaster"? I
>> can certainly see how removing a driver is easy when it is decided that
>> something is "end of life". I rarely see "end of life" in practice
>> though and viewing removal of obsolete code from a driver as a "disaster"
>> is not clear to me. Re-factoring code occurs frequently.
> 
> I'm thinking of the amount of code under arch/x86/kernel/cpu/resctrl. In
> v6.4-rc1 it looks like:
> 
> $ wc -l $resctrl/*.[ch]
>    996 arch/x86/kernel/cpu/resctrl/core.c
>    581 arch/x86/kernel/cpu/resctrl/ctrlmondata.c
>    560 arch/x86/kernel/cpu/resctrl/internal.h
>    845 arch/x86/kernel/cpu/resctrl/monitor.c
>   1600 arch/x86/kernel/cpu/resctrl/pseudo_lock.c
>     43 arch/x86/kernel/cpu/resctrl/pseudo_lock_event.h
>   3733 arch/x86/kernel/cpu/resctrl/rdtgroup.c
>   8358 total
> 
> Fenghua did a built-in implementation for one of the features that I'd
> like to implement as a driver and the bottom line of "git diff --stat" for
> his series of patches was:
> 
> 9 files changed, 1300 insertions(+), 10 deletions(-)
> 
> Projecting forward a few CPU generations there may be 2-3 different
> versions of that code. Plus all the other model specific features that
> we'd like to support. The core resctrl architectural code is going to
> disappear in the maze of "do this for CPU models X & Y, but do that
> for CPU model Z". 

It is hard to tell from just a diffstat how this implementation impacts
the core. A similar diffstat for the driver implementation may
help. "1300 insertions(+), 10 deletions(-)" does not seem like a lot of 
core refactoring.

>>> Q2) What about exclusive groups?
>>> A2) I didn’t try to handle in this RFC. Additional bits will be needed.
>>
>> It seems like every time a driver needs "additional bits" it would impact
>> all the other drivers.
> 
> It depends. If the new hook is just some additional callback function, then
> existing drivers would have an implied ".newfunc = NULL," in the registration
> structure. So wouldn't need any changes.
> 
> The hooks I implemented in my RFC series are the union of the requirements
> of each driver. But each driver just sets up the hooks that it needs. E.g. my
> silly example driver only used the "add files to the ctrlmon directories" hook.

My point is that the hooks themselves appear to be made safe by just providing
limited information (no pointers back to structures maintained by resctrl) and
thus when a new driver has different requirements it would have broad impact.
Similar to the example driver that you provided, if I understood correctly it
already pointed out that there may be a missing parameter of the group type
(control vs monitor).

>> The second part of my original question was "Where would it be decided whether
>> the overriding driver should be loaded and why can that logic not be in
>> enumeration within resctrl?" It is the user that needs to determine that there are
>> conflicting resources?
> 
> The horror of model specific features is appalling, or non-existent, enumeration.
> In the dim and distant past of resctrl there was once a point where it did
> sting compares of model strings against a list of specific SKUs that supported
> early RDT features.

My question was trying to understand where this logic is moved to (re. "Where would
it be decided whether the overriding driver should be loaded"). The feature will
remain model specific whether it is implemented in the core or a driver, so these
checks will need to be done somewhere, no? 

> 
>>> Q6) Domain awareness of drivers.
>>> A6) This is a challenge. Especially as the domain for a driver may not match up
>>> with any existing resource scope (e.g. driver may be socket scoped, which may
>>> not be the same as "L3 cache" scoped). After I posted this series I added
>>> an entry in the resource table with socket scope to handle this. Dynamically adding
>>> a new resource with a custom scope has challenges (because the domain lists
>>> attached to that resource are maintained by the resctrl cpu hot plug callbacks as
>>> CPUs come online and go offline.
>>
>> My comment was not about a need to make drivers "domain aware". My assumption was that
>> drivers are not domain aware since I did not see any related information shared
>> with the drivers and since the drivers override the schemata entries I thus assumed
>> that the schemata entries use some driver specific scope.
>> The challenge to add a resource with a custom scope seems like the biggest problem
>> raised thus far. Is this perhaps what started the venture down this driver interface?
> 
> New domain scopes wasn't a driving motivation, just a thing that was found along
> the implementation journey. After playing with some ways to have each driver keep
> track of scope I found that I'd replicated some of the core domain tracking cpuhp
> code and decided that juat making the core keep track of a socket scoped resource
> with call backs to the driver(s) for socket add/delete was the cleanest way to go.
> 
> That might mean asking the core to track other scopes (like "tile") in future if some
> control/measure feature has that scope. Having created a "node" scope in my
> patch series for SNC[1], it then is quite trivial to add additional resources
> with any scope needed.

I see. I thought that tracking scope was the hardest problem needing solving
in the driver.

Reinette

RE: [RFC PATCH 0/7] Add driver registration i/f to resctrl

[Luck, Tony]

> It is hard to tell from just a diffstat how this implementation impacts
> the core. A similar diffstat for the driver implementation may
> help. "1300 insertions(+), 10 deletions(-)" does not seem like a lot of 
> core refactoring.

Here's the resctrl bits of Fenghua's diffstat (with the filename of the new
feature changed to avoid giving too much away):

 arch/x86/kernel/cpu/resctrl/core.c        |  39 +++-
 arch/x86/kernel/cpu/resctrl/newfeature.c  | 780 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/cpu/resctrl/monitor.c     |   4 +-
 arch/x86/kernel/cpu/resctrl/rdtgroup.c    | 333 ++++++++++++++++++++++++++++-

Changes to core files are ~375 lines ... about 50% more than I added to core code
to provide a registration hook for this and many other drivers.  My driver registration
hooks will likely inflate by that much when the rough edges are smoothed.

-Tony

RE: [RFC PATCH 0/7] Add driver registration i/f to resctrl

[Luck, Tony]

Reinette,

You asked for examples of resources that don't fit well into the existing schemata model.

Here's the details behind one of those cases.

Existing resources have a single value per domain (e.g. a cache-way bitmask, or a memory
bandwidth percentage or target). One of my new resources has several parameters. At first
glance this might be solved by just listing a comma separated list of these parameters for
each domain. So the schemata entry for feature XYZ that has two domains might look like
this:

XYZ:0=param1,param2,param3;1=param1,param2,param3

But this feature has a second problem. The hardware supports a very limited set of variations.
This could be handled by reporting num_closid for this resource to that low number. But then
resctrl core code would limit all resources to that value. Instead the h/w allows programming
a mapping feature from closid numbers to resource instances (as the saying goes "any computer
science problem can be solved with one extra level of indirection").

So if the driver named these instances: A, B, C, D. Then a schemata file might look like:

XYZ:0=B;1=C

meaning that the driver will set up so CLOSID for this resctrl resource is mapped to instance "B"
on domain 0 and to instance "C" on domain 1.

The driver provides a way to set param1, param2, param3 for each of the A, B, C, D
instances.

-Tony

Re: [RFC PATCH 0/7] Add driver registration i/f to resctrl

[Reinette Chatre]

Hi Tony,

On 5/11/2023 1:35 PM, Luck, Tony wrote:
> Reinette,
> 
> You asked for examples of resources that don't fit well into the
> existing schemata model.
Primarily about me trying to understand what about the existing schemata
model makes these new features hard to support. My first confusion is
that it is not clear to me what you mean with "schemata
model". I think there are two parts to this: the schemata model as
the user interface and the schemata model as implemented within
resctrl.

It is not clear in your motivation whether you are talking about the
former or the latter and thus dug through the patches to figure this
out. At first it seemed as though the driver aims for a totally
different configuration mechanism with the support for driver specific
writable files in every group ... but then then the driver pivots and
hooks into schemata files so it then seems that the driver mechanism does
aim to maintain the schemata model as user interface (perhaps with
additional configuration files to give more meaning to the values
written to the schemata file).

So, it seems to me that the concern is not with the user interface, but
how the schemata parsing is done in resctrl. I just seem to keep guessing
here so I would appreciate more insight.

> Here's the details behind one of those cases.
> 
> Existing resources have a single value per domain (e.g. a cache-way
> bitmask, or a memory bandwidth percentage or target). One of my new
> resources has several parameters. At first glance this might be
> solved by just listing a comma separated list of these parameters
> for each domain. So the schemata entry for feature XYZ that has two
> domains might look like this:

 
> XYZ:0=param1,param2,param3;1=param1,param2,param3
> 
> But this feature has a second problem. The hardware supports a very
> limited set of variations. This could be handled by reporting
> num_closid for this resource to that low number. But then resctrl
> core code would limit all resources to that value. Instead the h/w
> allows programming a mapping feature from closid numbers to resource
> instances (as the saying goes "any computer science problem can be
> solved with one extra level of indirection").
> 
> So if the driver named these instances: A, B, C, D. Then a schemata
> file might look like:
> 
> XYZ:0=B;1=C
> 
> meaning that the driver will set up so CLOSID for this resctrl
> resource is mapped to instance "B" on domain 0 and to instance "C" on
> domain 1
Apologies but it is still not clear to me how this cannot be handled
in resctrl. For example, consider this hypothetical snippet that uses
a unique callback to parse a resource's schemata entry (similar to the
s->update() callback you introduce, but not have it be optional):

 	struct resctrl_schema *s;
 
 	list_for_each_entry(s, &resctrl_schema_all, list) {
		if (!strcmp(resname, s->name) && rdtgrp->closid < s->num_closid)
			return s->parse_line(tok, s, rdtgrp);
 	}
 	rdt_last_cmd_printf("Unknown or unsupported resource name '%s'\n", resname);

 
> The driver provides a way to set param1, param2, param3 for each of
> the A, B, C, D instances.

Seems like these could be some new RFTYPE_RES_XYZ files?

On the model specific motivation topic: If the goal of the driver
interface is to support model specific changes to resctrl there is a
publicly available use case for consideration:
https://lore.kernel.org/lkml/20230421141723.2405942-1-peternewman@google.com/

It is not obvious to me how that model specific problem could be solved
with this driver interface. Your insight here would be appreciated but it
seems to me that we will have to keep accommodating model specific code
in resctrl.

Reinette

RE: [RFC PATCH 0/7] Add driver registration i/f to resctrl

[Luck, Tony]

>> You asked for examples of resources that don't fit well into the
>> existing schemata model.
>Primarily about me trying to understand what about the existing schemata
>model makes these new features hard to support. My first confusion is
>that it is not clear to me what you mean with "schemata
>model". I think there are two parts to this: the schemata model as
>the user interface and the schemata model as implemented within
>resctrl.
>
>It is not clear in your motivation whether you are talking about the
>former or the latter and thus dug through the patches to figure this
>out. At first it seemed as though the driver aims for a totally
>different configuration mechanism with the support for driver specific
>writable files in every group ... but then then the driver pivots and
>hooks into schemata files so it then seems that the driver mechanism does
>aim to maintain the schemata model as user interface (perhaps with
>additional configuration files to give more meaning to the values
>written to the schemata file).

Different drivers may choose different subsets of the hooks available.

I have one case where a new file is, IMHO, required. There's no way to
fit what needs to be done into schemata. Another where I thought
the schemata hook makes sense (though that could be handled with
a file ... but it would look very schemata-like). 

>So, it seems to me that the concern is not with the user interface, but
>how the schemata parsing is done in resctrl. I just seem to keep guessing
>here so I would appreciate more insight.
>
>> Here's the details behind one of those cases.
>>
>> Existing resources have a single value per domain (e.g. a cache-way
>> bitmask, or a memory bandwidth percentage or target). One of my new
>> resources has several parameters. At first glance this might be
>> solved by just listing a comma separated list of these parameters
>> for each domain. So the schemata entry for feature XYZ that has two
>> domains might look like this:
>
>
>> XYZ:0=param1,param2,param3;1=param1,param2,param3
>>
>> But this feature has a second problem. The hardware supports a very
>> limited set of variations. This could be handled by reporting
>> num_closid for this resource to that low number. But then resctrl
>> core code would limit all resources to that value. Instead the h/w
>> allows programming a mapping feature from closid numbers to resource
>> instances (as the saying goes "any computer science problem can be
>> solved with one extra level of indirection").
>>
>> So if the driver named these instances: A, B, C, D. Then a schemata
>> file might look like:
>>
>> XYZ:0=B;1=C
>>
>> meaning that the driver will set up so CLOSID for this resctrl
>> resource is mapped to instance "B" on domain 0 and to instance "C" on
>> domain 1
>Apologies but it is still not clear to me how this cannot be handled
>in resctrl. For example, consider this hypothetical snippet that uses
>a unique callback to parse a resource's schemata entry (similar to the
>s->update() callback you introduce, but not have it be optional):
>
>       struct resctrl_schema *s;
>
>       list_for_each_entry(s, &resctrl_schema_all, list) {
>               if (!strcmp(resname, s->name) && rdtgrp->closid < s->num_closid)
>                       return s->parse_line(tok, s, rdtgrp);
>       }
>       rdt_last_cmd_printf("Unknown or unsupported resource name '%s'\n", resname);

That is much cleaner. It requires patching up the existing resctrl_schema to provide
a ->parser_line()  ... also a ->show_line() function.

If this proceeds, I can make the changes to provide this consistent interface. Thanks.

>> The driver provides a way to set param1, param2, param3 for each of
>> the A, B, C, D instances.
>
>Seems like these could be some new RFTYPE_RES_XYZ files?

In my current implementation the driver can make some/all of the files added
under the info/ directory writable. So this XYZ driver uses info/XYZ/param* files
as the interface for the user to set the parameters.

>
>On the model specific motivation topic: If the goal of the driver
>interface is to support model specific changes to resctrl there is a
>publicly available use case for consideration:
>https://lore.kernel.org/lkml/20230421141723.2405942-1-peternewman@google.com/
>
>It is not obvious to me how that model specific problem could be solved
>with this driver interface. Your insight here would be appreciated but it
>seems to me that we will have to keep accommodating model specific code
>in resctrl.

All my drivers are currently related to the control features of resctrl rather than the
monitor features. I don't see a way for a model specific driver to step in and mitigate
that particular problem that Peter has.

Seems quite hacky though. The trick with the "hard" RMID per core is going to miscount
LLC eviction traffic. If a core is switching rapidly between tasks with different RMIDs
that could be a significant difference from actual values.


But requests for some model specific quirks to work around h/w limitations doesn't
mean that we shouldn't create a driver layer for cases where it is possible to
separate s/w into layers.

-Tony

Re: [RFC PATCH 0/7] Add driver registration i/f to resctrl

[Reinette Chatre]

Hi Tony,

On 5/12/2023 1:35 PM, Luck, Tony wrote:

>>> The driver provides a way to set param1, param2, param3 for each of
>>> the A, B, C, D instances.
>>
>> Seems like these could be some new RFTYPE_RES_XYZ files?
> 
> In my current implementation the driver can make some/all of the files added
> under the info/ directory writable. So this XYZ driver uses info/XYZ/param* files
> as the interface for the user to set the parameters.

It sounds as though you are concerned about making info/* files writable?
I am not sure because whether the files are made writable via the core or
a driver does not matter since it will still be writable from the user
interface perspective.

Please note that there already are a few info files that are writable:
info/L3_MON/max_threshold_occupancy
info/L3_MON/mbm_total_bytes_config
info/L3_MON/mbm_local_bytes_config

It thus seems appropriate to use the info/* files as global resource
configuration and files within the resource groups handling local
configurations.

>> On the model specific motivation topic: If the goal of the driver
>> interface is to support model specific changes to resctrl there is a
>> publicly available use case for consideration:
>> https://lore.kernel.org/lkml/20230421141723.2405942-1-peternewman@google.com/
>>
>> It is not obvious to me how that model specific problem could be solved
>> with this driver interface. Your insight here would be appreciated but it
>> seems to me that we will have to keep accommodating model specific code
>> in resctrl.
> 
> All my drivers are currently related to the control features of resctrl rather than the
> monitor features. I don't see a way for a model specific driver to step in and mitigate
> that particular problem that Peter has.
> 
> Seems quite hacky though. The trick with the "hard" RMID per core is going to miscount
> LLC eviction traffic. If a core is switching rapidly between tasks with different RMIDs
> that could be a significant difference from actual values.

The submission is upfront about this and indeed disables LLC occupancy 
when "soft RMIDs" are enabled.
Please do provide direct feedback to that submission.

> But requests for some model specific quirks to work around h/w limitations doesn't
> mean that we shouldn't create a driver layer for cases where it is possible to
> separate s/w into layers.

Some of my questions were trimmed from replies without answers
so I am still trying to understand the driver interface motivation that
something being model specific is hard to integrate.

Reinette