Re: [PATCH v5 06/16] fs/resctrl: Introduce kernel mode states for resctrl

[Babu Moger <babu.moger@amd.com>]

Hi Reinette,

On 9/16/26 00:28, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/26/26 12:32 PM, Babu Moger wrote:
> 
>> ---
>>   fs/resctrl/internal.h | 51 +++++++++++++++++++++++++++++++++++++++++++
>>   fs/resctrl/rdtgroup.c | 33 ++++++++++++++++++++++++++++
>>   2 files changed, 84 insertions(+)
>>
>> diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
>> index e62a277dee85..4087bf44a06d 100644
>> --- a/fs/resctrl/internal.h
>> +++ b/fs/resctrl/internal.h
>> @@ -314,6 +314,57 @@ struct mbm_state {
>>   	u32	prev_bw;
>>   };
>>   
>> +/**
>> + * enum kmode_state - Control or monitoring state for a kernel mode
>> + * @KMODE_INHERIT: Inherit from the user space task.
>> + * @KMODE_ASSIGN: Use a global assignment for kernel mode.
>> + */
>> +enum kmode_state {
>> +	KMODE_INHERIT,
>> +	KMODE_ASSIGN
>> +};
>> +
>> +/**
>> + * struct resctrl_kmode_caps - Static kernel mode capabilities
>> + * @kmode_sup:	Bitmap of supported kernel modes. Empty when neither
>> + *		@ctrl_en nor @mon_en is set and kernel mode policy is
>> + *		unavailable on this system.
>> + * @ctrl_en:	Whether kernel mode may use global assignment for control.
>> + * @mon_en:	Whether kernel mode may use global assignment for monitoring.
> 
> Why is ctrl_en and mon_en needed? It seems to just store the output of
> whether system supports allocation and monitoring. I only see these used
> when user interacts reads or writes the kernel mode so not a "hot path" that
> needs to be optimized. Can these just be dropped and just use resctrl_arch_alloc_capable()
> and resctrl_arch_mon_capable() directly? Please note they are in process of
> being changed/renamed:

Sure. I'll remove it.

That also means we can consolidate everything into a single 
resctrl_kmode_cfg structure.


> https://lore.kernel.org/lkml/20260831174421.13921-7-tony.luck@intel.com/
> 
>> + */
>> +struct resctrl_kmode_caps {
>> +	DECLARE_BITMAP(kmode_sup, RESCTRL_NUM_KERNEL_MODES);
>> +	bool				ctrl_en;
>> +	bool				mon_en;
>> +};
>> +
>> +/**
>> + * struct resctrl_kmode_active - Runtime kernel mode state
>> + * @kmode_cur:	Currently selected kernel mode.
>> + * @ctrl_mode:	Control assignment state when kernel mode is active.
>> + * @mon_mode:	Monitoring assignment state when kernel mode is active.
>> + * @k_rdtgrp:	Resource group backing global assignment mode.
>> + *
>> + * When @kmode_cur is %RESCTRL_INHERIT_USER, assignment state is ignored and
>> + * @k_rdtgrp is %NULL.
> 
> This implies that this is only the state for RESCTRL_ASSIGN_GLOBAL_ENABLE_PER_CPU.
> If it is made specifically so there is no need to pretend it is generic and
> add all these caveats.

ok.

> 
>> + */
>> +struct resctrl_kmode_active {
>> +	enum resctrl_kernel_mode	kmode_cur;
>> +	enum kmode_state		ctrl_mode;
>> +	enum kmode_state		mon_mode;
> 
> Is "mode" accurate? This is all about kernel "mode" and now control and
> monitoring have other modes?

It can be control and monitor.

> 
>> +	struct rdtgroup			*k_rdtgrp;
> 
> Could naming be consistent? Consider, for example, kmode_rdtgrp? Although
> if this struct can be specific to the global per-CPU kernel mode then it can
> just be "rdtgrp".

Sure.

> 
>> +};
>> +
>> +/**
>> + * struct resctrl_kmode_cfg - Global kernel mode state
>> + * @caps:	Supported modes and assignment capabilities.
>> + * @active:	Active mode, assignment state, and assigned group.
> 
> Please do not list the struct members as part of its description elsewhere since
> that will be difficult to keep accurate. Just describe what the struct represents.

Will remove it.
> 
>> + */
>> +struct resctrl_kmode_cfg {
>> +	struct resctrl_kmode_caps	caps;
>> +	struct resctrl_kmode_active	active;
>> +};
>> +
>>   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 5dcbb0a964e8..3c53f3f74e5a 100644
>> --- a/fs/resctrl/rdtgroup.c
>> +++ b/fs/resctrl/rdtgroup.c
>> @@ -76,6 +76,13 @@ static void rdtgroup_destroy_root(void);
>>   
>>   struct dentry *debugfs_resctrl;
>>   
>> +/*
>> + * Global kernel mode policy state: supported modes, active mode, assignment
>> + * capabilities, assignment state, and the resource group selected for a global
>> + * assignment.
> 
> Same here - please do not just provide a list of the struct's members. A high level
> description instead.

ok.

> 
> This code is really strange. This whole series is difficult to read. I have not seen
> these styles used before and surprised that it comes from you.

ack.

> 
>> + */
>> +static struct resctrl_kmode_cfg resctrl_kcfg;
> 
> I think the code will be easier to read if "resctrl_kcfg" -> "resctrl_kmode".

Sure.

> 
>> +
>>   /*
>>    * 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 +2304,30 @@ static void io_alloc_init(void)
>>   	}
>>   }
>>   
>> +/*
>> + * Initialize kernel mode policy defaults from architecture capabilities.
>> + *
>> + * When ctrl_en or mon_en is set, RESCTRL_INHERIT_USER is supported and
>> + * selected as the initial active mode. When neither is set, kmode_sup is
>> + * left empty, kernel mode policy is unavailable, and kmode_cur remains at
>> + * its zero-initialized default (RESCTRL_INHERIT_USER) but is unused.
> 
> Above just verbatim describes the code. Please provide higher level why the
> code does what it does.

ok.

Thanks
Babu