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

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Babu,

On 10/19/26 10:43 AM, Babu Moger wrote:
> On 10/15/26 10:49, Reinette Chatre wrote:
>> On 10/1/26 8:10 AM, Babu Moger wrote:
>>> 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.

The next patch is already quite large. I would instead suggest that the
changelog of next patch includes why a resource group backing the global
kernel mode is required to be shareable. This patch can then follow after
that to ensure that the mode of a resource group backing kernel mode cannot
change.

> 
>> - 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?

Up to you, but please make these design decisions explicit. Do not hide them
behind hand waving like "These restrictions ensure that the active kernel-mode
association remains valid and consistent.". If you choose to delay this support
until your customers ask for it then please change the message to something
that highlights this. For example, "Cannot move a group that backs kernel mode"
can be something like "Moving the MON group that backs global_enable_per_cpu kernel
mode is not supported."

>>> 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.

There is a caller in this patch, no?

 
>>
>>> +
>>>   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)) {

Above calls rdtgroup_has_kmode_global().

>>> +        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].

ok ... but could the comment be changed to reflect this design choice instead
of stating what is obvious just by looking at the code?

Reinette