Re: [PATCH v6 15/18] fs/resctrl: Reject mode changes for groups backing kernel mode

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

Hi Reinette,

On 10/15/26 10:49, Reinette Chatre wrote:
> Hi Babu,
> 
> On 10/1/26 8:10 AM, Babu Moger wrote:
>> The RESCTRL_GLOBAL_ENABLE_PER_CPU kernel mode can be backed by only one
>> resource group at a time. The active kernel-mode association depends on
>> the group's mode and, for a monitor group, its parent control group.
> 
> I am not able to parse above. The second sentence is difficult to decipher.
> Apart from this it also assumes reader is familiar with how a group can
> back the global kernel mode but this is behavior that does not exist here.

Ack. Will re-write the changelog.

>> Prevent mode changes for a resource group that backs the active kernel
>> mode. Also prevent mode changes to the parent control group of a backing
>> monitor group, and prevent a backing monitor group from being moved to a
>> different parent. These restrictions ensure that the active kernel-mode
>> association remains valid and consistent.
> 
> "These restrictions ensure that the active kernel-mode association remains
> valid and consistent." could this be more specific?
> 
> This patch does two things:
> - prevent user from switching the mode of the resource group backing the
>    global per-CPU kernel mode. Why? The reason for this is never given and
>    there is only a hint in the *code* of the *next* patch that requires the
>    resource group to be shareable. The next changelog does not mention
>    this requirement either.
>    This is important to highlight since a resource group backing the global
>    kernel mode would have all its resource allocations shared by kernel work
>    done on behalf of *all* tasks. This makes it a clear requirement for this
>    group to be shareable and for its mode to never change.
>    This highlights that there is an ordering issue with the patches since this
>    series establishes the requirement after it enforces the requirement.

This check is only exercised when userspace attempts to activate the 
kernel mode, which is introduced in the next patch.  I can merge this 
change in the next patch.

> - prevent a monitor group backing global kernel mode from being moved to
>    another control group. Why should this be prevented? Could the global
>    kernel mode not just be reconfigured, just like when user space
>    provides it a new group? This is a design decision that is never motivated.

[1] There is no specific reason for preventing it. I just did not 
consider it a use case. Also, changing the parent of a backing monitor 
group affects both groups because the kernel resource allocation is 
moved from one group to another.

If there is a valid use case for supporting this, we could allow it and 
update the kernel mode association accordingly. What do you think?

> 
> 
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>> v6: New patch to fix sashiko feedback.
>>      https://sashiko.dev/#/patchset/cover.1787772750.git.babu.moger%40amd.com
>> ---
>>   fs/resctrl/internal.h |  2 ++
>>   fs/resctrl/rdtgroup.c | 35 +++++++++++++++++++++++++++++++++++
>>   2 files changed, 37 insertions(+)
>>
>> diff --git a/fs/resctrl/internal.h b/fs/resctrl/internal.h
>> index 6b8d14c4bd83..84238e9e8e15 100644
>> --- a/fs/resctrl/internal.h
>> +++ b/fs/resctrl/internal.h
>> @@ -462,6 +462,8 @@ ssize_t resctrl_mbm_assign_mode_write(struct kernfs_open_file *of, char *buf,
>>   void resctrl_bmec_files_show(struct rdt_resource *r, struct kernfs_node *l3_mon_kn,
>>   			     bool show);
>>   
>> +bool rdtgroup_has_kmode_global(struct rdtgroup *rdtgrp);
> 
> Does not look like this is needed, can this function just be static within fs/resctrl/rdtgroup.c?


Yes. The only issue is the build dependency. Since the function is 
declared static, it needs a caller in the same patch. One option is to 
merge this change with the next patch, which is where the function is 
first used.

> 
>> +
>>   int resctrl_num_mbm_cntrs_show(struct kernfs_open_file *of, struct seq_file *s, void *v);
>>   
>>   int resctrl_available_mbm_cntrs_show(struct kernfs_open_file *of, struct seq_file *s,
>> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
>> index 98451b417fe3..dcf9f26540d6 100644
>> --- a/fs/resctrl/rdtgroup.c
>> +++ b/fs/resctrl/rdtgroup.c
>> @@ -1738,6 +1738,27 @@ static bool rdtgroup_mode_test_exclusive(struct rdtgroup *rdtgrp)
>>   	return true;
>>   }
>>   
>> +/* True when @rdtgrp or one of its monitor groups backs the kernel mode. */
>> +bool rdtgroup_has_kmode_global(struct rdtgroup *rdtgrp)
>> +{
>> +	struct rdtgroup *child;
>> +
>> +	if (!resctrl_kmode.rdtgrp)
>> +		return false;
>> +
>> +	if (resctrl_kmode.rdtgrp == rdtgrp)
>> +		return true;
>> +
>> +	if (rdtgrp->type != RDTCTRL_GROUP)
>> +		return false;
>> +
>> +	list_for_each_entry(child, &rdtgrp->mon.crdtgrp_list, mon.crdtgrp_list)
>> +		if (child == resctrl_kmode.rdtgrp)
>> +			return true;
>> +
>> +	return false;
>> +}
>> +
>>   /*
>>    * rdtgroup_mode_write - Modify the resource group's mode
>>    */
>> @@ -1778,6 +1799,12 @@ static ssize_t rdtgroup_mode_write(struct kernfs_open_file *of,
>>   		goto out;
>>   	}
>>   
>> +	if (rdtgroup_has_kmode_global(rdtgrp)) {
>> +		rdt_last_cmd_puts("Cannot change mode for kernel-mode assigned group\n");
> 
> The messages to user space also switch back and forth between kernel mode and kernel-mode.

Will change to  "kernel mode".

> 
>> +		ret = -EINVAL;
>> +		goto out;
>> +	}
>> +
>>   	if (!strcmp(buf, "shareable")) {
>>   		if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
>>   			ret = rdtgroup_locksetup_exit(rdtgrp);
>> @@ -4697,6 +4724,14 @@ static int rdtgroup_rename(struct kernfs_node *kn,
>>   		goto out;
>>   	}
>>   
>> +	/* Reject moving to different parent if group backs kernel-mode */
> 
> Comment states the obvious. Could it instead state *why* this is rejected?

Same reason as in [1].

Thanks
Babu