Re: [PATCH v6 13/18] fs/resctrl: Deactivate kernel mode associations when a group is removed

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

Hi Babu,

On 10/1/26 8:10 AM, Babu Moger wrote:
> The active kernel mode association is backed by either a CTRL_MON or
> MON group.

This context does not reflect the code at this point in series but what
behavior will be after entire series is applied. It is also written as
general but specific to the global per-CPU kernel mode.

> When the backing group is removed, or when resctrl is unmounted, clear
> the programmed association and restore the default settings.

Please provide more context to user and help reader to understand what the
patch does without needing to decipher the patch.

For example, "restore the default settings" is a very vague way to
describe that removing the group backing the global per-CPU kernel mode
automatically switches the kernel mode to "inherit user". Please explain
at high level what the code does and why. As a hypothetical, why not
keep the kernel mode as "global per-CPU kernel mode" but switch the assigned
group to the default group? This would match most other similar resctrl behaviors,
no?

This is also user visible behavior that should be documented in resctrl.rst.

> ---
>  fs/resctrl/rdtgroup.c | 44 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 44 insertions(+)
> 
> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
> index f5c7790f7970..0cdc868a55b1 100644
> --- a/fs/resctrl/rdtgroup.c
> +++ b/fs/resctrl/rdtgroup.c
> @@ -1167,6 +1167,36 @@ static void resctrl_kmode_global_offline_cpu(unsigned int cpu)
>  	resctrl_configure_kmode_global(cpumask_of(cpu), rdtgrp, false);
>  }
>  
> +/*
> + * Disable the association on @rdtgrp's kmode_cpu_mask, clear the mask, and
> + * hide the group's kmode_cpus files.

"hide the group's kmode_cpus files"? Does not match the code below.
Even so, the comments just narrate the function.

> + */
> +static void rdtgroup_kmode_global_deactivate(struct rdtgroup *rdtgrp)
> +{
> +	resctrl_configure_kmode_global(&rdtgrp->kmode_cpu_mask, rdtgrp, false);
> +	cpumask_clear(&rdtgrp->kmode_cpu_mask);
> +}
> +
> +/*
> + * rdtgroup_kmode_global_detach() - Detach @rdtgrp from the global per-CPU mode
> + * @rdtgrp: Resctrl group being removed or torn down.
> + *
> + * If @rdtgrp backs the active association, disable it, clear kernel mode
> + * state, and restore RESCTRL_INHERIT_USER.
> + */
> +static void rdtgroup_kmode_global_detach(struct rdtgroup *rdtgrp)
> +{
> +	if (resctrl_kmode.rdtgrp != rdtgrp)

Please do not use tests on resctrl_kmode.rdtgrp as synonym for
resctrl_kmode.active == RESCTRL_GLOBAL_ENABLE_PER_CPU.

> +		return;
> +
> +	rdtgroup_kmode_global_deactivate(rdtgrp);
> +
> +	resctrl_kmode.rdtgrp = NULL;
> +	resctrl_kmode.kmode_cur = RESCTRL_INHERIT_USER;
> +	resctrl_kmode.ctrl = KMODE_ASSIGN;
> +	resctrl_kmode.mon = KMODE_ASSIGN;
> +}

There seems to be some duplication between rdtgroup_kmode_global_detach() and
resctrl_kmode_switch(). Since the contract is already established that
disabling RESCTRL_GLOBAL_ENABLE_PER_CPU enables RESCTRL_INHERIT_USER, could this
reset to RESCTRL_INHERIT_USER not just be done in rdtgroup_kmode_global_deactivate()
without duplicating it in rdtgroup_kmode_global_detach() and resctrl_kmode_switch()?

With that done, it looks like rdtgroup_kmode_global_detach() could just be
dropped if rdtgroup_kmode_global_deactivate() gets the check to ensure the
group needs removing?


> +
>  void *rdt_kn_parent_priv(struct kernfs_node *kn)
>  {
>  	/*
> @@ -3228,6 +3258,7 @@ static void free_all_child_rdtgrp(struct rdtgroup *rdtgrp)
>  
>  	head = &rdtgrp->mon.crdtgrp_list;
>  	list_for_each_entry_safe(sentry, stmp, head, mon.crdtgrp_list) {
> +		rdtgroup_kmode_global_detach(sentry);
>  		rdtgroup_unassign_cntrs(sentry);
>  		free_rmid(sentry->closid, sentry->mon.rmid);
>  		list_del(&sentry->mon.crdtgrp_list);
> @@ -3265,6 +3296,7 @@ static void rmdir_all_sub(void)
>  		cpumask_or(&rdtgroup_default.cpu_mask,
>  			   &rdtgroup_default.cpu_mask, &rdtgrp->cpu_mask);
>  
> +		rdtgroup_kmode_global_detach(rdtgrp);
>  		rdtgroup_unassign_cntrs(rdtgrp);
>  
>  		if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP ||
> @@ -3360,6 +3392,7 @@ static void resctrl_fs_teardown(void)
>  		return;
>  
>  	rmdir_all_sub();
> +	rdtgroup_kmode_global_detach(&rdtgroup_default);
>  	rdtgroup_unassign_cntrs(&rdtgroup_default);
>  	mon_put_kn_priv();
>  	rdt_pseudo_lock_release();
> @@ -4395,6 +4428,8 @@ static int rdtgroup_rmdir_mon(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
>  	u32 closid, rmid;
>  	int cpu;
>  
> +	rdtgroup_kmode_global_detach(rdtgrp);
> +
>  	/* Give any tasks back to the parent group */
>  	rdt_move_group_tasks(rdtgrp, prdtgrp, tmpmask);
>  
> @@ -4442,9 +4477,18 @@ static int rdtgroup_ctrl_remove(struct rdtgroup *rdtgrp)
>  
>  static int rdtgroup_rmdir_ctrl(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
>  {
> +	struct rdtgroup *sentry;
>  	u32 closid, rmid;
>  	int cpu;
>  
> +	/*
> +	 * A monitor group uses its parent's CLOSID. Detach a child that
> +	 * backs the association before that CLOSID is freed.

Why does this have to be done before the CLOSID is freed? The monitor groups
are cleared later from free_all_child_rdtgrp() that will call
rdtgroup_kmode_global_detach() for every monitor again.

> +	 */
> +	list_for_each_entry(sentry, &rdtgrp->mon.crdtgrp_list, mon.crdtgrp_list)
> +		rdtgroup_kmode_global_detach(sentry);
> +	rdtgroup_kmode_global_detach(rdtgrp);
> +
>  	/* Give any tasks back to the default group */
>  	rdt_move_group_tasks(rdtgrp, &rdtgroup_default, tmpmask);
>  

Reinette