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

[Babu Moger <babu.moger@amd.com>]

Hi Reinette,

On 6/22/26 11:47, Reinette Chatre wrote:
> 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).
> 

Sounds good. Lets revisit this again.

> 
> 
>>>> +
>>>> +    /*
>>>> +     * 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

Sure. Will do.

Thanks
Babu