[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <009c0e49-2219-482e-a1fe-9920fa0832c7@arm.com>
Date: Wed, 4 Feb 2026 11:40:06 +0000
From: Ben Horgan <ben.horgan@....com>
To: Reinette Chatre <reinette.chatre@...el.com>, linux-kernel@...r.kernel.org
Cc: tony.luck@...el.com, Dave.Martin@....com, james.morse@....com,
babu.moger@....com, bp@...en8.de
Subject: Re: [PATCH v2] fs/resctrl: Add missing kconfig entry for
CONFIG_RESCTRL_ASSIGN_FIXED
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@....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
Powered by blists - more mailing lists