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

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

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

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.

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