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

[Babu Moger]

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@xxxxxxx>
> > ---
> > 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