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

The last_cmd_status file is intended to report details about the most recent resctrl filesystem operation, specifically to aid in diagnosing failures.

However, when parsing io_alloc_cbm, if a user provides a domain ID that does not exist in the resource, the operation fails with -EINVAL without updating last_cmd_status. This results in inconsistent behaviour where the system call reports an error, but the status file may misleadingly

report "ok" (retained from a previous successful operation), leaving the

user unaware that the failure was caused by an invalid domain ID.

Update resctrl_io_alloc_parse_line() to explicitly write an error message to last_cmd_status when the target domain ID cannot be found.

Suggested-by Reinette Chatre <[email protected]>

Fixes 28fa2cce7a83 ("fs/resctrl: Introduce interface to modify io_alloc capacity bitmasks")

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

---
 fs/resctrl/ctrlmondata.c | 1 +
 1 file changed, 1 insertion(+)

fs/resctrl/ctrlmondata.c +0 -0

index b2d178d3556e..b96b661626c2 100644

Thank you very much for adding the fix. The actual change looks good to me, just a couple of nits on the changelog.

Currently, configuring the io_alloc_cbm interface requires an explicit

domain ID for each cache domain. On systems with high core counts and numerous cache clusters, this requirement becomes cumbersome for automation and management tasks that aim to apply a uniform policy.

Introduce a wildcard domain ID selector "*" for the io_alloc_cbm interface. This enables users to update the Capacity Bitmask (CBM)

across all cache domains in a single operation.

For example, a user can write "*=0" to the io_alloc_cbm file to

programme every domain with the same mask. The value supplied must,

however, remain within the valid range defined by the resource (e.g., min_cbm_bits).

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

---
 Documentation/filesystems/resctrl.rst | 10 ++++++++++
 fs/resctrl/ctrlmondata.c              | 15 ++++++++++++---
 2 files changed, 22 insertions(+), 3 deletions(-)

Documentation/filesystems/resctrl.rst +0 -0

index 8c8ce678148a..948219e58882 100644

Documentation/filesystems/resctrl.rst +5 -0

+
+			# echo "*=0" > /sys/fs/resctrl/info/L3/io_alloc_cbm
+			# cat /sys/fs/resctrl/info/L3/io_alloc_cbm
+			0=0;1=0
+
 		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

fs/resctrl/ctrlmondata.c +4 -1

index b96b661626c2..f47331a97337 100644
@@ -873,21 +873,26 @@ 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')
 		return 0;
 
+	update_all = false;
 	dom = strsep(&line, ";");
 	id = strsep(&dom, "=");
-	if (!dom || kstrtoul(id, 10, &dom_id)) {
+
+	if (dom && !strcmp(id, "*")) {

fs/resctrl/ctrlmondata.c +0 -0

 			data.buf = dom;
 			data.mode = RDT_MODE_SHAREABLE;
 			data.closid = closid;

@@ -903,10 +908,14 @@ static int resctrl_io_alloc_parse_line(char *line,  struct rdt_resource *r,
 				       &d->staged_config[s->conf_type],

Hi Aaron,

Quoted 1 line

Currently, configuring the io_alloc_cbm interface requires an explicit

(nit: "Currently" can be dropped. Expectation is that the patch starts with context that is by default the current behavior.)

Quoted 7 lines

domain ID for each cache domain. On systems with high core counts and numerous cache clusters, this requirement becomes cumbersome for automation and management tasks that aim to apply a uniform policy.

Introduce a wildcard domain ID selector "*" for the io_alloc_cbm interface. This enables users to update the Capacity Bitmask (CBM)

"update the" -> "set the same"?

Quoted 3 lines

across all cache domains in a single operation.

For example, a user can write "*=0" to the io_alloc_cbm file to

This example seems redundant. The description is clear and the documentation addition found in the patch describes this scenario anyway.

Quoted 1 line

programme every domain with the same mask. The value supplied must,

"programme" -> "program"

Quoted 17 lines

however, remain within the valid range defined by the resource (e.g., min_cbm_bits).

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

---
 Documentation/filesystems/resctrl.rst | 10 ++++++++++
 fs/resctrl/ctrlmondata.c              | 15 ++++++++++++---
 2 files changed, 22 insertions(+), 3 deletions(-)

Documentation/filesystems/resctrl.rst +0 -0

index 8c8ce678148a..948219e58882 100644
@@ -215,6 +215,16 @@ related to allocation:
 			# cat /sys/fs/resctrl/info/L3/io_alloc_cbm
 			0=00ff;1=000f
 

Documentation/filesystems/resctrl.rst related to allocation: · line 218

Quoted 1 line

Documentation/filesystems/resctrl.rst +1 -0

+		Set each CBM to a specified value.

Above sentence seems redundant when compared to below sentence that is more specific. Can it just be dropped?

Documentation/filesystems/resctrl.rst related to allocation: · line 222

Quoted 4 lines

Documentation/filesystems/resctrl.rst +4 -0

+
+		An ID of "*" configures all domains with the provided CBM.
+
+		Example::

Just to make this clear I suggest to add caveat:
"Example on a system that does not require a minimum number of consecutive bits in the mask::"

Quoted 34 lines

Documentation/filesystems/resctrl.rst +5 -0

+
+			# echo "*=0" > /sys/fs/resctrl/info/L3/io_alloc_cbm
+			# cat /sys/fs/resctrl/info/L3/io_alloc_cbm
+			0=0;1=0
+
 		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

fs/resctrl/ctrlmondata.c +6 -1

index b96b661626c2..f47331a97337 100644
@@ -873,21 +873,26 @@ 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')
 		return 0;
 
+	update_all = false;
 	dom = strsep(&line, ";");
 	id = strsep(&dom, "=");
-	if (!dom || kstrtoul(id, 10, &dom_id)) {
+
+	if (dom && !strcmp(id, "*")) {
+		update_all = true;
+	} else if (!dom || kstrtoul(id, 10, &dom_id)) {
 		rdt_last_cmd_puts("Missing '=' or non-numeric domain\n");
 		return -EINVAL;
 	}
 

fs/resctrl/ctrlmondata.c static int resctrl_io_alloc_parse_line(char *line, struct rdt_resource *r, · line 895

Quoted 4 lines

fs/resctrl/ctrlmondata.c +1 -1

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

While this code is correct in that dom_id will not be accessed when update_all is true it makes the code more difficult to understand when an uninitialized variable depends on certain flows.

Quoted 5 lines

fs/resctrl/ctrlmondata.c +0 -0

 			data.buf = dom;
 			data.mode = RDT_MODE_SHAREABLE;
 			data.closid = closid;

@@ -903,10 +908,14 @@ static int resctrl_io_alloc_parse_line(char *line,  struct rdt_resource *r,
 				       &d->staged_config[s->conf_type],

fs/resctrl/ctrlmondata.c static int resctrl_io_alloc_parse_line(char *line, struct rdt_resource *r, · line 917

Quoted 10 lines

fs/resctrl/ctrlmondata.c +4 -1

 				       sizeof(d->staged_config[0]));
 			}
-			goto next;
+			if (!update_all)
+				goto next;
 		}
 	}
 
+	if (update_all)
+		goto next;

One comment from v4 appears to be unaddressed. Copied here for convenience:
I see that this aims to support input like "*=f;*=0" but I do not see how something like this can ever succeed since parse_cbm() stages the config and should fail if any domain already has a config. Should this perhaps just return success here? This could be made more robust by only returning success if there is no more text to parse, thus failing on input like "*=f;1=f".

Below is a sample fixup that addresses the comments. What do you think?

fs/resctrl/ctrlmondata.c +7 -3

index 869e37f7d22a..9a7dfc48cb2e 100644
@@ -954,17 +954,21 @@ 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;
+	unsigned long dom_id = ULONG_MAX;
 	struct rdt_parse_data data;
 	struct rdt_ctrl_domain *d;
+	bool update_all = false;
 	char *dom = NULL, *id;
-	unsigned long dom_id;
-	bool update_all;
 
 next:
 	if (!line || line[0] == '\0')
 		return 0;
 
-	update_all = false;
+	if (update_all) {
+		rdt_last_cmd_puts("Configurations after global '*'\n");
+		return -EINVAL;
+	}
+
 	dom = strsep(&line, ";");
 	id = strsep(&dom, "=");
 

fs/resctrl/ctrlmondata.c static int resctrl_io_alloc_parse_line(char *line, struct rdt_resource *r, · line 921

Quoted 4 lines

fs/resctrl/ctrlmondata.c +1 -0

+
 	rdt_last_cmd_printf("Invalid domain %lu\n", dom_id);
 	return -EINVAL;
 }