Re: [PATCH v3 04/12] x86,fs/resctrl: Program PLZA through kmode arch hooks

[Babu Moger]

Hi Qinyun,

Thanks for reviewing the patches.

On 6/5/26 05:06, Qinyun Tan wrote:
> On Wed, May 20, 2026 at 06:09:27PM -0500, Moger, Babu wrote:
> > On 5/20/2026 5:16 PM, Luck, Tony wrote:
> > > So maybe I'm just confused by the name
> > > "global_assign_ctrl_inherit_mon_per_cpu"
> > >
> > > That sounds like "Use the CLOSID from PLZA, but keep the RMID from
> > > legacy PQR_ASSOC.
> >
> > > To make that happen, I thing the PLZA MSR should have rmid_en = 0.
> > > But the only code I see that sets this always sets rmid_en=1.
> >
> > Setting rmid_en = 0 in [1] disables counting of kernel usage for
> > RMID 1 (from ctrl1).
>
> I've been reviewing these patches and went back to the AMD hardware
> manual (Publication #69193, Rev 1.00, March 2026) to understand the
> RMID_EN semantics.
>
> The spec defines RMID_EN (bit 31 of PQR_PLZA_ASSOC, MSR C000_03FCh):
>
>      "RMID_EN (PLZA Resource Monitoring Identifier Association Enable)
>       - bit 31. When set and PLZA_EN is set, the Resource Monitoring
>       Identifier (RMID) for this logical processor executing at CPL=0
>       is specified in the RMID field instead of PQR_ASSOC[RMID]."
>
> Reading "instead of PQR_ASSOC[RMID]", my understanding is that when
> RMID_EN is clear, the hardware falls back to PQR_ASSOC[RMID] - the
> user process's RMID - rather than disabling monitoring entirely.
> If that reading is correct, RMID_EN would be a source selector
> between PLZA and PQR_ASSOC, not an on/off switch for counting.
>
> Babu, could you confirm whether that is the intended hardware
> behavior?

Yes. That is correct. RMID_EN is a source selector.

> > The key difference between the two modes is:
> >
> > In one mode, user and kernel usage are counted together.
> > In the other mode, kernel usage is counted separately from user usage.
>
> If RMID_EN=0 does fall back to PQR_ASSOC[RMID], then the "counted
> together" behavior only holds when the user process happens to be in
> the same group as the PLZA-bound group.  Taking Tony's example:
>
>    # mkdir ctrl1   # CLOSID=1, RMID=1, bound to inherit_mon via PLZA
>    # mkdir ctrl2   # CLOSID=2, RMID=2
>    # echo $$ > ctrl2/tasks
>
> With rmid_en=1 (current code): when the shell (in ctrl2, PQR_ASSOC
> RMID=2) enters kernel mode, kernel activity is counted under PLZA
> RMID=1.  The user process's RMID is NOT inherited.
>
> With rmid_en=0: kernel mode would fall back to PQR_ASSOC RMID=2,
> correctly inheriting the monitoring identity of the current user
> process - which seems to be what "inherit_mon" intends.
>
> If this reading is right, Tony's suggestion of adding a parameter to
> control rmid_en based on the mode would be the right fix:
>
>    void resctrl_arch_configure_kmode(const struct cpumask *cpu_mask,
>                                      u32 closid, u32 rmid,
>                                      bool assign_rmid, bool enable)
>
> where assign_rmid=true sets RMID_EN=1 (assign_mon) and
> assign_rmid=false sets RMID_EN=0 (inherit_mon).

Yes. You are right.   I will fix it next revision.
> ---
>
> Separately, I have a question about the group type constraint for
> global_assign_ctrl_assign_mon_per_cpu (assign_mon mode).
>
> In patch 08/12 ("fs/resctrl: Make info/kernel_mode writable and
> identify the bound group"), resctrl_kernel_mode_write() enforces that
> assign_mon must bind to an RDTMON_GROUP:
>
>    if (kmode == BIT(GLOBAL_ASSIGN_CTRL_ASSIGN_MON_PER_CPU) &&
>        rdtgrp->type != RDTMON_GROUP) {
>
> This forces the user to create a ctrl group plus a monitor sub-group.
> But since the kernel-mode group is dedicated to kernel work and no
> user tasks are assigned to it, the ctrl group's own RMID (allocated
> via alloc_rmid() at mkdir time) should be clean and usable.
> Requiring an extra monitor group seems to waste a scarce RMID.
>
> Would it make sense to relax this constraint so that assign_mon also
> accepts an RDTCTRL_GROUP?  The path infrastructure
> (rdtgroup_by_kmode_path and resctrl_kernel_mode_show) already handles
> the "<ctrl>//" form, so only the check above would need to change:
>
>    mkdir /sys/fs/resctrl/kernel_ctrl
>    echo "global_assign_ctrl_assign_mon_per_cpu:group=kernel_ctrl//" \
>         > info/kernel_mode

Yes. That is reasonable.

assign_mon  can be  RDTMON_GROUP or RDTCTRL_GROUP.

inherit_mon can only be RDTCTRL_GROUP.

Thanks

Babu