[PATCH v2 0/3] x86/resctrl: Add "*" shorthand to set minimum io_alloc CBM for all domains

[PATCH v2 0/3] x86/resctrl: Add "*" shorthand to set minimum io_alloc CBM for all domains

[Aaron Tomlin]

Hi Babu, Tony, Reinette,

This series was rebased on linus/master since commit 187d0801404f (tag
v6.18-12991-g187d0801404f).

As previously discussed [1], a special domain ID selector "*" has been
introduced 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.

Please let me know your thoughts.

[1]: https://lore.kernel.org/lkml/7e117908-41ae-4f42-8863-1361101c33ab@amd.com/


Changes since v1 [2]:

 - Updated each helper for consistency (Babu Moger)
 - Refactored the loop logic in function resctrl_io_alloc_parse_line()
   to improve readability
 - Added inline keyword to each helper
 - Added inline keyword to function parse_domain_cbm()

[2]: https://lore.kernel.org/lkml/20251126171653.1004321-1-atomlin@atomlin.com/

Aaron Tomlin (3):
  fs/resctrl: Add helpers to check io_alloc support and enabled state
  fs/resctrl: Return -EINVAL for a missing seq_show implementation
  x86/resctrl: Add "*" shorthand to set minimum io_alloc CBM for all
    domains

 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                  | 215 ++++++++++++++++------
 fs/resctrl/internal.h                     |  13 ++
 fs/resctrl/rdtgroup.c                     |   5 +-
 include/linux/resctrl.h                   |  30 ++-
 7 files changed, 230 insertions(+), 68 deletions(-)

-- 
2.51.0

[PATCH v2 1/3] fs/resctrl: Add helpers to check io_alloc support and enabled state

[Aaron Tomlin]

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 <atomlin@atomlin.com>
---
 fs/resctrl/ctrlmondata.c | 74 +++++++++++++++++++++++++++++-----------
 1 file changed, 54 insertions(+), 20 deletions(-)

diff --git a/fs/resctrl/ctrlmondata.c b/fs/resctrl/ctrlmondata.c
index b2d178d3556e..d2f9a4f2d887 100644
--- a/fs/resctrl/ctrlmondata.c
+++ b/fs/resctrl/ctrlmondata.c
@@ -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;
-	}
 
-	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;
-	}
 
 	io_alloc_closid = resctrl_io_alloc_closid(r);
 
-- 
2.51.0

Re: [PATCH v2 1/3] fs/resctrl: Add helpers to check io_alloc support and enabled state

[Reinette Chatre]

Hi Aaron,

On 12/15/25 3:02 PM, Aaron Tomlin wrote:
> 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 <atomlin@atomlin.com>
> ---
>  fs/resctrl/ctrlmondata.c | 74 +++++++++++++++++++++++++++++-----------
>  1 file changed, 54 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/resctrl/ctrlmondata.c b/fs/resctrl/ctrlmondata.c
> index b2d178d3556e..d2f9a4f2d887 100644
> --- a/fs/resctrl/ctrlmondata.c
> +++ b/fs/resctrl/ctrlmondata.c
> @@ -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;
> -	}
>  
> -	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;
> -	}
>  
>  	io_alloc_closid = resctrl_io_alloc_closid(r);
>  

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. 

Reinette

Re: [PATCH v2 1/3] fs/resctrl: Add helpers to check io_alloc support and enabled state

[Aaron Tomlin]

[-- Attachment #1: Type: text/plain, Size: 1326 bytes --]

On Tue, Dec 16, 2025 at 09:01:32PM -0800, Reinette Chatre wrote:
> 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.


Kind regards,
-- 
Aaron Tomlin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 1/3] fs/resctrl: Add helpers to check io_alloc support and enabled state

[Reinette Chatre]

Hi Aaron,

On 12/18/25 3:22 PM, Aaron Tomlin wrote:
> On Tue, Dec 16, 2025 at 09:01:32PM -0800, Reinette Chatre wrote:
>> 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.

You are correct that new features should be layered on a clean foundation. One
problem is that these two patches are independent. This change is misrepresented
as a pre-requisite of the new feature. Presenting it in this way impacts how the
feature itself is considered.

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

Reinette

ps. I will be offline over the holidays and may only be able to continue this discussion
in the next year.

Re: [PATCH v2 1/3] fs/resctrl: Add helpers to check io_alloc support and enabled state

[Aaron Tomlin]

[-- Attachment #1: Type: text/plain, Size: 4065 bytes --]

On Fri, Dec 19, 2025 at 09:05:10AM -0800, Reinette Chatre wrote:
> 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.
> 
Hi Reinette,

Thank you for your exceedingly thorough analysis. You make a most
compelling case, and having reflected upon your points, I find myself in
agreement.

My original aim was to streamline the callsites, yet I concede that the
introduced indirection has come at the expense of transparency.

As you rightly point out, maintaining the visibility of
rdt_last_cmd_clear() alongside the subsequent write is vital for ensuring
that developers do not inadvertently skip the necessary reset of the status
buffer. Furthermore, the loss of visual consistency within functions such
as resctrl_io_alloc_write() - where the interaction with user space becomes
partially obscured - is a valid concern that outweighs the perceived
aesthetic benefit of the cleanup. I also take your point regarding the
preference for lockdep_assert_held() over purely comment - based
documentation; it is a far more robust mechanism for enforcing correctness.

In light of these observations, I shall drop this patch from the series
entirely. I wish you a pleasant and restful break over the festive period,
and I look forward to resuming our technical dialogue in the New Year.


Best regards,
-- 
Aaron Tomlin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v2 2/3] fs/resctrl: Return -EINVAL for a missing seq_show implementation

[Aaron Tomlin]

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.

This patch change the function to return -EINVAL if the file type
handler (i.e., rft->seq_show) is NULL, providing proper feedback that
the operation is invalid for that file.

Signed-off-by: Aaron Tomlin <atomlin@atomlin.com>
---
 fs/resctrl/rdtgroup.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
index 8e39dfda56bc..e1dc63b2218f 100644
--- a/fs/resctrl/rdtgroup.c
+++ b/fs/resctrl/rdtgroup.c
@@ -314,7 +314,8 @@ static int rdtgroup_seqfile_show(struct seq_file *m, void *arg)
 
 	if (rft->seq_show)
 		return rft->seq_show(of, m, arg);
-	return 0;
+
+	return -EINVAL;
 }
 
 static ssize_t rdtgroup_file_write(struct kernfs_open_file *of, char *buf,
-- 
2.51.0

Re: [PATCH v2 2/3] fs/resctrl: Return -EINVAL for a missing seq_show implementation

[Reinette Chatre]

Hi Aaron,

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

On 12/15/25 3:02 PM, Aaron Tomlin wrote:
> 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.

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.

Reinette

Re: [PATCH v2 2/3] fs/resctrl: Return -EINVAL for a missing seq_show implementation

[Aaron Tomlin]

On Tue, Dec 16, 2025 at 09:02:27PM -0800, Reinette Chatre wrote:
> Hi Aaron,

Hi Reinette,

Thank you for your enquiry regarding the necessity of this specific change.

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

To be quite candid, this particular patch does not constitute a strict
prerequisite for the core functionality. The primary feature can, in
technical terms, operate without it.

However, it is important to note that this change was proposed as part of
the broader patchset.

> On 12/15/25 3:02 PM, Aaron Tomlin wrote:
> > 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.
> 
> 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.

To provide some context on my initial reasoning, I found it somewhat
unorthodox to attempt to access an interface that effectively does not
exist (in this case, a missing seq_show callback) without the kernel
returning an explicit error, such as -EINVAL. My instinct was to signal
that the operation was invalid rather than allowing a silent, successful
"empty" read.

However, I take your point entirely. If the intention is that a lack of
data should simply result in a successful return with no output, then the
current behaviour is indeed acceptable.

I shall revert this change from the series.


Kind rgards,
-- 
Aaron Tomlin

[PATCH v2 3/3] x86/resctrl: Add "*" shorthand to set minimum io_alloc CBM for all domains

[Aaron Tomlin]

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 <atomlin@atomlin.com>
---
 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(-)

diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
index 8c8ce678148a..9c128c8fba86 100644
--- a/Documentation/filesystems/resctrl.rst
+++ b/Documentation/filesystems/resctrl.rst
@@ -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
diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
index 3792ab4819dc..44aea6b534e0 100644
--- a/arch/x86/kernel/cpu/resctrl/core.c
+++ b/arch/x86/kernel/cpu/resctrl/core.c
@@ -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)
diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
index b20e705606b8..0f051d848422 100644
--- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
+++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
@@ -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;
diff --git a/fs/resctrl/ctrlmondata.c b/fs/resctrl/ctrlmondata.c
index d2f9a4f2d887..a36b9055a1be 100644
--- a/fs/resctrl/ctrlmondata.c
+++ b/fs/resctrl/ctrlmondata.c
@@ -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;
 			}
-			goto next;
 		}
+
+		if (!found)
+			return -EINVAL;
 	}
 
-	return -EINVAL;
+	return 0;
 }
 
 ssize_t resctrl_io_alloc_cbm_write(struct kernfs_open_file *of, char *buf,
@@ -982,6 +1051,8 @@ ssize_t resctrl_io_alloc_cbm_write(struct kernfs_open_file *of, char *buf,
 		goto out_clear_configs;
 
 	ret = resctrl_arch_update_domains(r, io_alloc_closid);
+	if (resctrl_should_io_alloc_min_cbm(r))
+		r->cache.io_alloc.io_alloc_min_cbm = false;
 
 out_clear_configs:
 	rdt_staged_configs_clear();
diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
index bff4a54ae333..3846737992bb 100644
--- a/fs/resctrl/internal.h
+++ b/fs/resctrl/internal.h
@@ -143,6 +143,19 @@ enum rdt_group_type {
 	RDT_NUM_GROUP,
 };
 
+/**
+ * enum resctrl_kv_sel - Selector for "domain=cbm" key/value parsing
+ * @RDT_PARSE_DOMAIN: Parse the left-hand side (domain ID)
+ * @RDT_PARSE_CBM: Parse the right-hand side (CBM value)
+ *
+ * Used by parsers of key/value pairs formatted as "domain=cbm" to indicate
+ * which token should be extracted and converted.
+ */
+enum resctrl_kv_sel {
+	RDT_PARSE_DOMAIN,
+	RDT_PARSE_CBM,
+};
+
 /**
  * enum rdtgrp_mode - Mode of a RDT resource group
  * @RDT_MODE_SHAREABLE: This resource group allows sharing of its allocations
diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
index e1dc63b2218f..83ce3bafa5cb 100644
--- a/fs/resctrl/rdtgroup.c
+++ b/fs/resctrl/rdtgroup.c
@@ -2196,7 +2196,7 @@ static void io_alloc_init(void)
 {
 	struct rdt_resource *r = resctrl_arch_get_resource(RDT_RESOURCE_L3);
 
-	if (r->cache.io_alloc_capable) {
+	if (r->cache.io_alloc.io_alloc_capable) {
 		resctrl_file_fflags_init("io_alloc", RFTYPE_CTRL_INFO |
 					 RFTYPE_RES_CACHE);
 		resctrl_file_fflags_init("io_alloc_cbm",
diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index 54701668b3df..9e75959565dc 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -215,7 +215,10 @@ struct resctrl_cache {
 	unsigned int	shareable_bits;
 	bool		arch_has_sparse_bitmasks;
 	bool		arch_has_per_cpu_cfg;
-	bool		io_alloc_capable;
+	struct {
+		bool io_alloc_capable;
+		bool io_alloc_min_cbm;
+	} io_alloc;
 };
 
 /**
@@ -415,6 +418,31 @@ static inline bool resctrl_is_mbm_event(enum resctrl_event_id eventid)
 		eventid <= QOS_L3_MBM_LOCAL_EVENT_ID);
 }
 
+/**
+ * apply_io_alloc_min_cbm() - Apply minimum io_alloc CBM
+ *
+ * @r: resctrl resource
+ *
+ * Return: Minimum number of consecutive io_alloc CBM bits to be set.
+ */
+static inline u32 apply_io_alloc_min_cbm(struct rdt_resource *r)
+{
+	return r->cache.min_cbm_bits;
+}
+
+/**
+ * resctrl_should_io_alloc_min_cbm() - Should the minimum io_alloc
+ *				       CBM be applied
+ * @r: resctrl resource
+ *
+ * Return: True if the minimum number of consecutive
+ * bits to be set in the io_alloc CBM should be applied.
+ */
+static inline bool resctrl_should_io_alloc_min_cbm(struct rdt_resource *r)
+{
+	return r->cache.io_alloc.io_alloc_min_cbm;
+}
+
 u32 resctrl_get_mon_evt_cfg(enum resctrl_event_id eventid);
 
 /* Iterate over all memory bandwidth events */
-- 
2.51.0

Re: [PATCH v2 3/3] x86/resctrl: Add "*" shorthand to set minimum io_alloc CBM for all domains

[Reinette Chatre]

Hi Aaron,

On 12/15/25 3:02 PM, Aaron Tomlin wrote:
> This patch introduces a special domain ID selector "*" for io_alloc_cbm

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.

> 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

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?

> /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 <atomlin@atomlin.com>
> ---
>  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(-)
> 
> diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
> index 8c8ce678148a..9c128c8fba86 100644
> --- a/Documentation/filesystems/resctrl.rst
> +++ b/Documentation/filesystems/resctrl.rst
> @@ -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
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 3792ab4819dc..44aea6b534e0 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -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)
> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> index b20e705606b8..0f051d848422 100644
> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> @@ -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;
> +			}
>  

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

>  			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;
> diff --git a/fs/resctrl/ctrlmondata.c b/fs/resctrl/ctrlmondata.c
> index d2f9a4f2d887..a36b9055a1be 100644
> --- a/fs/resctrl/ctrlmondata.c
> +++ b/fs/resctrl/ctrlmondata.c
> @@ -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;
> +}

Why is this new parser needed? 

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

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.


> +			r->cache.io_alloc.io_alloc_min_cbm = true;
> +			return 0;

If I understand correctly the parsing discards user provided value. If this check thinks it needs to
program the min CBM it appears to program the min CBM via a backdoor in the config write helper that
circumvents the flow used by everything else in resctrl that first stages the values and then calls
the config helper to write to hardware. This is not acceptable. Please integrate any new feature into
existing flows that follow existing patterns. Punching a feature specific back door via this new state
(io_alloc_min_cbm) is not appropriate.

> +		}
> +		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;
>  			}
> -			goto next;
>  		}
> +
> +		if (!found)
> +			return -EINVAL;
>  	}
>  
> -	return -EINVAL;
> +	return 0;
>  }

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.

Reinette

[1] https://lore.kernel.org/lkml/239e3a89-1075-439b-bf1c-d2be5be5c30c@amd.com/
[2] https://lore.kernel.org/lkml/5xlapgkp5bktan7xhy6l6b7c4qgeje7weu4cy6cbuux5npwijo@lhf5uvtuns5k/

Re: [PATCH v2 3/3] x86/resctrl: Add "*" shorthand to set minimum io_alloc CBM for all domains

[Aaron Tomlin]

[-- Attachment #1: Type: text/plain, Size: 4136 bytes --]

On Tue, Dec 16, 2025 at 09:53:20PM -0800, Reinette Chatre wrote:
Hi Reinette,

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

Thank you for the guidance regarding the kernel style and submission requirements.

While I do have experience in kernel development, I appreciate the reminder
that strict adherence to the documented process is paramount. I will
carefully re-read the specified sections to ensure full compliance with the
expected standards.

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

You are quite right to raise this point.

Indeed, supporting arbitrary identical CBM values across domains was the
original intent of this work. I appreciate the suggestion and fully agree
that the implementation should be expanded to allow for this additional
flexibility, rather than being restricted to the minimum CBM value.

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

Acknowledged.

> Why is this new parser needed? 

It can be dropped. Kindly ignore. 

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

You are quite right to point this out; I appreciate the thorough review of
the validation logic. My original intention with this implementation was to
provide a mechanism to "reset" the configuration to the minimum supported
CBM only. However, the restriction itself is unnecessary.

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

Acknowledged.

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

Acknowledged.

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

Understood.


Kind regards,
-- 
Aaron Tomlin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/3] x86/resctrl: Add "*" shorthand to set minimum io_alloc CBM for all domains

[Moger, Babu]

Hi Aaron,

On 12/15/2025 5:02 PM, Aaron Tomlin wrote:
> 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 <atomlin@atomlin.com>
> ---
>   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(-)
> 
> diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
> index 8c8ce678148a..9c128c8fba86 100644
> --- a/Documentation/filesystems/resctrl.rst
> +++ b/Documentation/filesystems/resctrl.rst
> @@ -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
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 3792ab4819dc..44aea6b534e0 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -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)
> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> index b20e705606b8..0f051d848422 100644
> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c
> @@ -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;
> diff --git a/fs/resctrl/ctrlmondata.c b/fs/resctrl/ctrlmondata.c
> index d2f9a4f2d887..a36b9055a1be 100644
> --- a/fs/resctrl/ctrlmondata.c
> +++ b/fs/resctrl/ctrlmondata.c
> @@ -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;
>   			}
> -			goto next;
>   		}
> +
> +		if (!found)
> +			return -EINVAL;
>   	}
>   
> -	return -EINVAL;
> +	return 0;
>   }

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

diff --git a/fs/resctrl/ctrlmondata.c b/fs/resctrl/ctrlmondata.c
index f7608121f5e3..d113d8a3ee02 100644
--- a/fs/resctrl/ctrlmondata.c
+++ b/fs/resctrl/ctrlmondata.c
@@ -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')
@@ -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;


What do you think?

thanks
Babu

RE: [PATCH v2 3/3] x86/resctrl: Add "*" shorthand to set minimum io_alloc CBM for all domains

[Luck, Tony]

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

That will accept arbitrary chars after the "*".

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

> 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

Re: RE: [PATCH v2 3/3] x86/resctrl: Add "*" shorthand to set minimum io_alloc CBM for all domains

[Moger, Babu]

On 12/18/2025 3:49 PM, Luck, Tony wrote:
>> +       if (id && *id == '*') {
> 
> That will accept arbitrary chars after the "*".
> 
> 	if (id && !strcmp(id, "*"))

oh.ok. Thanks

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

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

> # echo "L3:*=fff" > schemata
> 
> # echo "MB:*=50" > schemata
> 
> -Tony
> 
> 

Re: [PATCH v2 3/3] x86/resctrl: Add "*" shorthand to set minimum io_alloc CBM for all domains

[Aaron Tomlin]

[-- Attachment #1: Type: text/plain, Size: 749 bytes --]

On Thu, Dec 18, 2025 at 03:56:24PM -0600, Moger, Babu wrote:
> > 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,

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


Kind regards,
-- 
Aaron Tomlin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/3] x86/resctrl: Add "*" shorthand to set minimum io_alloc CBM for all domains

[Moger, Babu]

Hi Aaron,

On 12/18/2025 6:31 PM, Aaron Tomlin wrote:
> On Thu, Dec 18, 2025 at 03:56:24PM -0600, Moger, Babu wrote:
>>> 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,
> 
>> 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?
> 

Yes. That is correct. Lets do this only for io_alloc.

Reinette commented on the already.

https://lore.kernel.org/lkml/a76c2eee-2a64-42ab-b81c-90638321cd15@intel.com/

Thanks
Babu

Re: [PATCH v2 3/3] x86/resctrl: Add "*" shorthand to set minimum io_alloc CBM for all domains

[Reinette Chatre]

Hi Tony,

On 12/18/25 1:49 PM, Luck, Tony wrote:
>> +       if (id && *id == '*') {
> 
> That will accept arbitrary chars after the "*".
> 
> 	if (id && !strcmp(id, "*"))
> 
>> 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].

Reinette

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

Re: [PATCH v2 3/3] x86/resctrl: Add "*" shorthand to set minimum io_alloc CBM for all domains

[Aaron Tomlin]

[-- Attachment #1: Type: text/plain, Size: 806 bytes --]

On Thu, Dec 18, 2025 at 02:59:59PM -0800, Reinette Chatre wrote:
> 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.

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

Kind regards,
-- 
Aaron Tomlin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/3] x86/resctrl: Add "*" shorthand to set minimum io_alloc CBM for all domains

[Reinette Chatre]

Hi Aaron and Tony,

On 12/18/25 4:44 PM, Aaron Tomlin wrote:
> On Thu, Dec 18, 2025 at 02:59:59PM -0800, Reinette Chatre wrote:
>> 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

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


> reflects this consensus once our wider discussion has concluded.
> 
>>
>> [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/f4a043d2-9cb0-41c9-a45d-31f96fd007d5@amd.com/
[4] https://lore.kernel.org/lkml/SJ1PR11MB60836AB4270419338FBB4D1EFCCEA@SJ1PR11MB6083.namprd11.prod.outlook.com/

Re: [PATCH v2 3/3] x86/resctrl: Add "*" shorthand to set minimum io_alloc CBM for all domains

[Luck, Tony]

On Fri, Dec 19, 2025 at 10:28:32AM -0800, Reinette Chatre wrote:
> Hi Aaron and Tony,
> 
> On 12/18/25 4:44 PM, Aaron Tomlin wrote:
> > On Thu, Dec 18, 2025 at 02:59:59PM -0800, Reinette Chatre wrote:
> >> 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
> 
> 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

> '*' 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.
> 
> Reinette
> 
> 
> > reflects this consensus once our wider discussion has concluded.
> > 
> >>
> >> [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/f4a043d2-9cb0-41c9-a45d-31f96fd007d5@amd.com/
> [4] https://lore.kernel.org/lkml/SJ1PR11MB60836AB4270419338FBB4D1EFCCEA@SJ1PR11MB6083.namprd11.prod.outlook.com/

-Tony

Re: [PATCH v2 3/3] x86/resctrl: Add "*" shorthand to set minimum io_alloc CBM for all domains

[Reinette Chatre]

Hi Tony,

On 12/19/25 12:42 PM, Luck, Tony wrote:
> On Fri, Dec 19, 2025 at 10:28:32AM -0800, Reinette Chatre wrote:
>> Hi Aaron and Tony,
>>
>> On 12/18/25 4:44 PM, Aaron Tomlin wrote:
>>> On Thu, Dec 18, 2025 at 02:59:59PM -0800, Reinette Chatre wrote:
>>>> 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
>>
>> 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

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. 


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

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


>>
>> Reinette
>>
>>
>>> reflects this consensus once our wider discussion has concluded.
>>>
>>>>
>>>> [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/f4a043d2-9cb0-41c9-a45d-31f96fd007d5@amd.com/
>> [4] https://lore.kernel.org/lkml/SJ1PR11MB60836AB4270419338FBB4D1EFCCEA@SJ1PR11MB6083.namprd11.prod.outlook.com/
> 
> -Tony

Re: [PATCH v2 3/3] x86/resctrl: Add "*" shorthand to set minimum io_alloc CBM for all domains

[Luck, Tony]

On Fri, Dec 19, 2025 at 02:00:32PM -0800, Reinette Chatre wrote:
> Hi Tony,
> 
> On 12/19/25 12:42 PM, Luck, Tony wrote:
> > On Fri, Dec 19, 2025 at 10:28:32AM -0800, Reinette Chatre wrote:
> >> Hi Aaron and Tony,
> >>
> >> On 12/18/25 4:44 PM, Aaron Tomlin wrote:
> >>> On Thu, Dec 18, 2025 at 02:59:59PM -0800, Reinette Chatre wrote:
> >>>> 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
> >>
> >> 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
> 
> 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. 

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.

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

I don't have any requests from users.

> 
> Reinette
> 
> 
> >>
> >> Reinette
> >>
> >>
> >>> reflects this consensus once our wider discussion has concluded.
> >>>
> >>>>
> >>>> [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/f4a043d2-9cb0-41c9-a45d-31f96fd007d5@amd.com/
> >> [4] https://lore.kernel.org/lkml/SJ1PR11MB60836AB4270419338FBB4D1EFCCEA@SJ1PR11MB6083.namprd11.prod.outlook.com/
> > 
> > -Tony
> 

-Tony

Re: [PATCH v2 3/3] x86/resctrl: Add "*" shorthand to set minimum io_alloc CBM for all domains

[Aaron Tomlin]

[-- Attachment #1: Type: text/plain, Size: 1070 bytes --]

On Fri, Dec 19, 2025 at 03:05:34PM -0800, Luck, Tony wrote:
> 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.

Hi Tony,

I appreciate your reservations regarding where the responsibility for such
abstractions should lie. However, I am of the view that forcing the user to
manually specify every domain for a global policy represents an unnecessary
hurdle that the kernel is uniquely positioned to clear.

I shall indeed employ Babu's suggestion in a follow-up patch for review, as
the resulting simplicity of the implementation should, I hope, alleviate
any concerns regarding the addition of technical debt.


Kind regards,
-- 
Aaron Tomlin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/3] x86/resctrl: Add "*" shorthand to set minimum io_alloc CBM for all domains

[Aaron Tomlin]

[-- Attachment #1: Type: text/plain, Size: 456 bytes --]

On Fri, Dec 19, 2025 at 10:28:32AM -0800, Reinette Chatre wrote:
> 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.

Hi Reinette,

Understood.

I have taken the feedback on board and shall prepare a follow-up patch
that reflects this narrower focus, alongside the removal of the
aforementioned refactoring patch.


Kind regards,
-- 
Aaron Tomlin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 3/3] x86/resctrl: Add "*" shorthand to set minimum io_alloc CBM for all domains

[Aaron Tomlin]

[-- Attachment #1: Type: text/plain, Size: 1733 bytes --]

On Thu, Dec 18, 2025 at 03:32:56PM -0600, Moger, Babu wrote:
> Can't we just simplify to this patch without adding new data structures?

Hi Babu,

Indeed, we certainly can.

> diff --git a/fs/resctrl/ctrlmondata.c b/fs/resctrl/ctrlmondata.c
> index f7608121f5e3..d113d8a3ee02 100644
> --- a/fs/resctrl/ctrlmondata.c
> +++ b/fs/resctrl/ctrlmondata.c
> @@ -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')
> @@ -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;

The above is suitable. I will prepare a follow-up series once our wider
discussion has concluded and we have reached a consensus on the
architectural direction.


Kind regards,
-- 
Aaron Tomlin

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]