Re: [PATCH v6 06/18] fs/resctrl: Introduce kernel mode states for resctrl

["Moger, Babu" <bmoger@amd.com>]

Hi Reinette,

On 10/15/2026 9:37 AM, Reinette Chatre wrote:
> Hi Babu,
> 
> nit: "for resctrl" in subject is redundant.

ok.

> 
> On 10/1/26 8:10 AM, Babu Moger wrote:
>> The filesystem tracks supported kernel modes, the active mode, and the
>> control and monitoring association for the assigned group.
> 
> Please let the context describe the context at the time of the patch, not how
> things will work after entire series is applied.
> 
> Please use consistent terminology, this series switches between "assigned group"
> and "backing group" to refer to the same group.

Will use the "backing group". That seems more clearer.

> 
> Please use the changelog to guide the reader to understand the architecture
> and how things fit together. Many changelogs are very cryptic without explanation
> of what the patch does and why.
> 
Ack.

>>
>> Introduce struct resctrl_kmode_cfg and initialize the default configuration
>> from the platform's capabilities.
> 
> Please check all changelogs to ensure they all are able to provide an
> answer to "why". More specifically, please consider:
> 	https://lore.kernel.org/lkml/20250911165433.GBaML-yTUZHkywuJIe@fat_crate.local/
> 

Ack

>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
> ...
> 
>> ---
>>   fs/resctrl/internal.h | 29 +++++++++++++++++++++++++++++
>>   fs/resctrl/rdtgroup.c | 25 +++++++++++++++++++++++++
>>   2 files changed, 54 insertions(+)
>>
>> diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
>> index e62a277dee85..98f04a2a762b 100644
>> --- a/fs/resctrl/internal.h
>> +++ b/fs/resctrl/internal.h
>> @@ -314,6 +314,35 @@ struct mbm_state {
>>   	u32	prev_bw;
>>   };
>>   
>> +/**
>> + * enum kmode_state - Control or monitoring association
> 
> What does "control or monitoring association" mean? This is becoming
> an overloaded term used to mean different things and becoming confusing.
> For comparison, earlier patch introduced *another*
> "control and monitoring association" enum:
> 
> 	enum resctrl_kernel_mode - Kernel mode control and monitoring association

Sure.

> 
>> + * @KMODE_INHERIT: Inherit from the user-space task.
>> + * @KMODE_ASSIGN: Use the backing group's CLOSID or RMID.
>> + */
>> +enum kmode_state {
>> +	KMODE_INHERIT,
>> +	KMODE_ASSIGN
>> +};
>> +
>> +/**
>> + * struct resctrl_kmode_cfg - Kernel mode configuration
>> + * @kmode_sup:	Bitmap of supported kernel modes.
>> + * @kmode_cur:	Currently selected kernel mode.
> 
> Adding this comment during a second pass ... please consider later motivations
> about naming of these fields. This field is introduced as "kmode_cur" but when
> considering the language used in rest of series "active" seems more appropriate.
> 
> @kmode_sup could also just be @supported (the "kmode" is redundant).

Sure. Will change kmode_cur to "current".


> 
>> + * @ctrl:	Control association for %RESCTRL_GLOBAL_ENABLE_PER_CPU.
>> + *		Kept while %RESCTRL_INHERIT_USER is selected.
> 
> The "Kept while ..." does not seem to match implementation. Looks like
> resctrl_kmode_switch() always resets it to KMODE_ASSIGN?

Yes. Will remove the line.

> 
> At this point the reader is left to decipher the architecture via cryptic
> changelogs and inaccurate comments. It is very difficult to parse.
> 
>> + * @mon:	Monitoring association for %RESCTRL_GLOBAL_ENABLE_PER_CPU.
>> + *		Kept while %RESCTRL_INHERIT_USER is selected.
> 
> Similar comment about "Kept ..."

Yes. Will remove the text.

> 
>> + * @rdtgrp:	Resource group backing %RESCTRL_GLOBAL_ENABLE_PER_CPU.
>> + *		%NULL while that mode is not selected.
>> + */
>> +struct resctrl_kmode_cfg {
>> +	DECLARE_BITMAP(kmode_sup, RESCTRL_NUM_KERNEL_MODES);
>> +	enum resctrl_kernel_mode	kmode_cur;
>> +	enum kmode_state		ctrl;
>> +	enum kmode_state		mon;
>> +	struct rdtgroup			*rdtgrp;
>> +};
>> +
>>   extern struct mutex rdtgroup_mutex;
>>   
>>   static inline const char *rdt_kn_name(const struct kernfs_node *kn)
>> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
>> index 68be9b903ac6..7c39792be1fa 100644
>> --- a/fs/resctrl/rdtgroup.c
>> +++ b/fs/resctrl/rdtgroup.c
>> @@ -76,6 +76,9 @@ static void rdtgroup_destroy_root(void);
>>   
>>   struct dentry *debugfs_resctrl;
>>   
>> +/* Kernel mode for this system. */
>> +static struct resctrl_kmode_cfg resctrl_kmode;
>> +
>>   /*
>>    * Memory bandwidth monitoring event to use for the default CTRL_MON group
>>    * and each new CTRL_MON group created by the user.  Only relevant when
>> @@ -2297,6 +2300,26 @@ static void io_alloc_init(void)
>>   	}
>>   }
>>   
>> +/*
>> + * Set RESCTRL_INHERIT_USER as the default when the platform supports
>> + * allocation or monitoring. Otherwise leave kernel mode unsupported.
> 
> This (and why) would be useful to describe in changelog.

Ack.

> 
>> + */
>> +static void resctrl_kmode_init(void)
>> +{
>> +	if (!resctrl_arch_alloc_capable() && !resctrl_arch_mon_capable())
>> +		return;
>> +
> 
> This check seems unnecessary? resctrl_kmode_init() will only be called
> on a system that is capable of allocation or monitoring.
> The planned AET changes will change this behavior see
> https://lore.kernel.org/lkml/20260928221509.68002-16-tony.luck@intel.com/
> but in that case it still seems as though initializing the kernel mode as
> below always needs to be done.
> 
> That begs the question why is resctrl_kmode_init() needed at all? Why not use
> a static initializer?

Sure. Will do.

> 
> I find resctrl_kmode_init() to be potentially confusing since it does *not*
> actually initialize resctrl_kmode. This is only possible to see two patches
> later where get_rdt_resources() is changed so that x86 sets its supported
> modes and this runs *before* resctrl_kmode_init().
> 
> If anyone decides one day "this initialization code is missing a bitmap_zero()"
> then it will break PLZA.

Sounds good. Will remove resctrl_kmode_init() and statically initialize 
resctrl_kmode.

Thanks
Babu