Re: [PATCH v6 12/18] fs/resctrl: Add CPU hotplug support for kernel mode associations

[Reinette Chatre]

Hi Babu,

On 10/1/26 8:10 AM, Babu Moger wrote:
> Kernel mode allocation and monitoring associations are maintained on a

wrt "Kernel mode", this is specific to only the "global per-CPU kernel mode", no?

> per-CPU basis. Extend the kernel mode infrastructure to integrate with
> CPU hotplug handling, allowing kernel mode state to be managed as CPUs
> enter or leave the online set.
> 
> Introduce resctrl_kmode_global_online_cpu() and
> resctrl_kmode_global_offline_cpu(), and hook them into the existing
> resctrl_online_cpu() and resctrl_offline_cpu() paths.

Above just narrates the diff. Please check all changelogs to match
tip changelog expectations. So far they are light on context/problem/solution
and many miss an answer to "why".

It deserves a mention in the changelog that the active kernel mode is automatically
enabled on a new online CPU followed by why this is done. This is a big design decision
that will be user visible but I missed where this is highlighted and motivated.

Changelog should help reader to understand the patch. In this series the changelogs
provide little help.

> Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
> ---
...

> ---
>  fs/resctrl/rdtgroup.c | 57 +++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 57 insertions(+)
> 
> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
> index 0e638dc654c1..f5c7790f7970 100644
> --- a/fs/resctrl/rdtgroup.c
> +++ b/fs/resctrl/rdtgroup.c
> @@ -410,6 +410,17 @@ static int rdtgroup_kmode_cpus_show(struct kernfs_open_file *of,
>  	return ret;
>  }
>  
> +static void resctrl_configure_kmode_global(const struct cpumask *mask,
> +					   struct rdtgroup *rdtgrp,
> +					   bool enable)
> +{
> +	resctrl_arch_configure_kmode_global(mask, rdtgrp->closid,
> +					    resctrl_kmode.ctrl == KMODE_ASSIGN,
> +					    rdtgrp->mon.rmid,
> +					    resctrl_kmode.mon == KMODE_ASSIGN,
> +					    enable);
> +}
> +
>  /*
>   * Update the PGR_ASSOC MSR on all cpus in @cpu_mask,
>   *
> @@ -1114,6 +1125,48 @@ static int resctrl_kernel_mode_show(struct kernfs_open_file *of,
>  	return 0;
>  }
>  
> +/*
> + * resctrl_kmode_global_online_cpu() - Program kernel mode association for @cpu
> + * @cpu: CPU that has just been brought online
> + *
> + * When global_enable_per_cpu is active, @cpu joins that association and is

Please consistently use the enum name in comments and code to help grep find
how/where it is used in the code.

> + * programmed on.

What does "is programmed on" mean? CPU is already online.

> + */
> +static void resctrl_kmode_global_online_cpu(unsigned int cpu)
> +{
> +	struct rdtgroup *rdtgrp = resctrl_kmode.rdtgrp;
> +
> +	if (resctrl_kmode.kmode_cur != RESCTRL_GLOBAL_ENABLE_PER_CPU || !rdtgrp)
> +		return;
> +
> +	cpumask_set_cpu(cpu, &rdtgrp->kmode_cpu_mask);
> +
> +	resctrl_configure_kmode_global(cpumask_of(cpu), rdtgrp, true);

Shouldn't CPU be added to mask *after* it has been configured? I understand that
configuration cannot fail but having the order set makes it easier to make changes
later.

> +}
> +
> +/*
> + * resctrl_kmode_global_offline_cpu() - Clear kernel mode association for @cpu
> + * @cpu: CPU being taken offline.
> + *
> + * When global_enable_per_cpu is active and @cpu is in kmode_cpu_mask, the
> + * association is disabled on @cpu and @cpu is removed from that mask. A CPU
> + * outside the mask is left unchanged.
> + */
> +static void resctrl_kmode_global_offline_cpu(unsigned int cpu)
> +{
> +	struct rdtgroup *rdtgrp = resctrl_kmode.rdtgrp;
> +
> +	if (resctrl_kmode.kmode_cur != RESCTRL_GLOBAL_ENABLE_PER_CPU || !rdtgrp)
> +		return;
> +
> +	if (!cpumask_test_cpu(cpu, &rdtgrp->kmode_cpu_mask))
> +		return;
> +
> +	cpumask_clear_cpu(cpu, &rdtgrp->kmode_cpu_mask);
> +
> +	resctrl_configure_kmode_global(cpumask_of(cpu), rdtgrp, false);
> +}
> +
>  void *rdt_kn_parent_priv(struct kernfs_node *kn)
>  {
>  	/*
> @@ -4896,6 +4949,7 @@ void resctrl_online_cpu(unsigned int cpu)
>  	mutex_lock(&rdtgroup_mutex);
>  	/* The CPU is set in default rdtgroup after online. */
>  	cpumask_set_cpu(cpu, &rdtgroup_default.cpu_mask);
> +	resctrl_kmode_global_online_cpu(cpu);
>  	mutex_unlock(&rdtgroup_mutex);
>  }
>  
> @@ -4939,6 +4993,9 @@ void resctrl_offline_cpu(unsigned int cpu)
>  		}
>  	}
>  
> +	/* Clear any active kernel mode association on this CPU. */

Above comment can be dropped, it just duplicates the function comment.

> +	resctrl_kmode_global_offline_cpu(cpu);
> +
>  	if (!l3->mon_capable)
>  		goto out_unlock;
>  

Reinette