Re: [PATCH v3 08/12] fs/resctrl: Make info/kernel_mode writable and identify the bound group

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

Hi Babu,

On 6/18/26 6:29 PM, Babu Moger wrote:
> On 6/16/26 18:42, Reinette Chatre wrote:
>> On 4/30/26 4:24 PM, Babu Moger wrote:

...

>>> +/**
>>> + * rdtgroup_config_kmode_clear() - Tear down the kernel-mode binding on @rdtgrp
>>> + * @rdtgrp:    Resctrl group whose kernel-mode binding is being released.
>>> + *        May be %NULL when no group is currently bound, in which case
>>> + *        this is a no-op.
>>> + * @kmode:    Kernel-mode policy currently active on @rdtgrp, as a
>>> + *        BIT(&enum resctrl_kernel_modes) value.  When this is
>>> + *        BIT(INHERIT_CTRL_AND_MON) the hardware tear-down is skipped
>>> + *        because no MSR was previously programmed.
>>> + *
>>> + * Disables the kernel-mode binding on the CPUs @rdtgrp covers (its
>>> + * @kmode_cpu_mask, or all online CPUs when that mask is empty) and resets
>>> + * the per-group bookkeeping (@kmode and @kmode_cpu_mask).  This is the
>>> + * disable counterpart of rdtgroup_config_kmode() and exists so that a write
>>> + * that transitions the active mode to BIT(INHERIT_CTRL_AND_MON) -- which
>>> + * skips rdtgroup_config_kmode() entirely -- still tears down the previously
>>> + * bound group instead of leaving stale enable bits behind.
>>> + *
>>> + * On allocation failure the function returns -ENOMEM and leaves both the
>>> + * hardware state and @rdtgrp's bookkeeping unchanged so the caller can fail
>>> + * the operation atomically and last_cmd_status reflects reality.
>>> + *
>>> + * Context: Caller must hold rdtgroup_mutex.
>>> + *
>>> + * Return: 0 on success (including the @rdtgrp == %NULL and INHERIT cases),
>>> + * -ENOMEM if cpumask allocation fails.
>>> + */
>>> +static int rdtgroup_config_kmode_clear(struct rdtgroup *rdtgrp, int kmode)
>>> +{
>>> +    cpumask_var_t disable_mask;
>>> +    u32 closid, rmid;
>>> +
>>> +    if (!rdtgrp)
>>> +        return 0;
>>> +
>>> +    if (kmode == BIT(INHERIT_CTRL_AND_MON))
>>> +        goto out_clear;
>>> +
>>> +    if (!zalloc_cpumask_var(&disable_mask, GFP_KERNEL))
>>> +        return -ENOMEM;
>>> +
>>> +    if (rdtgrp->type == RDTMON_GROUP) {
>>> +        closid = rdtgrp->mon.parent->closid;
>>> +        rmid = rdtgrp->mon.rmid;
>>> +    } else {
>>> +        closid = rdtgrp->closid;
>>> +        rmid = rdtgrp->mon.rmid;
>>> +    }
>>
> 
> I can directly use it like below. I dont need to check for RDTMON_GROUP.
> 
>     closid = rdtgrp->closid;
>      rmid = rdtgrp->mon.rmid;
> 
> 
>> Same comment as above ... but actually, why is closid/rmid needed at all? This
>> function is intended to *reset* the kernel mode so needing a valid/active closid and
>> rmid does not look right.
> 
> This is a bit tricky. I may need CLOSID/RMID in
> resctrl_arch_configure_kmode(). According to the specification, only
> the PLZA_EN field is allowed to differ across CPUs where PLZA is
> enabled; all other fields must remain consistent across CPUs within
> the same domain. If CLOSID/RMID are not passed, it could result in
> inconsistent values across CPUs.


I see. Let's revisit this in next version. It is not quite clear to me how
the rework of cpu_mask wrangling will impact the resctrl_arch_configure_kmode()
calls. To simplify this for now resctrl could continue to provide closid and rmid
to architecture (with the API documentation in include/linux/resctrl.h documenting
why it is provided and that it may be unused by architecture). 



>>> +
>>> +    /*
>>> +     * Split "<mode>:group=<spec>"; the ":group=<spec>" suffix is optional
>>> +     * and when omitted the default control group (&rdtgroup_default) is used.
>>> +     */
>>> +    group_str = strstr(buf, ":group=");
>>> +    if (group_str) {
>>> +        *group_str = '\0';
>>> +        group_str += strlen(":group=");
>>> +    }
>>> +    mode_str = buf;
>>> +
>>> +    mutex_lock(&rdtgroup_mutex);
>>> +    rdt_last_cmd_clear();
>>> +
>>> +    for (i = 0; i < RESCTRL_NUM_KERNEL_MODES; i++)
>>> +        if (!strcmp(mode_str, resctrl_mode_str[i]))
>>> +            break;
>>> +    if (i == RESCTRL_NUM_KERNEL_MODES) {
>>> +        rdt_last_cmd_puts("Unknown kernel mode\n");
>>> +        ret = -EINVAL;
>>> +        goto out_unlock;
>>> +    }
>>> +
>>> +    if (!(resctrl_kcfg.kmode & BIT(i))) {
>>> +        rdt_last_cmd_puts("Kernel mode not available\n");
>>> +        ret = -EINVAL;
>>> +        goto out_unlock;
>>> +    }
>>> +
>>> +    kmode = BIT(i);
>>
>> Can kmode be of enum type to be assigned the actual enum value to avoid all these BIT(enum value) usages?
> 
> You mean?
> 
> enum resctrl_kernel_modes {
>     INHERIT_CTRL_AND_MON        = 1U << 0,  /* 1 */
>     GLOBAL_ASSIGN_CTRL_INHERIT_MON    = 1U << 1,  /* 2 */
>     GLOBAL_ASSIGN_CTRL_ASSIGN_MON    = 1U << 2,  /* 4 */
> };
> 
> #define RESCTRL_NUM_KERNEL_MODES  3

No. I mean:
	enum resctrl_kernel_mode kmode;
... with a change like this code like below can be simplified:

>>> +    if (kmode == BIT(GLOBAL_ASSIGN_CTRL_ASSIGN_MON_PER_CPU) &&

	kmode == GLOBAL_ASSIGN_CTRL_ASSIGN_MON_PER_CPU

>>> +        rdtgrp->type != RDTMON_GROUP) {
>>> +        rdt_last_cmd_puts("global_assign_ctrl_assign_mon_per_cpu requires a monitor group\n");
>>> +        ret = -EINVAL;
>>> +        goto out_unlock;
>>> +    }
>>> +    if (kmode == BIT(GLOBAL_ASSIGN_CTRL_INHERIT_MON_PER_CPU) &&

	kmode == GLOBAL_ASSIGN_CTRL_INHERIT_MON_PER_CPU

>>> +        rdtgrp->type != RDTCTRL_GROUP) {
>>> +        rdt_last_cmd_puts("global_assign_ctrl_inherit_mon_per_cpu requires a control group\n");
>>> +        ret = -EINVAL;
>>> +        goto out_unlock;
>>> +    }
>>> +

Reinette