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

[Ben Horgan <ben.horgan@arm.com>]

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