Re: [PATCH v2 01/16] fs/resctrl: Add kernel mode (kmode) data structures and arch hook

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

Hi Reinette,

On 3/24/26 17:51, Reinette Chatre wrote:
> Hi Babu,
>
> On 3/12/26 1:36 PM, Babu Moger wrote:
>> Add resctrl_kmode, resctrl_kmode_cfg, kernel mode bit defines, and
>> resctrl_arch_get_kmode_cfg() for resctrl kernel mode (e.g. PLZA) support.
> We should not have to start every series from scratch.
> Documentation/process/maintainer-tip.rst. Always.

Sure. Yea. I did not focus on that aspect of patch submission in this 
series. Will do next revision.


>
>> ---
>>   include/linux/resctrl.h       | 10 ++++++++++
>>   include/linux/resctrl_types.h | 30 ++++++++++++++++++++++++++++++
>>   2 files changed, 40 insertions(+)
>>
>> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
>> index 006e57fd7ca5..2c36d1ac392f 100644
>> --- a/include/linux/resctrl.h
>> +++ b/include/linux/resctrl.h
>> @@ -699,6 +699,16 @@ int resctrl_arch_io_alloc_enable(struct rdt_resource *r, bool enable);
>>    */
>>   bool resctrl_arch_get_io_alloc_enabled(struct rdt_resource *r);
>>   
>> +/**
>> + * resctrl_arch_get_kmode_cfg() - Get resctrl kernel mode configuration
>> + * @kcfg:	Filled with current kernel mode config (kmode, kmode_cur, k_rdtgrp).
>> + *
>> + * Used by the arch (e.g. x86) to report which kernel mode is active and,
>> + * when a global assign mode is in use, which rdtgroup is assigned to
>> + * kernel work.
>> + */
>> +void resctrl_arch_get_kmode_cfg(struct resctrl_kmode_cfg *kcfg);
> This interface does not look right. Would it not be resctrl fs that determines
> which resource group is assigned? This cannot be set by arch. Why does arch decide
> which mode is active? Is this not also resctrl fs? Should arch not just tell
> resctrl fs what it supports?
Yes. Sure. Let the arch tell what is supported.  Let fs decide what is 
default.
>
>> +
>>   extern unsigned int resctrl_rmid_realloc_threshold;
>>   extern unsigned int resctrl_rmid_realloc_limit;
>>   
>> diff --git a/include/linux/resctrl_types.h b/include/linux/resctrl_types.h
>> index a5f56faa18d2..6b78b08eab29 100644
>> --- a/include/linux/resctrl_types.h
>> +++ b/include/linux/resctrl_types.h
>> @@ -65,7 +65,37 @@ enum resctrl_event_id {
>>   	QOS_NUM_EVENTS,
>>   };
>>   
>> +/**
>> + * struct resctrl_kmode - Resctrl kernel mode descriptor
>> + * @name:	Human-readable name of the kernel mode.
>> + * @val:	Bitmask value for the kernel mode (e.g. INHERIT_CTRL_AND_MON).
>> + */
>> +struct resctrl_kmode {
>> +	char    name[32];
>> +	u32     val;
>> +};
> There is no reason why this needs to be in a central header exposed to archs. Could
> this not be a static within the only function that uses it? Something like
> rdt_mode_str[]?
Yes. I think so.
>
>> +
>> +/**
>> + * struct resctrl_kmode_cfg - Resctrl kernel mode configuration
>> + * @kmode:	Requested kernel mode.
>> + * @kmode_cur:	Currently active kernel mode.
>> + * @k_rdtgrp:	Resource control structure in use, or NULL otherwise.
>> + */
>> +struct resctrl_kmode_cfg {
>> +	u32 kmode;
>> +	u32 kmode_cur;
>> +	struct rdtgroup *k_rdtgrp;
>> +};
>> +
>>   #define QOS_NUM_L3_MBM_EVENTS	(QOS_L3_MBM_LOCAL_EVENT_ID - QOS_L3_MBM_TOTAL_EVENT_ID + 1)
>>   #define MBM_STATE_IDX(evt)	((evt) - QOS_L3_MBM_TOTAL_EVENT_ID)
>>   
>> +/* Resctrl kernel mode bits (e.g. for PLZA). */
>> +#define INHERIT_CTRL_AND_MON		BIT(0)	/* Kernel uses same CLOSID/RMID as user. */
>> +/* One CLOSID for all kernel work; RMID inherited from user. */
>> +#define GLOBAL_ASSIGN_CTRL_INHERIT_MON	BIT(1)
>> +/* One resource group (CLOSID+RMID) for all kernel work. */
>> +#define GLOBAL_ASSIGN_CTRL_ASSIGN_MON	BIT(2)
>> +#define RESCTRL_KERNEL_MODES_NUM	3
> I think it will make the code much easier to understand if the different modes are described by an
> enum. For example,
Yes. Sure.
>
> 	enum resctrl_kernel_modes {
> 		INHERIT_CTRL_AND_MON,
> 		GLOBAL_ASSIGN_CTRL_INHERIT_MON,
> 		GLOBAL_ASSIGN_CTRL_ASSIGN_MON,
> 		RESCTRL_KMODE_LAST = GLOBAL_ASSIGN_CTRL_ASSIGN_MON
> 	};
> 	#define RESCTRL_NUM_KERNEL_MODES (RESCTRL_KMODE_LAST + 1)
>
> The supported kernel modes can still be managed as a bitmap with intuitive API using the
> enum that will make the code easier to read. For example, __set_bit(INHERIT_CTRL_AND_MON, ...)
> or BIT(INHERIT_CTRL_AND_MON). The naming is awkward at the moment though, we should improve here.
> 		

Sure.  Yes. We need to think about naming.. Let me think about it.

Thanks

Babu