Re: [RESEND PATCH v4 14/15] fs/resctrl: Allow user space to write kmode_cpus/kmode_cpus_list

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

Hi Babu,

On 7/7/26 2:50 PM, Babu Moger wrote:
> kmode_cpus and kmode_cpus_list expose the CPU scope for the rdtgroup bound
> to the active kernel-mode policy. They are currently read-only, so changing
> the scope requires rebinding through info/kernel_mode, which reprograms the
> whole binding instead of only the CPUs whose state changes.
> 
> Make kmode_cpus and kmode_cpus_list writable. Parse writes as a bitmap or
> CPU range list. Reject pseudo-locked and pseudo-lock-setup groups, writes
> to a group other than resctrl_kcfg.k_rdtgrp (including stale file
> descriptors left open across an info/kernel_mode change), malformed input,
> and masks that name offline CPUs.
> 
> Update the bound group's kmode_cpu_mask and reprogram hardware
> incrementally: disable kernel-mode association on CPUs in the old mask but
> not the new mask, and enable it on CPUs in the new mask but not the old
> mask.
> 
> Document the interface in Documentation/filesystems/resctrl.rst.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
> v4: Empty masks are now allowed and updated masks are in rdtgroup->kmode_cpu_mask.
>     Updated the changelog.
> 
> v3: New patch to add "kmode_cpus" and "kmode_cpus_list" to support
>     kernel_modes.
> ---
>  Documentation/filesystems/resctrl.rst |  30 +++++
>  fs/resctrl/rdtgroup.c                 | 151 +++++++++++++++++++++++++-
>  2 files changed, 179 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
> index 5a13814d1325..4a2bdd74d4aa 100644
> --- a/Documentation/filesystems/resctrl.rst
> +++ b/Documentation/filesystems/resctrl.rst
> @@ -676,6 +676,36 @@ All groups contain the following files:
>  "cpus_list":
>  	Just like "cpus", only using ranges of CPUs instead of bitmasks.
>  
> +"kmode_cpus":
> +	Visible only on the rdtgroup currently bound to the active kernel

"Visible" -> "Accessible"?
"on" -> "within"?

rdtgroup -> "resource group"
bound -> "assigned"

> +	mode (see "info/kernel_mode"); hidden on every other rdtgroup,
> +	including when "inherit_ctrl_and_mon" is active.

No need to mention that it is hidden in other groups.

> +
> +	Bitmask of the logical CPUs scoped for this group's kernel-mode

What does the "scoped" distinction mean?

> +	binding.  At bind time through info/kernel_mode, every currently

What is "bind time"? Could "bind time through" be replaced with "assigned via"?
Can "assign" be used instead of "bind" throughout this text?

> +	online CPU is included in the scope.  CPUs that come online later
> +	are automatically added to the scope and programmed with the binding.

I do not think "scope" is the right term here.

> +
> +	Writing a mask reprograms the binding incrementally: it enables on

I interpret "incrementally" as the write _adds_ the new CPUs to the mask, which
is not what the implementation does.

> +	the CPUs newly added by the write and disables on the CPUs dropped
> +	from the previous mask.  An empty mask disables the binding on all
> +	CPUs in the current scope.  The mask must contain only online CPUs;

"CPUs in the current scope" what does "in the current scope" refer to?

> +	masks naming offline CPUs are rejected.
> +	Errors are reported in "info/last_cmd_status".  Example::
> +
> +	  # mkdir ctrl1
> +	  # echo "global_assign_ctrl_inherit_mon_per_cpu:group=ctrl1//" \
> +	        > info/kernel_mode
> +	  # echo 0-3 > ctrl1/kmode_cpus_list
> +	  # cat ctrl1/kmode_cpus
> +	  f
> +	  # cat ctrl1/kmode_cpus_list
> +	  0-3
> +
> +"kmode_cpus_list":
> +	Just like "kmode_cpus", only using ranges of CPUs instead of bitmasks.
> +	Writable with the same semantics and restrictions as "kmode_cpus".
> +
>  
>  When control is enabled all CTRL_MON groups will also contain:
>  
> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
> index 7b06c3b3f00e..8ecd107368b3 100644
> --- a/fs/resctrl/rdtgroup.c
> +++ b/fs/resctrl/rdtgroup.c
> @@ -423,6 +423,151 @@ static int rdtgroup_kmode_cpus_show(struct kernfs_open_file *of,
>  	return ret;
>  }
>  
> +/**
> + * kmode_cpus_write() - Update @rdtgrp's kmode_cpu_mask from @newmask
> + * @rdtgrp:	Resctrl group whose kmode_cpu_mask is being updated.
> + * @kmode:	Kernel-mode policy currently active on @rdtgrp.
> + * @newmask:	Set of online CPUs scoped for @rdtgrp's kernel-mode binding.
> + * @tmpmask:	Caller-allocated scratch cpumask used to compute the
> + *		incremental enable/disable deltas; contents on entry are
> + *		ignored and on return are unspecified.

ah - this is where "incremental" comes from. I think this is an implementation
detail that should be invisible to user space.

> + *
> + * Compute the difference between @rdtgrp->kmode_cpu_mask and @newmask
> + * and call resctrl_arch_configure_kmode() only on the CPUs whose enable
> + * state actually changes:
> + *
> + *   - disable on (old & ~new)
> + *   - enable on (new & ~old)
> + *
> + * Then copy @newmask into @rdtgrp->kmode_cpu_mask so subsequent
> + * show/write operations reflect the updated scope.

This can be seen from the code.

> + */
> +static void kmode_cpus_write(struct rdtgroup *rdtgrp, enum resctrl_kernel_mode kmode,
> +			     cpumask_var_t newmask, cpumask_var_t tmpmask)
> +{
> +	bool assign_mon = (kmode == GLOBAL_ASSIGN_CTRL_ASSIGN_MON_PER_CPU);
> +	u32 closid, rmid;
> +
> +	closid = rdtgrp->closid;
> +	rmid = rdtgrp->mon.rmid;
> +
> +	/* CPUs dropped from this group: old & ~newmask. */
> +	cpumask_andnot(tmpmask, &rdtgrp->kmode_cpu_mask, newmask);
> +	if (!cpumask_empty(tmpmask))
> +		resctrl_arch_configure_kmode(tmpmask, closid, rmid, assign_mon, false);
> +
> +	/* CPUs newly added: newmask & ~old. */
> +	cpumask_andnot(tmpmask, newmask, &rdtgrp->kmode_cpu_mask);
> +	if (!cpumask_empty(tmpmask))
> +		resctrl_arch_configure_kmode(tmpmask, closid, rmid, assign_mon, true);
> +
> +	cpumask_copy(&rdtgrp->kmode_cpu_mask, newmask);
> +}
> +
> +/**
> + * rdtgroup_kmode_cpus_write() - Sysfs write handler for kmode_cpus[_list]
> + * @of:		kernfs open file (selects bitmap vs range-list parsing via
> + *		is_cpu_list()).
> + * @buf:	NUL-terminated input from userspace.
> + * @nbytes:	Length of @buf, returned on success.
> + * @off:	File offset (unused).
> + *
> + * Parses @buf into a cpumask and rejects:
> + *   - pseudo-locked / pseudo-lock-setup groups,
> + *   - writes when INHERIT_CTRL_AND_MON is active or to a group other than
> + *     resctrl_kcfg.k_rdtgrp (stale fds opened before an info/kernel_mode
> + *     change),
> + *   - malformed input,
> + *   - masks containing offline CPUs.

This just describes the code and seems unnecessarry.


> + *
> + * Validated masks are passed to kmode_cpus_write() to update
> + * @rdtgrp->kmode_cpu_mask and reprogram hardware incrementally.
> + * Errors are reported in last_cmd_status.
> + *
> + * Return: @nbytes on success, -ENOENT if the group has been deleted,
> + * -EINVAL for pseudo-locked or pseudo-lock-setup groups, malformed input, or
> + * offline CPUs in the requested mask, -EBUSY if INHERIT_CTRL_AND_MON is active
> + * or the group is not resctrl_kcfg.k_rdtgrp, and -ENOMEM if the scratch
> + * cpumasks cannot be allocated.
> + */
> +static ssize_t rdtgroup_kmode_cpus_write(struct kernfs_open_file *of,
> +					 char *buf, size_t nbytes, loff_t off)
> +{
> +	cpumask_var_t tmpmask, newmask;
> +	struct rdtgroup *rdtgrp;
> +	int ret;
> +
> +	if (!buf)
> +		return -EINVAL;
> +
> +	if (!zalloc_cpumask_var(&tmpmask, GFP_KERNEL))
> +		return -ENOMEM;
> +	if (!zalloc_cpumask_var(&newmask, GFP_KERNEL)) {
> +		free_cpumask_var(tmpmask);
> +		return -ENOMEM;
> +	}

Please see related recent changes to resctrl:
commit 242c0ab4d51d ("fs/resctrl: Change last_cmd_status custom during input parsing")

For comparison you can view latest
implementation of rdtgroup_cpus_write().

> +
> +	rdtgrp = rdtgroup_kn_lock_live(of->kn);
> +	if (!rdtgrp) {
> +		ret = -ENOENT;
> +		goto unlock;
> +	}
> +
> +	rdt_last_cmd_clear();

rdtgroup_kn_lock_live() now calls rdt_last_cmd_clear().

> +
> +	if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED ||
> +	    rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
> +		ret = -EINVAL;
> +		rdt_last_cmd_puts("Pseudo-locked group cannot host kernel-mode binding\n");
> +		goto unlock;
> +	}

Similar to previous comment I believe this sprinkling of pseudo-locked mode
checks can be avoided by ensuring that (a) a pseudo-locked/locksetup group
cannot be assigned to a kernel mode, and (b) a group assigned to a kernel mode
cannot have its mode changed.

In this case then resctrl_kmode_cfg::k_rdtgroup can never be a pseudo-locked group
and the if (resctrl_kcfg.k_rdtgrp != rdtgrp) check below would be sufficient?

> +
> +	if (resctrl_kcfg.kmode_cur == INHERIT_CTRL_AND_MON) {
> +		ret = -EBUSY;
> +		rdt_last_cmd_puts("No active kernel-mode binding\n");

I do not know where this "binding" term came from and all of a sudden it is
everywhere. The inconsistent constantly changing terms used in this series makes
it difficult to follow.

> +		goto unlock;
> +	}
> +
> +	/*
> +	 * The visibility layer (kernfs_show()) prevents fresh open() on a

Visibility layer? Another new term. After this introduction it is the only
instance of this term in all kernel source.

> +	 * non-bound group, but file descriptors opened while the group was
> +	 * bound stay valid across an info/kernel_mode change.  Reject those
> +	 * stale-fd writes so they cannot corrupt the now-active binding.
> +	 */
> +	if (resctrl_kcfg.k_rdtgrp != rdtgrp) {
> +		ret = -EBUSY;
> +		rdt_last_cmd_puts("Group is not the active kernel-mode binding\n");
> +		goto unlock;
> +	}
> +
> +	if (is_cpu_list(of))
> +		ret = cpulist_parse(buf, newmask);
> +	else
> +		ret = cpumask_parse(buf, newmask);
> +
> +	if (ret) {
> +		rdt_last_cmd_puts("Bad CPU list/mask\n");
> +		goto unlock;
> +	}
> +
> +	/* kernel-mode binding is only programmed on online CPUs. */
> +	cpumask_andnot(tmpmask, newmask, cpu_online_mask);
> +	if (!cpumask_empty(tmpmask)) {
> +		ret = -EINVAL;
> +		rdt_last_cmd_puts("Can only assign online CPUs\n");
> +		goto unlock;
> +	}
> +
> +	kmode_cpus_write(rdtgrp, resctrl_kcfg.kmode_cur, newmask, tmpmask);
> +
> +unlock:
> +	rdtgroup_kn_unlock(of->kn);
> +	free_cpumask_var(tmpmask);
> +	free_cpumask_var(newmask);
> +
> +	return ret ?: nbytes;
> +}
> +
>  /*
>   * Update the PGR_ASSOC MSR on all cpus in @cpu_mask,
>   *
> @@ -2531,15 +2676,17 @@ static struct rftype res_common_files[] = {
>  	},
>  	{
>  		.name		= "kmode_cpus",
> -		.mode		= 0444,
> +		.mode		= 0644,
>  		.kf_ops		= &rdtgroup_kf_single_ops,
> +		.write		= rdtgroup_kmode_cpus_write,
>  		.seq_show	= rdtgroup_kmode_cpus_show,
>  		.fflags		= RFTYPE_BASE,
>  	},
>  	{
>  		.name		= "kmode_cpus_list",
> -		.mode		= 0444,
> +		.mode		= 0644,
>  		.kf_ops		= &rdtgroup_kf_single_ops,
> +		.write		= rdtgroup_kmode_cpus_write,
>  		.seq_show	= rdtgroup_kmode_cpus_show,
>  		.flags		= RFTYPE_FLAGS_CPUS_LIST,
>  		.fflags		= RFTYPE_BASE,

Reinette