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/19/26 17:01, Reinette Chatre wrote:
> 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.

Sure. Will move after patch 16.

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

Ok. Sure.


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


My bad.

> 
>   
>>>
>>>> +
>>>>    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().

You are right. This can be static.

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

Sure. Will do.
Thanks
Babu