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

[Babu Moger]

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