[v2] fs/resctrl: Add missing kconfig entry for CONFIG_RESCTRL_ASSIGN_FIXED

[PATCH v2] fs/resctrl: Add missing kconfig entry for CONFIG_RESCTRL_ASSIGN_FIXED

Posted by Ben Horgan 1 week, 2 days ago

The commit 3b497c3f4f04 ("fs/resctrl: Introduce the interface to display
monitoring modes") introduced CONFIG_RESCTRL_ASSIGN_FIXED but left adding
the kconfig entry until it was necessary. Add the kconfig entry as it is
now necessary in order to support ABMC on MPAM where the counter assignment
mode is indeed fixed.

Also, take the opportunity ensure that a user attempt to change between
different counter assignment mode fails from the resctrl code rather than
delegating to the arch specific code and let the user know by adding a
message in last_cmd_status.

Signed-off-by: Ben Horgan <ben.horgan@arm.com>
---
Changes since v1:
Update the commit message to make it clear this is an anticipated follow on
patch rather than a fix.
Only fail attempts to change to a different counter assignment mode.
Kconfig indenting.
Use "counter assignment mode" text throughout.
---
 fs/resctrl/Kconfig   | 9 +++++++++
 fs/resctrl/monitor.c | 6 ++++++
 2 files changed, 15 insertions(+)

diff --git a/fs/resctrl/Kconfig b/fs/resctrl/Kconfig
index 21671301bd8a..d833dea81aea 100644
--- a/fs/resctrl/Kconfig
+++ b/fs/resctrl/Kconfig
@@ -37,3 +37,12 @@ config RESCTRL_RMID_DEPENDS_ON_CLOSID
 	  Enabled by the architecture when the RMID values depend on the CLOSID.
 	  This causes the CLOSID allocator to search for CLOSID with clean
 	  RMID.
+
+config RESCTRL_ASSIGN_FIXED
+	bool
+	depends on RESCTRL_FS
+	help
+	  Enabled by the architecture when the counter assignment mode is not
+	  configurable. This ensures that counter assignment mode is not
+	  advertised as configurable and attempts to change counter assignment
+	  mode fail.
diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
index 572a9925bd6c..93756be50062 100644
--- a/fs/resctrl/monitor.c
+++ b/fs/resctrl/monitor.c
@@ -1451,6 +1451,12 @@ ssize_t resctrl_mbm_assign_mode_write(struct kernfs_open_file *of, char *buf,
 	}
 
 	if (enable != resctrl_arch_mbm_cntr_assign_enabled(r)) {
+		if (IS_ENABLED(CONFIG_RESCTRL_ASSIGN_FIXED)) {
+			ret = -EINVAL;
+			rdt_last_cmd_puts("counter assignment mode is not configurable\n");
+			goto out_unlock;
+		}
+
 		ret = resctrl_arch_mbm_cntr_assign_set(r, enable);
 		if (ret)
 			goto out_unlock;
-- 
2.43.0

Re: [PATCH v2] fs/resctrl: Add missing kconfig entry for CONFIG_RESCTRL_ASSIGN_FIXED

Posted by Reinette Chatre 4 days, 14 hours ago

Hi Ben,

I just have a couple of comments regarding style.

Subject can be shortened by removing redundant words:
	fs/resctrl: Add CONFIG_RESCTRL_ASSIGN_FIXED Kconfig entry

Please note the kconfig -> Kconfig change for rest of changelog also.

On 1/28/26 8:12 AM, Ben Horgan wrote:
> The commit 3b497c3f4f04 ("fs/resctrl: Introduce the interface to display
> monitoring modes") introduced CONFIG_RESCTRL_ASSIGN_FIXED but left adding

In x86 area there is a custom how to make references to a commit stand out
in the changelog. Please see commit a121798ae669 ("x86/resctrl: Fix allocation
of cleanest CLOSID on platforms with no monitors") for an example.

> the kconfig entry until it was necessary. Add the kconfig entry as it is

Note the kconfig -> Kconfig and please split the context from the solution (see
"Changelog" in Documentation/process/maintainer-tip.rst for reference).

> now necessary in order to support ABMC on MPAM where the counter assignment

ABMC is name of AMD's feature. MPAM need not adopt AMD's feature name. On resctrl
fs side it is just a generic "counter assignment" or "mbm_event" mode.

> mode is indeed fixed.
> 
> Also, take the opportunity ensure that a user attempt to change between
> different counter assignment mode fails from the resctrl code rather than
> delegating to the arch specific code and let the user know by adding a
> message in last_cmd_status.

This can be connected to the change to not make it seem like an afterthought nor
contain trigger word ("Also, ...") that hints this is a logical change that
belongs in separate patch. Finally, the "let the user know ..." is not necessary
since it can be seen from the patch self.

Here is an example that addresses above notes but please consider it critically
and do not just copy&paste without reading and considering corrections and improvement:

  Commit

    3b497c3f4f04 ("fs/resctrl: Introduce the interface to display monitoring modes")

  introduced CONFIG_RESCTRL_ASSIGN_FIXED but left adding the Kconfig entry until it
  was necessary.

  Add CONFIG_RESCTRL_ASSIGN_FIXED in order to support MPAM where the counter assignment
  mode is indeed fixed.                 

  CONFIG_RESCTRL_ASSIGN_FIXED is a resctrl fs Kconfig option so handle attempts   
  to modify counter assignment mode when it is enabled in resctrl fs rather than  
  delegating to arch specific code.

> 
> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
> ---
> Changes since v1:
> Update the commit message to make it clear this is an anticipated follow on
> patch rather than a fix.
> Only fail attempts to change to a different counter assignment mode.
> Kconfig indenting.
> Use "counter assignment mode" text throughout.
> ---
>  fs/resctrl/Kconfig   | 9 +++++++++
>  fs/resctrl/monitor.c | 6 ++++++
>  2 files changed, 15 insertions(+)
> 
> diff --git a/fs/resctrl/Kconfig b/fs/resctrl/Kconfig
> index 21671301bd8a..d833dea81aea 100644
> --- a/fs/resctrl/Kconfig
> +++ b/fs/resctrl/Kconfig
> @@ -37,3 +37,12 @@ config RESCTRL_RMID_DEPENDS_ON_CLOSID
>  	  Enabled by the architecture when the RMID values depend on the CLOSID.
>  	  This causes the CLOSID allocator to search for CLOSID with clean
>  	  RMID.
> +
> +config RESCTRL_ASSIGN_FIXED
> +	bool
> +	depends on RESCTRL_FS
> +	help
> +	  Enabled by the architecture when the counter assignment mode is not
> +	  configurable. This ensures that counter assignment mode is not
> +	  advertised as configurable and attempts to change counter assignment
> +	  mode fail.
> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> index 572a9925bd6c..93756be50062 100644
> --- a/fs/resctrl/monitor.c
> +++ b/fs/resctrl/monitor.c
> @@ -1451,6 +1451,12 @@ ssize_t resctrl_mbm_assign_mode_write(struct kernfs_open_file *of, char *buf,
>  	}
>  
>  	if (enable != resctrl_arch_mbm_cntr_assign_enabled(r)) {
> +		if (IS_ENABLED(CONFIG_RESCTRL_ASSIGN_FIXED)) {
> +			ret = -EINVAL;
> +			rdt_last_cmd_puts("counter assignment mode is not configurable\n");

Please start output to user space with upper case (unless it is a term that is usually written with
lower case). You can use "grep rdt_last_cmd_puts fs/resctrl/*" to get an idea if the patterns used.

> +			goto out_unlock;
> +		}
> +
>  		ret = resctrl_arch_mbm_cntr_assign_set(r, enable);
>  		if (ret)
>  			goto out_unlock;

Reinette

Re: [PATCH v2] fs/resctrl: Add missing kconfig entry for CONFIG_RESCTRL_ASSIGN_FIXED

Posted by Ben Horgan 3 days, 1 hour ago

Hi Reinette,

On 2/2/26 21:50, Reinette Chatre wrote:
> Hi Ben,
> 
> I just have a couple of comments regarding style.
> 
> Subject can be shortened by removing redundant words:
> 	fs/resctrl: Add CONFIG_RESCTRL_ASSIGN_FIXED Kconfig entry

Ok, I'll update to this.

> 
> Please note the kconfig -> Kconfig change for rest of changelog also.
> 
> On 1/28/26 8:12 AM, Ben Horgan wrote:
>> The commit 3b497c3f4f04 ("fs/resctrl: Introduce the interface to display
>> monitoring modes") introduced CONFIG_RESCTRL_ASSIGN_FIXED but left adding
> 
> In x86 area there is a custom how to make references to a commit stand out
> in the changelog. Please see commit a121798ae669 ("x86/resctrl: Fix allocation
> of cleanest CLOSID on platforms with no monitors") for an example.
> 
>> the kconfig entry until it was necessary. Add the kconfig entry as it is
> 
> Note the kconfig -> Kconfig and please split the context from the solution (see
> "Changelog" in Documentation/process/maintainer-tip.rst for reference).
> 
>> now necessary in order to support ABMC on MPAM where the counter assignment
> 
> ABMC is name of AMD's feature. MPAM need not adopt AMD's feature name. On resctrl
> fs side it is just a generic "counter assignment" or "mbm_event" mode.
> 
>> mode is indeed fixed.
>>
>> Also, take the opportunity ensure that a user attempt to change between
>> different counter assignment mode fails from the resctrl code rather than
>> delegating to the arch specific code and let the user know by adding a
>> message in last_cmd_status.
> 
> This can be connected to the change to not make it seem like an afterthought nor
> contain trigger word ("Also, ...") that hints this is a logical change that
> belongs in separate patch. Finally, the "let the user know ..." is not necessary
> since it can be seen from the patch self.
> > Here is an example that addresses above notes but please consider it
critically
> and do not just copy&paste without reading and considering corrections and improvement:
> 
> 
>   Commit
> 
>     3b497c3f4f04 ("fs/resctrl: Introduce the interface to display monitoring modes")
> 
>   introduced CONFIG_RESCTRL_ASSIGN_FIXED but left adding the Kconfig entry until it
>   was necessary.
> 
>   Add CONFIG_RESCTRL_ASSIGN_FIXED in order to support MPAM where the counter assignment
>   mode is indeed fixed.                 
>                                                                                 
>   CONFIG_RESCTRL_ASSIGN_FIXED is a resctrl fs Kconfig option so handle attempts   
>   to modify counter assignment mode when it is enabled in resctrl fs rather than  
>   delegating to arch specific code.

Based on your comments and having a look at maintainer-tip.rst this
seems like a good commit message that matches the commit. So, I'll go
with it. Thanks for suggesting it.

> 
>                                       
>>
>> Signed-off-by: Ben Horgan <ben.horgan@arm.com>
>> ---
>> Changes since v1:
>> Update the commit message to make it clear this is an anticipated follow on
>> patch rather than a fix.
>> Only fail attempts to change to a different counter assignment mode.
>> Kconfig indenting.
>> Use "counter assignment mode" text throughout.
>> ---
>>  fs/resctrl/Kconfig   | 9 +++++++++
>>  fs/resctrl/monitor.c | 6 ++++++
>>  2 files changed, 15 insertions(+)
>>
>> diff --git a/fs/resctrl/Kconfig b/fs/resctrl/Kconfig
>> index 21671301bd8a..d833dea81aea 100644
>> --- a/fs/resctrl/Kconfig
>> +++ b/fs/resctrl/Kconfig
>> @@ -37,3 +37,12 @@ config RESCTRL_RMID_DEPENDS_ON_CLOSID
>>  	  Enabled by the architecture when the RMID values depend on the CLOSID.
>>  	  This causes the CLOSID allocator to search for CLOSID with clean
>>  	  RMID.
>> +
>> +config RESCTRL_ASSIGN_FIXED
>> +	bool
>> +	depends on RESCTRL_FS
>> +	help
>> +	  Enabled by the architecture when the counter assignment mode is not
>> +	  configurable. This ensures that counter assignment mode is not
>> +	  advertised as configurable and attempts to change counter assignment
>> +	  mode fail.
>> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
>> index 572a9925bd6c..93756be50062 100644
>> --- a/fs/resctrl/monitor.c
>> +++ b/fs/resctrl/monitor.c
>> @@ -1451,6 +1451,12 @@ ssize_t resctrl_mbm_assign_mode_write(struct kernfs_open_file *of, char *buf,
>>  	}
>>  
>>  	if (enable != resctrl_arch_mbm_cntr_assign_enabled(r)) {
>> +		if (IS_ENABLED(CONFIG_RESCTRL_ASSIGN_FIXED)) {
>> +			ret = -EINVAL;
>> +			rdt_last_cmd_puts("counter assignment mode is not configurable\n");
> 
> Please start output to user space with upper case (unless it is a term that is usually written with
> lower case). You can use "grep rdt_last_cmd_puts fs/resctrl/*" to get an idea if the patterns used.

Yes, it looks better with a capital C.

> 
>> +			goto out_unlock;
>> +		}
>> +
>>  		ret = resctrl_arch_mbm_cntr_assign_set(r, enable);
>>  		if (ret)
>>  			goto out_unlock;
> 
> 
> Reinette


Thanks,

Ben