Re: [PATCH v5 09/16] fs/resctrl: Add interface to display supported and active kernel modes

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

Hi Reinette,

On 9/16/26 00:32, Reinette Chatre wrote:
> Hi Babu,
> 
> On 8/26/26 12:32 PM, Babu Moger wrote:
>> ---
>>   Documentation/filesystems/resctrl.rst |  29 +++++++
>>   fs/resctrl/rdtgroup.c                 | 115 ++++++++++++++++++++++++++
>>   2 files changed, 144 insertions(+)
>>
>> diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
>> index f3e941404967..c6e8cf828e18 100644
>> --- a/Documentation/filesystems/resctrl.rst
>> +++ b/Documentation/filesystems/resctrl.rst
>> @@ -528,6 +528,35 @@ conveyed in the error returns from file operations. E.g.
>>   	# cat info/last_cmd_status
>>   	mask f7 has non-consecutive 1-bits
>>   
>> +"kernel_mode":
>> +	In the top level of the "info" directory, "kernel_mode" reports
>> +	supported and active kernel modes available on the system.
>> +
>> +	Reading the file lists one mode per line. The active mode is wrapped in
>> +	square brackets. inherit_user is shown without options.
>> +	assign_global_enable_per_cpu is shown as::
>> +
>> +	  assign_global_enable_per_cpu:ctrl=<assign|inherit>;mon=<assign|inherit>;group=<ctrl>/<mon>/
>> +
>> +	- inherit_user: inherit allocation and monitoring from the user task.
> 
> To make this easier to read, instead of mixing the modes, could you please have text like:
> 	
> 	Possible kernel modes are:
> 	
> 	<description of inherit_user>
> 
> 	<description of assign_global_enable_per_cpu>
> 
> In addition to being easier to read something like above will also be easier to expand.

Sure. Will do.

> 
>> +	- assign_global_enable_per_cpu: kernel mode may use separate
>> +	  allocation and/or monitoring associations. ctrl= and mon= show
>> +	  whether each is assigned or inherited, and group= identifies the
> 
> Earlier is "ctrl=<assign|inherit>;mon=<assign|inherit>" and above is
> "ctrl= and mon= show whether each is assigned or inherited". This is rewrites
> the same thing in different ways without helping to explain what the parameters
> and their values mean.

Will add of the descriptions of "mon" and "ctrl" states.

> 
> 
>> +	  associated resource group using <CTRL_MON>/<MON>/ path syntax.
> 
> This text is difficult to parse. It switches between "assigned" and "associated"
> *in the same sentence* when referring to the same thing. Please use consistent terms
> and just be specific about what this actually by avoiding this vague language.

Ok. Will rephrase.

> 
>> +
>> +	Only supported modes are listed. On an inactive
>> +	assign_global_enable_per_cpu line, ctrl= and mon= show platform
>> +	capability defaults rather than the last active assignment, and
> 
> I find the text to be very vague. Above could be moved to section dedicated to
> the assign_global_enable_per_cpu mode and be made specific. For example,
> 
> 	When inactive, the parameter values displayed are the default values
> 	that will be used when no new value is provided during enabling/activation(*)
> 	
> 	(*) pick a term and stick with it

Ok. Sure.

> 
>> +	group=// is shown. When the mode is active, ctrl= and mon= report
>> +	the current assignment state and group= identifies the associated
>> +	resource group.
> 
> Last sentence just repeats the paragraph above, no?

Yes. Will rephrase this paragraph.

> 
> 
>> +
>> +	Example::
>> +
>> +	  # cat info/kernel_mode
>> +	  [inherit_user]
>> +	  assign_global_enable_per_cpu:ctrl=assign;mon=assign;group=//
>> +
>>   Resource alloc and monitor groups
>>   =================================
>>   
>> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
>> index 3bb0e3a203ac..992af586a194 100644
>> --- a/fs/resctrl/rdtgroup.c
>> +++ b/fs/resctrl/rdtgroup.c
>> @@ -1010,6 +1010,114 @@ static int rdt_last_cmd_status_show(struct kernfs_open_file *of,
>>   	return 0;
>>   }
>>   
>> +/* Sysfs lines for info/kernel_mode; indexed by enum resctrl_kernel_mode */
>> +static const char * const resctrl_mode_str[] = {
> 
> This is incredibly close to resctrl's existing rdtgroup_mode_str while being
> very high level. Could it be renamed to be more specific to this feature?
> For example, resctrl_kmode_str[]?

Sure.

> 
>> +	[RESCTRL_INHERIT_USER]	=			"inherit_user",
>> +	[RESCTRL_ASSIGN_GLOBAL_ENABLE_PER_CPU] =	"assign_global_enable_per_cpu"
> 
> As mentioned earlier I am starting to think that the "assign" in the name
> is not necessary and creates confusion with the "assign" values of its
> parameters.

Sure. Will remove "assign" from the name.

> 
>> +};
>> +
>> +static_assert(ARRAY_SIZE(resctrl_mode_str) == RESCTRL_NUM_KERNEL_MODES);
>> +
>> +static const char *resctrl_kmode_state_str(enum kmode_state state)
>> +{
>> +	return state == KMODE_ASSIGN ? "assign" : "inherit";
>> +}
>> +
>> +static void resctrl_kmode_group_path(struct rdtgroup *rdtgrp,
>> +				     const char **ctrl, const char **mon)
>> +{
>> +	*ctrl = "";
>> +	*mon = "";
>> +
>> +	if (!rdtgrp)
>> +		return;
>> +
>> +	if (rdtgrp->type == RDTMON_GROUP) {
>> +		*ctrl = rdt_kn_name(rdtgrp->mon.parent->kn);
>> +		*mon = rdt_kn_name(rdtgrp->kn);
>> +	} else {
>> +		*ctrl = rdt_kn_name(rdtgrp->kn);
>> +	}
>> +}
>> +
>> +/**
>> + * resctrl_kernel_mode_show() - Display supported and active kernel modes
>> + * @of: kernfs open file
>> + * @seq: output seq_file
>> + * @v: unused
>> + *
>> + * Lists one line per mode set in resctrl_kcfg.caps.kmode_sup. Brackets the
>> + * active mode. inherit_user is shown without any options.
>> + * assign_global_enable_per_cpu is shown as:
>> + *
>> + *   assign_global_enable_per_cpu:ctrl=<assign|inherit>;mon=<assign|inherit>;\
>> + *					group=<ctrl>/<mon>/
>> + *
>> + * When assign_global_enable_per_cpu is inactive, ctrl=assign and mon=assign
>> + * reflect resctrl_kcfg.caps.ctrl_en and resctrl_kcfg.caps.mon_en, and
>> + * group=//. When active, assign state comes from resctrl_kcfg.active.ctrl_mode
>> + * and resctrl_kcfg.active.mon_mode.
> 
> The code is much easier to read than above summary of it.

Let me rephrase it briefly.

> 
>> + *
>> + * Return: 0 on success, or -ENOENT on error.
>> + */
>> +static int resctrl_kernel_mode_show(struct kernfs_open_file *of,
>> +				    struct seq_file *seq, void *v)
>> +{
>> +	const char *ctrl_state, *mon_state;
>> +	enum resctrl_kernel_mode mode;
>> +	struct rdtgroup *rdtgrp;
>> +	const char *ctrl, *mon;
>> +	bool active;
>> +	int ret = 0;
>> +
>> +	if (!info_kn_lock(of->kn))
>> +		return -ENOENT;
>> +
>> +	for (mode = 0; mode < RESCTRL_NUM_KERNEL_MODES; mode++) {
>> +		if (!test_bit(mode, resctrl_kcfg.caps.kmode_sup))
>> +			continue;
>> +
>> +		active = (resctrl_kcfg.active.kmode_cur == mode);
>> +
>> +		if (mode == RESCTRL_INHERIT_USER) {
>> +			seq_printf(seq, active ? "[%s]\n" : "%s\n",
>> +				   resctrl_mode_str[mode]);
>> +			continue;
>> +		}
>> +
>> +		if (active) {
>> +			ctrl_state = resctrl_kmode_state_str(resctrl_kcfg.active.ctrl_mode);
>> +			mon_state = resctrl_kmode_state_str(resctrl_kcfg.active.mon_mode);
>> +			rdtgrp = resctrl_kcfg.active.k_rdtgrp;
>> +			if (WARN_ON(!rdtgrp)) {
> 
> Just pr_warn() is sufficient and avoids the discussion about panic_on_warn kernels.

ok.

> 
>> +				rdt_last_cmd_puts("Invalid kernel mode group\n");
>> +				ret = -ENOENT;
>> +				goto out_unlock;
>> +			}
>> +			resctrl_kmode_group_path(rdtgrp, &ctrl, &mon);
>> +		} else {
>> +			ctrl_state = resctrl_kcfg.caps.ctrl_en ? "assign" : "inherit";
> 
> This does not look right. As I understand ctrl_en represents whether the system supports
> allocation or not. So above means that if system supports allocation then the allocation
> state is "assign" while a system that does *not* support allocation inherits *allocation*
> association from user space?

When allocation is not supported, we should not print "ctrl". It should 
just print "<mode>:mon=assign;group=//"

Same way just print "<mode>ctrl=assign;group=//" when monitoring is not 
supported.

Will take care of it.
> 
>> +			mon_state = resctrl_kcfg.caps.mon_en ? "assign" : "inherit";
>> +			ctrl = "";
>> +			mon = "";
>> +		}
>> +
>> +		if (active) {
> 
> Why are there two separate "if (active)" blocks?

Will merge it.

> 
>> +			seq_printf(seq, "[%s:ctrl=%s;mon=%s;group=%s/%s/]\n",
> 
> This does not look right. The reason why this version moved to a single global mode
> with parameters is to be able to support systems that do not support allocation or
> monitoring. Having these separate parameters thus enables resctrl to only show the
> "ctrl=" parameter on a system that only supports allocation and only show the
> "mon=" parameter on a system that only supports monitoring. Above just keeps showing
> both whether system supports allocation/monitoring or not.

Yes. That is correct. It should only print states that are supported. 
Will change it.

thanks
Babu