Re: [PATCH v13 01/25] fs/resctrl: Ensure default group reports tasks on monitor-only systems

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

Hi Tony,

On 9/28/26 3:14 PM, Tony Luck wrote:
> This is a pre-existing issue, but the resctrl_arch_alloc_capable() check in
> is_closid_match() actively breaks the default group on systems that only
> support monitoring capabilities.

It looks like my comments regarding tip changelog requirements are still being
missed. It is highly time-consuming to manually verify every claim made within
these changelogs, and then separately draft feedback on where they fail to meet
the documented tip requirements.

You inspired me to automate these steps. I created a "changelog checker" that
automatically verifies the claims and checks the changelog against the documented
tip requirements. These are the standard, straightforward expectations for our
submissions.

The tool will handle reviews of your changelogs instead of me so I don't lose any
more manual review time. You can coordinate directly with the bot's feedback now!

Here is what it has to say about this changelog:

Verified claims

  - rdtgroup_tasks_show() → show_rdt_tasks() → is_closid_match() — CONFIRMED (that is the chain in
    fs/resctrl/rdtgroup.c).
  - resctrl_arch_alloc_capable() false on monitor-only systems makes is_closid_match() return false for all tasks —
    CONFIRMED (x86 returns rdt_alloc_capable, set once in resctrl_cpu_detect() and never cleared; rdt_get_tree()
    permits a mount with monitoring alone).
  - The default group has type RDTCTRL_GROUP, so is_rmid_match() also returns false — CONFIRMED
    (rdtgroup_setup_default() sets type = RDTCTRL_GROUP; is_rmid_match() requires RDTMON_GROUP).
  - The root tasks file appears empty — CONFIRMED. The file always exists on a monitor-only mount; it carries
    RFTYPE_BASE, which rdt_get_tree() passes unconditionally.
  - A RDTCTRL_GROUP other than the default can only be created when allocation is supported — CONFIRMED
    (rdtgroup_mkdir() gates rdtgroup_mkdir_ctrl_mon() on resctrl_arch_alloc_capable()).
  - "whenever the remaining resctrl_arch_match_closid() check could meaningfully succeed,
    resctrl_arch_alloc_capable() was already implicitly true, making the test redundant" — FALSE.
    rdtgroup_setup_default() sets closid = RESCTRL_RESERVED_CLOSID, and every task that has not been moved keeps
    that CLOSID, so resctrl_arch_match_closid() succeeds for the default group while resctrl_arch_alloc_capable()
    is false. That is the case the patch fixes.
  - Fixes: e6b2fac36fcc — CONFIRMED. That commit introduced the capability test into the tasks-file path, and its
    own changelog asserted "This is harmless as rdtgroup_mkdir() tests these capable flags", which overlooked the
    default group.

Findings

  [R18][R9] must-fix  The safety argument contradicts the bug being fixed
      The last paragraph concludes the removed test was redundant. If it
      were redundant the patch would be a no-op. The default group is the
      one group whose result changes, and the parenthetical that excludes
      it from the first sentence is not carried into the second. Scope the
      claim: redundant for every other CTRL_MON group, wrong for the
      default group.

  [R5][R6][R24] should-fix  No context paragraph; the problem leads
      The body opens by naming the offending check. A reader has not yet
      been told that resctrl can be mounted with monitoring only, that the
      default group owns every unassigned task, or that CTRL_MON
      membership is decided by CLOSID. Without that, the problem paragraph
      has nothing to attach to.

  [R12] should-fix  No imperative solution sentence
      "Removing the resctrl_arch_alloc_capable() test is safe because ..."
      is a gerund, and the patch never says "Drop ...". The tip tree asks
      for the fix in the imperative.

  [R8] should-fix  Three paragraphs re-derive a one-term removal
      The indented call chain, then "is_closid_match() unconditionally
      returns false for all tasks", then the empty-file effect all restate
      the same single condition. The user-visible symptom plus the reason
      both match tests fail is enough.

  [R20] nit  "This is a pre-existing issue, but ..."
      This addresses the series reviewer, not a future reader of the
      commit. The Fixes: tag already conveys that the bug predates the
      series; such notes belong below the --- line.

  [R13] nit  "actively breaks" — "breaks" carries the same meaning.

  [R23][RC5] nit  Prose uses `type == RDTCTRL_GROUP`
      Documentation/filesystems/resctrl.rst calls these "CTRL_MON" groups
      and the root-owned ones "MON" groups. Those are the terms of art a
      reviewer can map to the interface without opening the diff.
  Tag ordering (Fixes → Reported-by → Closes → Signed-off-by) matches R21. Wrapping is 70–76 columns, within RC4 —
  checkpatch's single "Prefer a maximum 75 chars" warning is not reportable for resctrl.


> 
> When a user reads the root /sys/fs/resctrl/tasks file on a system with
> monitoring capabilities but no allocation capabilities, the following call
> chain occurs:
> 	rdtgroup_tasks_show()
> 	show_rdt_tasks()
> 	is_closid_match()
> 
> Since resctrl_arch_alloc_capable() evaluates to false on such systems,
> is_closid_match() unconditionally returns false for all tasks. Furthermore,
> because the default group has type RDTCTRL_GROUP, is_rmid_match() will also
> return false.
> 
> This causes the root tasks file to appear completely empty, hiding all tasks
> on the system that have not been explicitly moved to a monitoring group.
> 
> Removing the resctrl_arch_alloc_capable() test is safe because a struct
> rdtgroup with type == RDTCTRL_GROUP (other than the always-present default
> group) can only be created on systems where allocation is supported. So
> whenever the remaining resctrl_arch_match_closid() check could meaningfully
> succeed, resctrl_arch_alloc_capable() was already implicitly true, making
> the test redundant.
> 
> Fixes: e6b2fac36fcc ("x86/resctrl: Use is_closid_match() in more places")
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://sashiko.dev/#/patchset/20260831174421.13921-1-tony.luck%40intel.com?part=9
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
Reinette