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


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

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

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

> +	if (rdtgrp == resctrl_kmode.rdtgrp &&
> +	    rdtgrp->mon.parent != new_prdtgrp) {
> +		rdt_last_cmd_puts("Cannot move a group that backs kernel mode\n");
> +		ret = -EPERM;
> +		goto out;
> +	}
> +
>  	/*
>  	 * Allocate the cpumask for use in mongrp_reparent() to avoid the
>  	 * possibility of failing to allocate it after kernfs_rename() has

Reinette