x86/resctrl: Add "*" shorthand to set minimum io_alloc CBM for all domains

This patch introduces two helpers to validate io_alloc support and whether it is enabled. This reduces code duplication, clarifies intent, and preserves existing semantics and messages.

Signed-off-by Aaron Tomlin <[email protected]>

---
 fs/resctrl/ctrlmondata.c | 74 +++++++++++++++++++++++++++++-----------
 1 file changed, 54 insertions(+), 20 deletions(-)

fs/resctrl/ctrlmondata.c +52 -16

index b2d178d3556e..d2f9a4f2d887 100644
@@ -758,6 +758,50 @@ u32 resctrl_io_alloc_closid(struct rdt_resource *r)
 		return resctrl_arch_get_num_closid(r) - 1;
 }
 
+/*
+ * resctrl_io_alloc_supported() - Establish if io_alloc is supported
+ *
+ * @s: resctrl resource schema.
+ *
+ * This function must be called under the cpu hotplug lock
+ * and rdtgroup mutex
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+static inline int resctrl_io_alloc_supported(struct resctrl_schema *s)
+{
+	struct rdt_resource *r = s->res;
+
+	if (!r->cache.io_alloc.io_alloc_capable) {
+		rdt_last_cmd_printf("io_alloc is not supported on %s\n", s->name);
+		return -ENODEV;
+	}
+
+	return 0;
+}
+
+/*
+ * resctrl_io_alloc_enabled() - Establish if io_alloc is enabled
+ *
+ * @s: resctrl resource schema
+ *
+ * This function must be called under the cpu hotplug lock
+ * and rdtgroup mutex
+ *
+ * Return: 0 on success, negative error code otherwise.
+ */
+static inline int resctrl_io_alloc_enabled(struct resctrl_schema *s)
+{
+	struct rdt_resource *r = s->res;
+
+	if (!resctrl_arch_get_io_alloc_enabled(r)) {
+		rdt_last_cmd_printf("io_alloc is not enabled on %s\n", s->name);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 ssize_t resctrl_io_alloc_write(struct kernfs_open_file *of, char *buf,
 			       size_t nbytes, loff_t off)
 {

@@ -777,11 +821,9 @@ ssize_t resctrl_io_alloc_write(struct kernfs_open_file *of, char *buf,
 
 	rdt_last_cmd_clear();
 
-	if (!r->cache.io_alloc_capable) {
-		rdt_last_cmd_printf("io_alloc is not supported on %s\n", s->name);
-		ret = -ENODEV;
+	ret = resctrl_io_alloc_supported(s);
+	if (ret)
 		goto out_unlock;
-	}
 
 	/* If the feature is already up to date, no action is needed. */
 	if (resctrl_arch_get_io_alloc_enabled(r) == enable)

@@ -839,17 +881,13 @@ int resctrl_io_alloc_cbm_show(struct kernfs_open_file *of, struct seq_file *seq,
 
 	rdt_last_cmd_clear();
 
-	if (!r->cache.io_alloc_capable) {
-		rdt_last_cmd_printf("io_alloc is not supported on %s\n", s->name);
-		ret = -ENODEV;
+	ret = resctrl_io_alloc_supported(s);
+	if (ret)
 		goto out_unlock;
-	}
 
-	if (!resctrl_arch_get_io_alloc_enabled(r)) {
-		rdt_last_cmd_printf("io_alloc is not enabled on %s\n", s->name);
-		ret = -EINVAL;
+	ret = resctrl_io_alloc_enabled(s);
+	if (ret)
 		goto out_unlock;
-	}
 
 	/*
 	 * When CDP is enabled, the CBMs of the highest CLOSID of CDP_CODE and

@@ -928,17 +966,13 @@ ssize_t resctrl_io_alloc_cbm_write(struct kernfs_open_file *of, char *buf,
 	mutex_lock(&rdtgroup_mutex);
 	rdt_last_cmd_clear();
 
-	if (!r->cache.io_alloc_capable) {
-		rdt_last_cmd_printf("io_alloc is not supported on %s\n", s->name);
-		ret = -ENODEV;
+	ret = resctrl_io_alloc_supported(s);
+	if (ret)
 		goto out_unlock;
-	}
 

How does this patch benefit the goal of this submission, which is to set identical CBM on all domains?

If this change benefited this submission I should find these new helpers used in later patches but they are not. This seems to be an unrelated and unnecessary change mixed with this submission. It does not fix a bug nor does it support this submission and thus just adds noise when considering the new feature for inclusion. This patch can be dropped.

Quoted 8 lines · Reinette Chatre

How does this patch benefit the goal of this submission, which is to set identical CBM on all domains?

If this change benefited this submission I should find these new helpers used in later patches but they are not. This seems to be an unrelated and unnecessary change mixed with this submission. It does not fix a bug nor does it support this submission and thus just adds noise when considering the new feature for inclusion. This patch can be dropped.

Hi Reinette,

Thank you for your feedback regarding the relevance of this patch to the overall objective.

To be clear, this change was included as part of the broader series. Whilst I grant that this patch is not a strong requirement, I deem it a useful prerequisite clean-up. In my view, streamlining the existing infrastructure before layering on new features is a sound practice that prevents the accumulation of technical debt.

Given that this refactoring provides a cleaner foundation, I would propose that it remain in the series rather than being excluded. I believe the long-term benefit to the maintainability of the resctrl code outweighs the concern of it being "noise" in the context of this specific feature.

I have a different view on how this patch impacts maintainability. There are several techniques to help developers. For example, while the new functions introduced in this patch intend to be helpful to developer by including the comment "This function must be called under the cpu hotplug lock and and rdtgroup mutex" the right way to communicate this is to use lockdep_assert_cpus_held() and lockdep_assert_held(&rdtgroup_mutex). Existence of these tools demonstrate that even while knowing what the right thing to do is, mistakes can still appear.

Something else required by these new functions is that rdt_last_cmd_clear() needs to be called beforehand. This is not possible to automate like the examples above and thus relies on developer to "get right". As one that have seen many patches flow into resctrl I can say with confidence that this is one of the things where there are often mistakes.

With that in mind, note how every hunk includes rdt_last_cmd_clear() followed by the rdt_last_cmd_printf() being moved. How the buffer is used cannot be more clear, right? This patch adds a layer of indirection that makes this relationship more difficult to see. It thus does not simplify how to reason about this code.

Surely, rdt_last_cmd_clear() is not required to be a few lines away from a call that writes to the buffer but having these calls in the same function/scope makes it obvious where the buffer is cleared and where data is written to it.

Also, as resctrl documentation states about the "last_cmd_status" file: "If the command failed, it will provide more information that can be conveyed in the error returns from file operations.". Now, while keeping this in mind, consider, for example how resctrl_io_alloc_write() appears to developer before and after this change. The current implementation is consistent: every time there is a failure it is accompanied by a write to last_cmd_status buffer to make sure the error details are conveyed to user space. After this change the function is inconsistent: some errors result in a print to last_cmd_status and some do not. It is not that the print to last_cmd_status is removed but it is behind another layer of indirection that makes resctrl_io_alloc_write() more difficult to read. A developer can no longer just look at resctrl_io_alloc_write() and learn how it interacts with user space.

Same for the error codes. It is important to know and be consistent which error codes are returned to user space. Adding these behind another layer of indirection where that is all that function does seems unnecessary to me.

In summary, no, I do not see how this change benefits maintainability.

The rdtgroup_seqfile_show() function, which is the sequence file handler for reading data from resctrl files, previously returned 0 (success) if the file's associated rftype did not define a .seq_show implementation.

This behavior is incorrect and confusing, as a read operation that does not define a display function should be treated as an error.

Hi Aaron,

How is this change required to support the feature of enabling user to set CBM across domains?

Quoted 4 lines

The rdtgroup_seqfile_show() function, which is the sequence file handler for reading data from resctrl files, previously returned 0 (success) if the file's associated rftype did not define a .seq_show implementation.

Quoted 2 lines

This behavior is incorrect and confusing, as a read operation that does not define a display function should be treated as an error.

Why should it be treated as an error? Not having rftype::seq_show() set when user is intended to be able to see data when reading from the file is a kernel bug. Otherwise it seems fine to return nothing when there is nothing to show and doing so be successful.

This patch introduces a special domain ID selector "*" for io_alloc_cbm

that allows setting the CBM of each cache domain to its minimum number of consecutive bits in a single operation. For example, writing "*=0" to

/sys/fs/resctrl/info/L3/io_alloc_cbm programs each domain's CBM to the hardware-defined minimum, greatly simplifying automation and management tasks. The user is required to specify the correct minimum stored in /sys/fs/resctrl/info/L3/min_cbm_bits.

Signed-off-by Aaron Tomlin <[email protected]>

---
 Documentation/filesystems/resctrl.rst     |  10 ++
 arch/x86/kernel/cpu/resctrl/core.c        |   2 +-
 arch/x86/kernel/cpu/resctrl/ctrlmondata.c |  23 ++--
 fs/resctrl/ctrlmondata.c                  | 141 ++++++++++++++++------
 fs/resctrl/internal.h                     |  13 ++
 fs/resctrl/rdtgroup.c                     |   2 +-
 include/linux/resctrl.h                   |  30 ++++-
 7 files changed, 174 insertions(+), 47 deletions(-)

Documentation/filesystems/resctrl.rst +10 -0

index 8c8ce678148a..9c128c8fba86 100644
@@ -215,6 +215,16 @@ related to allocation:
 			# cat /sys/fs/resctrl/info/L3/io_alloc_cbm
 			0=00ff;1=000f
 
+		Set each CBM to their minimum number of consecutive bits.
+
+		A special value "*" is required to represent all cache IDs.
+		The user should specify the correct minimum stored in
+		/sys/fs/resctrl/info/L3/min_cbm_bits.
+
+		Example::
+
+			# echo "*=0" > /sys/fs/resctrl/info/L3/io_alloc_cbm
+
 		When CDP is enabled "io_alloc_cbm" associated with the CDP_DATA and CDP_CODE
 		resources may reflect the same values. For example, values read from and
 		written to /sys/fs/resctrl/info/L3DATA/io_alloc_cbm may be reflected by

arch/x86/kernel/cpu/resctrl/core.c +1 -1

index 3792ab4819dc..44aea6b534e0 100644
@@ -276,7 +276,7 @@ static void rdt_get_cdp_config(int level)
 
 static void rdt_set_io_alloc_capable(struct rdt_resource *r)
 {
-	r->cache.io_alloc_capable = true;
+	r->cache.io_alloc.io_alloc_capable = true;
 }
 
 static void rdt_get_cdp_l3_config(void)

arch/x86/kernel/cpu/resctrl/ctrlmondata.c +4 -8

index b20e705606b8..0f051d848422 100644
@@ -57,14 +57,19 @@ int resctrl_arch_update_domains(struct rdt_resource *r, u32 closid)
 		hw_dom = resctrl_to_arch_ctrl_dom(d);
 		msr_param.res = NULL;
 		for (t = 0; t < CDP_NUM_TYPES; t++) {
-			cfg = &hw_dom->d_resctrl.staged_config[t];
-			if (!cfg->have_new_ctrl)
-				continue;
-
-			idx = resctrl_get_config_index(closid, t);
-			if (cfg->new_ctrl == hw_dom->ctrl_val[idx])
-				continue;
-			hw_dom->ctrl_val[idx] = cfg->new_ctrl;
+			if (resctrl_should_io_alloc_min_cbm(r)) {
+				idx = resctrl_get_config_index(closid, t);
+				hw_dom->ctrl_val[idx] = apply_io_alloc_min_cbm(r);
+			} else {

arch/x86/kernel/cpu/resctrl/ctrlmondata.c +1 -1

 			if (!msr_param.res) {
 				msr_param.low = idx;

@@ -123,7 +128,7 @@ int resctrl_arch_io_alloc_enable(struct rdt_resource *r, bool enable)
 {
 	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
 
-	if (hw_res->r_resctrl.cache.io_alloc_capable &&
+	if (hw_res->r_resctrl.cache.io_alloc.io_alloc_capable &&
 	    hw_res->sdciae_enabled != enable) {
 		_resctrl_sdciae_enable(r, enable);
 		hw_res->sdciae_enabled = enable;

fs/resctrl/ctrlmondata.c +53 -6

index d2f9a4f2d887..a36b9055a1be 100644
@@ -688,7 +688,7 @@ int resctrl_io_alloc_show(struct kernfs_open_file *of, struct seq_file *seq, voi
 
 	mutex_lock(&rdtgroup_mutex);
 
-	if (r->cache.io_alloc_capable) {
+	if (r->cache.io_alloc.io_alloc_capable) {
 		if (resctrl_arch_get_io_alloc_enabled(r))
 			seq_puts(seq, "enabled\n");
 		else

@@ -712,13 +712,31 @@ static bool resctrl_io_alloc_closid_supported(u32 io_alloc_closid)
 	return io_alloc_closid < closids_supported();
 }
 
+/**
+ * resctrl_sync_cdp_peer - Mirror staged configuration to the CDP peer type
+ *
+ * @s: resctrl schema
+ * @d: resctrl control domain
+ *
+ * The caller must ensure CDP is enabled for the resource and must be
+ * called under the cpu hotplug lock and rdtgroup mutex
+ */
+static void resctrl_sync_cdp_peer(struct resctrl_schema *s, struct rdt_ctrl_domain *d)
+{
+	enum resctrl_conf_type peer_type;
+
+	peer_type = resctrl_peer_type(s->conf_type);
+	memcpy(&d->staged_config[peer_type],
+	       &d->staged_config[s->conf_type],
+	       sizeof(d->staged_config[0]));
+}
+
 /*
  * Initialize io_alloc CLOSID cache resource CBM with all usable (shared
  * and unused) cache portions.
  */
 static int resctrl_io_alloc_init_cbm(struct resctrl_schema *s, u32 closid)
 {
-	enum resctrl_conf_type peer_type;
 	struct rdt_resource *r = s->res;
 	struct rdt_ctrl_domain *d;
 	int ret;

@@ -731,11 +749,8 @@ static int resctrl_io_alloc_init_cbm(struct resctrl_schema *s, u32 closid)
 
 	/* Keep CDP_CODE and CDP_DATA of io_alloc CLOSID's CBM in sync. */
 	if (resctrl_arch_get_cdp_enabled(r->rid)) {
-		peer_type = resctrl_peer_type(s->conf_type);
 		list_for_each_entry(d, &s->res->ctrl_domains, hdr.list)
-			memcpy(&d->staged_config[peer_type],
-			       &d->staged_config[s->conf_type],
-			       sizeof(d->staged_config[0]));
+			resctrl_sync_cdp_peer(s, d);
 	}
 
 	ret = resctrl_arch_update_domains(r, closid);

@@ -903,49 +918,103 @@ int resctrl_io_alloc_cbm_show(struct kernfs_open_file *of, struct seq_file *seq,
 	return ret;
 }
 
+/**
+ * parse_domain_cbm - Parse "domain=cbm" and return either side
+ *
+ * @line_ptr: Pointer to the input string
+ * @out_val: Output from parsed value (either domain ID or CDM)
+ * @which: Selector for which side to parse
+ *
+ * It is assumed that *line_ptr and *out_val are valid.
+ *
+ * Return: 0 on success and advance *line_ptr to point past the
+ * delimiter, EINVAL on parsing error
+ */
+static inline int parse_domain_cbm(char **line_ptr, unsigned long *out_val,
+				   enum resctrl_kv_sel which)
+{
+	char *rhs, *lhs, *tok;
+	unsigned int base;
+
+	rhs = *line_ptr;
+
+	lhs = strsep(&rhs, "=");
+	if (!lhs || !rhs)
+		goto err;
+
+	if (which == RDT_PARSE_DOMAIN) {
+		tok = lhs;
+		base = 10;
+	} else {
+		tok = rhs;
+		base = 16;
+	}
+	tok = strim(tok);

fs/resctrl/ctrlmondata.c +4 -6

+
 static int resctrl_io_alloc_parse_line(char *line,  struct rdt_resource *r,
 				       struct resctrl_schema *s, u32 closid)
 {
-	enum resctrl_conf_type peer_type;
 	struct rdt_parse_data data;
 	struct rdt_ctrl_domain *d;
-	char *dom = NULL, *id;
-	unsigned long dom_id;
+	char *cbm = NULL;
+	unsigned long out_val;
+	int ret;
 
-next:
-	if (!line || line[0] == '\0')
-		return 0;

fs/resctrl/ctrlmondata.c +26 -17

+		}
+		rdt_last_cmd_puts("Invalid io_alloc min CBM\n");
 		return -EINVAL;
 	}
 
-	dom = strim(dom);
-	list_for_each_entry(d, &r->ctrl_domains, hdr.list) {
-		if (d->hdr.id == dom_id) {
-			data.buf = dom;
-			data.mode = RDT_MODE_SHAREABLE;
-			data.closid = closid;
-			if (parse_cbm(&data, s, d))
-				return -EINVAL;
-			/*
-			 * Keep io_alloc CLOSID's CBM of CDP_CODE and CDP_DATA
-			 * in sync.
-			 */
-			if (resctrl_arch_get_cdp_enabled(r->rid)) {
-				peer_type = resctrl_peer_type(s->conf_type);
-				memcpy(&d->staged_config[peer_type],
-				       &d->staged_config[s->conf_type],
-				       sizeof(d->staged_config[0]));
+	while (line && line[0] != '\0') {
+		bool found = false;
+
+		cbm = strsep(&line, ";");
+		ret = parse_domain_cbm(&cbm, &out_val, RDT_PARSE_DOMAIN);
+		if (ret)
+			return ret;
+
+		cbm = strim(cbm);
+		list_for_each_entry(d, &r->ctrl_domains, hdr.list) {
+			if (d->hdr.id == out_val) {
+				data.buf = cbm;
+				data.mode = RDT_MODE_SHAREABLE;
+				data.closid = closid;
+				if (parse_cbm(&data, s, d))
+					return -EINVAL;
+				/*
+				 * Keep io_alloc CLOSID's CBM of CDP_CODE and CDP_DATA
+				 * in sync.
+				 */
+				if (resctrl_arch_get_cdp_enabled(r->rid))
+					resctrl_sync_cdp_peer(s, d);
+				found = true;
+				break;
 			}

Kernel code and patches to kernel have a couple of style requirements that are documented. It takes some getting used to but it is required that submissions follow the kernel style in order to be considered for inclusion. Documentation relevant to above are:
Documentation/process/submitting-patches.rst: Consider whole document but specifically search for "This patch" for information related to above. Documentation/process/maintainer-tip.rst: Consider whole document but specifically consider "Changelog" relevant to this part.

Is there a reason to limit this implementation to only support the minimum CBM? This feature can easily enable a user to set identical CBM across all domains without the CBM being required to be the minimum CBM, no?

No. resctrl_arch_update_domains() is a helper to just program configurations. Please avoid bleeding feature specific handling into dedicated helpers.

Why is this new parser needed?

This does not look right. If I understand correctly out_val contains the user provided value/CBM, that is if user provided "*=3" then out_val contains "3". This value is in turn compared against the *number of bits* required. First, if indeed just forcing user to set the minimum CBM (which I already mentioned seems restrictive) then if out_val is "3" and min_cbm_bits is "2" then this check will fail while the user indeed did set the minimum CBM, no? Additionally, user providing value of 6 or C or any other value with two consecutive bits set would also be a valid value as a "minimum", no?

This additional parsing and checks adds unnecessary complexity. Just keep original parsing that relies on parse_cbm() that calls cbm_validate() that ensures the provided CBM is valid for this resource while taking min_cbm_bits into account.

I agree with Babu [1]. This is a lot of complexity just to support handling of an additional character. resctrl does a lot of parsing of per-domain user space input and the current implementation follows the same pattern used throughout resctrl. Introducing a new pattern and

parser unique to IO alloc feature adds unnecessary complexity. Simple and consistent is better. I considered your response [2] but to me this code is not more readable than a simple implementation built on top of existing, familiar, and consistent patterns. Another motivation from [2] is that this is easier to maintain but I fail to see that. About the motivation from [2] for "possible enhancements down the line" ... when/if need for an enhancement is required then code can be refactored to support it.

On a workflow note: please allow for discussions about your submission. Taking a long time to respond and then responding that you disagree with feedback immediately followed by a new version of the series that does not address feedback is not productive.

This patch introduces a special domain ID selector "*" for io_alloc_cbm that allows setting the CBM of each cache domain to its minimum number of consecutive bits in a single operation. For example, writing "*=0" to /sys/fs/resctrl/info/L3/io_alloc_cbm programs each domain's CBM to the hardware-defined minimum, greatly simplifying automation and management tasks. The user is required to specify the correct minimum stored in /sys/fs/resctrl/info/L3/min_cbm_bits.

Signed-off-by Aaron Tomlin <[email protected]>

---
  Documentation/filesystems/resctrl.rst     |  10 ++
  arch/x86/kernel/cpu/resctrl/core.c        |   2 +-
  arch/x86/kernel/cpu/resctrl/ctrlmondata.c |  23 ++--
  fs/resctrl/ctrlmondata.c                  | 141 ++++++++++++++++------
  fs/resctrl/internal.h                     |  13 ++
  fs/resctrl/rdtgroup.c                     |   2 +-
  include/linux/resctrl.h                   |  30 ++++-
  7 files changed, 174 insertions(+), 47 deletions(-)

Documentation/filesystems/resctrl.rst +10 -0

index 8c8ce678148a..9c128c8fba86 100644
@@ -215,6 +215,16 @@ related to allocation:
  			# cat /sys/fs/resctrl/info/L3/io_alloc_cbm
  			0=00ff;1=000f
 
+		Set each CBM to their minimum number of consecutive bits.
+
+		A special value "*" is required to represent all cache IDs.
+		The user should specify the correct minimum stored in
+		/sys/fs/resctrl/info/L3/min_cbm_bits.
+
+		Example::
+
+			# echo "*=0" > /sys/fs/resctrl/info/L3/io_alloc_cbm
+
  		When CDP is enabled "io_alloc_cbm" associated with the CDP_DATA and CDP_CODE
  		resources may reflect the same values. For example, values read from and
  		written to /sys/fs/resctrl/info/L3DATA/io_alloc_cbm may be reflected by

arch/x86/kernel/cpu/resctrl/core.c +1 -1

index 3792ab4819dc..44aea6b534e0 100644
@@ -276,7 +276,7 @@ static void rdt_get_cdp_config(int level)
 
  static void rdt_set_io_alloc_capable(struct rdt_resource *r)
  {
-	r->cache.io_alloc_capable = true;
+	r->cache.io_alloc.io_alloc_capable = true;
  }
 
  static void rdt_get_cdp_l3_config(void)

arch/x86/kernel/cpu/resctrl/ctrlmondata.c +14 -9

index b20e705606b8..0f051d848422 100644
@@ -57,14 +57,19 @@ int resctrl_arch_update_domains(struct rdt_resource *r, u32 closid)
  		hw_dom = resctrl_to_arch_ctrl_dom(d);
  		msr_param.res = NULL;
  		for (t = 0; t < CDP_NUM_TYPES; t++) {
-			cfg = &hw_dom->d_resctrl.staged_config[t];
-			if (!cfg->have_new_ctrl)
-				continue;
-
-			idx = resctrl_get_config_index(closid, t);
-			if (cfg->new_ctrl == hw_dom->ctrl_val[idx])
-				continue;
-			hw_dom->ctrl_val[idx] = cfg->new_ctrl;
+			if (resctrl_should_io_alloc_min_cbm(r)) {
+				idx = resctrl_get_config_index(closid, t);
+				hw_dom->ctrl_val[idx] = apply_io_alloc_min_cbm(r);
+			} else {
+				cfg = &hw_dom->d_resctrl.staged_config[t];
+				if (!cfg->have_new_ctrl)
+					continue;
+
+				idx = resctrl_get_config_index(closid, t);
+				if (cfg->new_ctrl == hw_dom->ctrl_val[idx])
+					continue;
+				hw_dom->ctrl_val[idx] = cfg->new_ctrl;
+			}
 
  			if (!msr_param.res) {
  				msr_param.low = idx;

@@ -123,7 +128,7 @@ int resctrl_arch_io_alloc_enable(struct rdt_resource *r, bool enable)
  {
  	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
 
-	if (hw_res->r_resctrl.cache.io_alloc_capable &&
+	if (hw_res->r_resctrl.cache.io_alloc.io_alloc_capable &&
  	    hw_res->sdciae_enabled != enable) {
  		_resctrl_sdciae_enable(r, enable);
  		hw_res->sdciae_enabled = enable;

fs/resctrl/ctrlmondata.c +100 -33

index d2f9a4f2d887..a36b9055a1be 100644
@@ -688,7 +688,7 @@ int resctrl_io_alloc_show(struct kernfs_open_file *of, struct seq_file *seq, voi
 
  	mutex_lock(&rdtgroup_mutex);
 
-	if (r->cache.io_alloc_capable) {
+	if (r->cache.io_alloc.io_alloc_capable) {
  		if (resctrl_arch_get_io_alloc_enabled(r))
  			seq_puts(seq, "enabled\n");
  		else

@@ -712,13 +712,31 @@ static bool resctrl_io_alloc_closid_supported(u32 io_alloc_closid)
  	return io_alloc_closid < closids_supported();
  }
 
+/**
+ * resctrl_sync_cdp_peer - Mirror staged configuration to the CDP peer type
+ *
+ * @s: resctrl schema
+ * @d: resctrl control domain
+ *
+ * The caller must ensure CDP is enabled for the resource and must be
+ * called under the cpu hotplug lock and rdtgroup mutex
+ */
+static void resctrl_sync_cdp_peer(struct resctrl_schema *s, struct rdt_ctrl_domain *d)
+{
+	enum resctrl_conf_type peer_type;
+
+	peer_type = resctrl_peer_type(s->conf_type);
+	memcpy(&d->staged_config[peer_type],
+	       &d->staged_config[s->conf_type],
+	       sizeof(d->staged_config[0]));
+}
+
  /*
   * Initialize io_alloc CLOSID cache resource CBM with all usable (shared
   * and unused) cache portions.
   */
  static int resctrl_io_alloc_init_cbm(struct resctrl_schema *s, u32 closid)
  {
-	enum resctrl_conf_type peer_type;
  	struct rdt_resource *r = s->res;
  	struct rdt_ctrl_domain *d;
  	int ret;

@@ -731,11 +749,8 @@ static int resctrl_io_alloc_init_cbm(struct resctrl_schema *s, u32 closid)
 
  	/* Keep CDP_CODE and CDP_DATA of io_alloc CLOSID's CBM in sync. */
  	if (resctrl_arch_get_cdp_enabled(r->rid)) {
-		peer_type = resctrl_peer_type(s->conf_type);
  		list_for_each_entry(d, &s->res->ctrl_domains, hdr.list)
-			memcpy(&d->staged_config[peer_type],
-			       &d->staged_config[s->conf_type],
-			       sizeof(d->staged_config[0]));
+			resctrl_sync_cdp_peer(s, d);
  	}
 
  	ret = resctrl_arch_update_domains(r, closid);

@@ -903,49 +918,103 @@ int resctrl_io_alloc_cbm_show(struct kernfs_open_file *of, struct seq_file *seq,
  	return ret;
  }
 
+/**
+ * parse_domain_cbm - Parse "domain=cbm" and return either side
+ *
+ * @line_ptr: Pointer to the input string
+ * @out_val: Output from parsed value (either domain ID or CDM)
+ * @which: Selector for which side to parse
+ *
+ * It is assumed that *line_ptr and *out_val are valid.
+ *
+ * Return: 0 on success and advance *line_ptr to point past the
+ * delimiter, EINVAL on parsing error
+ */
+static inline int parse_domain_cbm(char **line_ptr, unsigned long *out_val,
+				   enum resctrl_kv_sel which)
+{
+	char *rhs, *lhs, *tok;
+	unsigned int base;
+
+	rhs = *line_ptr;
+
+	lhs = strsep(&rhs, "=");
+	if (!lhs || !rhs)
+		goto err;
+
+	if (which == RDT_PARSE_DOMAIN) {
+		tok = lhs;
+		base = 10;
+	} else {
+		tok = rhs;
+		base = 16;
+	}
+	tok = strim(tok);
+
+	if (kstrtoul(tok, base, out_val))
+		goto err;
+
+	*line_ptr = rhs;
+	return 0;
+err:
+	rdt_last_cmd_puts("Invalid domain=cbm: missing '=' or non-numeric value\n");
+	return -EINVAL;
+}
+
  static int resctrl_io_alloc_parse_line(char *line,  struct rdt_resource *r,
  				       struct resctrl_schema *s, u32 closid)
  {
-	enum resctrl_conf_type peer_type;
  	struct rdt_parse_data data;
  	struct rdt_ctrl_domain *d;
-	char *dom = NULL, *id;
-	unsigned long dom_id;
+	char *cbm = NULL;
+	unsigned long out_val;
+	int ret;
 
-next:
-	if (!line || line[0] == '\0')
-		return 0;
+	if (line[0] == '*') {
+		ret = parse_domain_cbm(&line, &out_val, RDT_PARSE_CBM);
+		if (ret)
+			return ret;
 
-	dom = strsep(&line, ";");
-	id = strsep(&dom, "=");
-	if (!dom || kstrtoul(id, 10, &dom_id)) {
-		rdt_last_cmd_puts("Missing '=' or non-numeric domain\n");
+		if (out_val == r->cache.min_cbm_bits) {
+			r->cache.io_alloc.io_alloc_min_cbm = true;
+			return 0;
+		}
+		rdt_last_cmd_puts("Invalid io_alloc min CBM\n");
  		return -EINVAL;
  	}
 
-	dom = strim(dom);
-	list_for_each_entry(d, &r->ctrl_domains, hdr.list) {
-		if (d->hdr.id == dom_id) {
-			data.buf = dom;
-			data.mode = RDT_MODE_SHAREABLE;
-			data.closid = closid;
-			if (parse_cbm(&data, s, d))
-				return -EINVAL;
-			/*
-			 * Keep io_alloc CLOSID's CBM of CDP_CODE and CDP_DATA
-			 * in sync.
-			 */
-			if (resctrl_arch_get_cdp_enabled(r->rid)) {
-				peer_type = resctrl_peer_type(s->conf_type);
-				memcpy(&d->staged_config[peer_type],
-				       &d->staged_config[s->conf_type],
-				       sizeof(d->staged_config[0]));
+	while (line && line[0] != '\0') {
+		bool found = false;
+
+		cbm = strsep(&line, ";");
+		ret = parse_domain_cbm(&cbm, &out_val, RDT_PARSE_DOMAIN);
+		if (ret)
+			return ret;
+
+		cbm = strim(cbm);
+		list_for_each_entry(d, &r->ctrl_domains, hdr.list) {
+			if (d->hdr.id == out_val) {
+				data.buf = cbm;
+				data.mode = RDT_MODE_SHAREABLE;
+				data.closid = closid;
+				if (parse_cbm(&data, s, d))
+					return -EINVAL;
+				/*
+				 * Keep io_alloc CLOSID's CBM of CDP_CODE and CDP_DATA
+				 * in sync.
+				 */
+				if (resctrl_arch_get_cdp_enabled(r->rid))
+					resctrl_sync_cdp_peer(s, d);
+				found = true;
+				break;
  			}

+ if (id && *id == '*') {

What do you think?

Quoted 1 line

+ if (id && *id == '*') {

That will accept arbitrary chars after the "*".

	if (id && !strcmp(id, "*"))

Quoted 2 lines

What do you think?

Why is io_alloc special? The same simple change could apply to parse_line() to allow setting all domains in a resource to be set to the same value:

# echo "L3:*=fff" > schemata

# echo "MB:*=50" > schemata

-Tony

Quoted 3 lines

Why is io_alloc special? The same simple change could apply to parse_line() to allow setting all domains in a resource to be set to the same value:

Yes. I am fine with that approach. Treat "*" equivalent for all the resources not just io_alloc.

Quoted 3 lines · Babu

Quoted 3 lines

Why is io_alloc special? The same simple change could apply to parse_line() to allow setting all domains in a resource to be set to the same value:

Hi Tony, Babu,

Quoted 2 lines

Yes. I am fine with that approach. Treat "*" equivalent for all the resources not just io_alloc.

No. While I appreciate the desire for a more uniform interface, I would strongly suggest that this particular change remain applicable only to io_alloc for the time being. If I recall correctly, the architectural requirements for other resources differ enough that the suggested approach may not be universally applicable or appropriate in every case, no?

Quoted 1 line

+ if (id && *id == '*') {

That will accept arbitrary chars after the "*".

	if (id && !strcmp(id, "*"))

Quoted 1 line

What do you think?

Why is io_alloc special? The same simple change could apply to parse_line() to allow setting all domains in a resource to be set to the same value:

# echo "L3:*=fff" > schemata

# echo "MB:*=50" > schemata

If I remember correctly the idea was to limit this feature to io_alloc to avoid needing to deal with L2 asymmetric domains [1].

[1] https://lore.kernel.org/lkml/SJ1PR11MB60833A27A1B8057CDDFB1B2BFCCFA@SJ1PR11MB6083.namprd11.prod.outlook.com/

Quoted 2 lines · Reinette Chatre

If I remember correctly the idea was to limit this feature to io_alloc to avoid needing to deal with L2 asymmetric domains [1].

Hi Reinette,

You are quite right; I am in complete agreement with your assessment. The primary intention behind limiting the scope to io_alloc was indeed to avoid the complexities associated with L2 asymmetric domains.

Are we all in alignment to focus this feature entirely on io_alloc for the time being? If so, I will be pleased to prepare a follow-up series that

reflects this consensus once our wider discussion has concluded.

Quoted 2 lines

[1] https://lore.kernel.org/lkml/SJ1PR11MB60833A27A1B8057CDDFB1B2BFCCFA@SJ1PR11MB6083.namprd11.prod.outlook.com/

Hi Aaron and Tony,

Quoted 4 lines · Aaron Tomlin

Quoted 2 lines · Reinette Chatre

If I remember correctly the idea was to limit this feature to io_alloc to avoid needing to deal with L2 asymmetric domains [1].

Quoted 8 lines

Hi Reinette,

You are quite right; I am in complete agreement with your assessment. The primary intention behind limiting the scope to io_alloc was indeed to avoid the complexities associated with L2 asymmetric domains.

Are we all in alignment to focus this feature entirely on io_alloc for the time being? If so, I will be pleased to prepare a follow-up series that

From my side I am ok to limit this to io_alloc. Of course, this does not prevent cache allocation from supporting this syntax in the future.

Tony: did you perhaps imply with examples in [2] that '*' only be supported by L3 and MB, not L2? Can it be guaranteed that L3 will never be asymmetric? Not that it is a blocker though, as discussed earlier there are ways [3] to support

'*' when domains may be asymmetric. That proposal is only reasonable if considering the feature as "let user set same CBM on all domains" that just happens to support the "reset to min" use case for L3 io_alloc. I assume even on asymmetric domains the min would be the same? If there is indeed a requirement to support "reset to defaults" for general cache allocation then this feature would not work for asymmetric domains as you highlighted in [4].

Although, a "reset to defaults" for cache allocation use case may be better handled by removing and recreating the resource group since the defaults will take into account any exclusive allocations?

Reinette

Quoted 4 lines

reflects this consensus once our wider discussion has concluded.

Quoted 2 lines

[1] https://lore.kernel.org/lkml/SJ1PR11MB60833A27A1B8057CDDFB1B2BFCCFA@SJ1PR11MB6083.namprd11.prod.outlook.com/

[2] https://lore.kernel.org/all/SJ1PR11MB6083CCA2A7904E459B1AA35DFCA8A@SJ1PR11MB6083.namprd11.prod.outlook.com/
[3] https://lore.kernel.org/lkml/[email protected]/
[4] https://lore.kernel.org/lkml/SJ1PR11MB60836AB4270419338FBB4D1EFCCEA@SJ1PR11MB6083.namprd11.prod.outlook.com/

Quoted 16 lines · Reinette Chatre

Hi Aaron and Tony,

Quoted 4 lines · Aaron Tomlin

Quoted 2 lines · Reinette Chatre

If I remember correctly the idea was to limit this feature to io_alloc to avoid needing to deal with L2 asymmetric domains [1].

Quoted 8 lines

Hi Reinette,

You are quite right; I am in complete agreement with your assessment. The primary intention behind limiting the scope to io_alloc was indeed to avoid the complexities associated with L2 asymmetric domains.

Are we all in alignment to focus this feature entirely on io_alloc for the time being? If so, I will be pleased to prepare a follow-up series that

Quoted 6 lines

From my side I am ok to limit this to io_alloc. Of course, this does not prevent cache allocation from supporting this syntax in the future.

Tony: did you perhaps imply with examples in [2] that '*' only be supported by L3 and MB, not L2? Can it be guaranteed that L3 will never be asymmetric? Not that it is a blocker though, as discussed earlier there are ways [3] to support

I'd forgotten the L2 asymmetry issue. If we wanted to enable "*" more generally, then resctrl would have to limit it to symmetric resources or to allow setting values that work for all domains in an asymmetric resource). But that seems more complexity in the kernel for something than can easily be handled in user space. E.g. to reset L3 to ffff

# sed -n -e '/L3:/s/\(=[0-9a-f][0-9a-f]*\)/=ffff/gp' schemata > schemata

Quoted 9 lines

'*' when domains may be asymmetric. That proposal is only reasonable if considering the feature as "let user set same CBM on all domains" that just happens to support the "reset to min" use case for L3 io_alloc. I assume even on asymmetric domains the min would be the same? If there is indeed a requirement to support "reset to defaults" for general cache allocation then this feature would not work for asymmetric domains as you highlighted in [4].

Although, a "reset to defaults" for cache allocation use case may be better handled by removing and recreating the resource group since the defaults will take into account any exclusive allocations?

Removing the directory would free the RMID and allocate a new one when you recreated it. Losing cache occupancy information completely, and disturbing memory bandwidth monitoring. Also leaving the user to hunt down tasks that were reassigned to the default CLOS and reassign them. So too many side effects.

Quoted 11 lines

Reinette

Quoted 4 lines

reflects this consensus once our wider discussion has concluded.

Quoted 2 lines

[1] https://lore.kernel.org/lkml/SJ1PR11MB60833A27A1B8057CDDFB1B2BFCCFA@SJ1PR11MB6083.namprd11.prod.outlook.com/

[2] https://lore.kernel.org/all/SJ1PR11MB6083CCA2A7904E459B1AA35DFCA8A@SJ1PR11MB6083.namprd11.prod.outlook.com/
[3] https://lore.kernel.org/lkml/[email protected]/
[4] https://lore.kernel.org/lkml/SJ1PR11MB60836AB4270419338FBB4D1EFCCEA@SJ1PR11MB6083.namprd11.prod.outlook.com/

-Tony

Hi Tony,

Quoted 30 lines · Tony

Quoted 16 lines · Reinette Chatre

Hi Aaron and Tony,

Quoted 4 lines · Aaron Tomlin

Quoted 2 lines · Reinette Chatre

If I remember correctly the idea was to limit this feature to io_alloc to avoid needing to deal with L2 asymmetric domains [1].

Quoted 8 lines

Hi Reinette,

You are quite right; I am in complete agreement with your assessment. The primary intention behind limiting the scope to io_alloc was indeed to avoid the complexities associated with L2 asymmetric domains.

Are we all in alignment to focus this feature entirely on io_alloc for the time being? If so, I will be pleased to prepare a follow-up series that

Quoted 6 lines

From my side I am ok to limit this to io_alloc. Of course, this does not prevent cache allocation from supporting this syntax in the future.

Tony: did you perhaps imply with examples in [2] that '*' only be supported by L3 and MB, not L2? Can it be guaranteed that L3 will never be asymmetric? Not that it is a blocker though, as discussed earlier there are ways [3] to support

I'd forgotten the L2 asymmetry issue. If we wanted to enable "*" more generally, then resctrl would have to limit it to symmetric resources or to allow setting values that work for all domains in an asymmetric resource). But that seems more complexity in the kernel for something than can easily be handled in user space. E.g. to reset L3 to ffff

Quoted 1 line

# sed -n -e '/L3:/s/\(=[0-9a-f][0-9a-f]*\)/=ffff/gp' schemata > schemata

ack.

Can I interpret this as you being ok to limit support for '*' syntax to io_alloc (for now?)? As I understand the intended implementation discussed here will indeed just allow setting values that will work for all domains ... all or nothing. So, if L3 does become asymmetric along the way then the implementation does seem to remain reasonable.

Quoted 11 lines

Quoted 10 lines

'*' when domains may be asymmetric. That proposal is only reasonable if considering the feature as "let user set same CBM on all domains" that just happens to support the "reset to min" use case for L3 io_alloc. I assume even on asymmetric domains the min would be the same? If there is indeed a requirement to support "reset to defaults" for general cache allocation then this feature would not work for asymmetric domains as you highlighted in [4].

Although, a "reset to defaults" for cache allocation use case may be better handled by removing and recreating the resource group since the defaults will take into account any exclusive allocations?

Quoted 5 lines

Removing the directory would free the RMID and allocate a new one when you recreated it. Losing cache occupancy information completely, and disturbing memory bandwidth monitoring. Also leaving the user to hunt down tasks that were reassigned to the default CLOS and reassign them. So too many side effects.

I agree. Even so, without knowing more details about use case it is difficult to reason about user space expectations here. This is regarding a "reset to defaults" use case that was raised in [4]. Did you raise that concern based on insights into requests from users to support this? I think we would need to create new syntax if resctrl needs to support such use case.

Reinette

Quoted 13 lines

Quoted 11 lines

Reinette

Quoted 4 lines

reflects this consensus once our wider discussion has concluded.

Quoted 2 lines

[1] https://lore.kernel.org/lkml/SJ1PR11MB60833A27A1B8057CDDFB1B2BFCCFA@SJ1PR11MB6083.namprd11.prod.outlook.com/

[2] https://lore.kernel.org/all/SJ1PR11MB6083CCA2A7904E459B1AA35DFCA8A@SJ1PR11MB6083.namprd11.prod.outlook.com/
[3] https://lore.kernel.org/lkml/[email protected]/
[4] https://lore.kernel.org/lkml/SJ1PR11MB60836AB4270419338FBB4D1EFCCEA@SJ1PR11MB6083.namprd11.prod.outlook.com/

-Tony

I still don't see a good reason for the kernel to handle any of this.

Having some resctrl control files accept wildcards while others don't seems like a confusing interface. Is there something I'm missing that makes io_alloc harder for users to handle than L3 or MB in schemata?

Babu's simpler implementation for io_alloc makes me more comfortable with this. But I'm still unconvinced that wildcards must be handled in kernel code.

From my side I am ok to limit this to io_alloc. Of course, this does not prevent cache allocation from supporting this syntax in the future.

Can't we just simplify to this patch without adding new data structures?

fs/resctrl/ctrlmondata.c +1 -0

index f7608121f5e3..d113d8a3ee02 100644
@@ -874,6 +874,7 @@ static int resctrl_io_alloc_parse_line(char *line,
 struct rdt_resource *r,
        struct rdt_ctrl_domain *d;
        char *dom = NULL, *id;
        unsigned long dom_id;
+       bool update_all;
 
 next:

        if (!line || line[0] == '\0')

fs/resctrl/ctrlmondata.c +6 -2

@@ -881,14 +882,18 @@ static int resctrl_io_alloc_parse_line(char *line,
 struct rdt_resource *r,
 
        dom = strsep(&line, ";");
        id = strsep(&dom, "=");
-       if (!dom || kstrtoul(id, 10, &dom_id)) {
+       update_all = false;
+
+       if (id && *id == '*') {
+               update_all = true;
+       } else if (!dom || kstrtoul(id, 10, &dom_id)) {
                rdt_last_cmd_puts("Missing '=' or non-numeric domain\n");
                return -EINVAL;
        }
 
        dom = strim(dom);
        list_for_each_entry(d, &r->ctrl_domains, hdr.list) {
-               if (d->hdr.id == dom_id) {
+               if (update_all || (d->hdr.id == dom_id)) {
                        data.buf = dom;
                        data.mode = RDT_MODE_SHAREABLE;

                        data.closid = closid;