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

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

Hi Tony,

On 10/8/26 4:43 PM, Tony Luck wrote:
> resctrl can be mounted with monitoring support only, with no allocation
> support. In that configuration every task that has not been explicitly
> moved to a MON group remains in the default group, and CTRL_MON group
> membership is decided purely by comparing a task's CLOSID against the
> group's closid.
> 
> is_closid_match() also requires resctrl_arch_alloc_capable() to be true.
> On a monitor-only system that is never the case, so the check breaks the
> default group: it unconditionally returns false for all tasks, and
> is_rmid_match() also returns false because the default group has type
> RDTCTRL_GROUP rather than RDTMON_GROUP. Reading the root tasks file then
> shows no tasks at all, even though every unmoved task belongs there.
> 
> Drop the resctrl_arch_alloc_capable() test from is_closid_match(). A
> CTRL_MON group other than the default group can only be created when
> allocation is supported, so for those groups the test is redundant. But
> the default group always has closid == RESCTRL_RESERVED_CLOSID and is
> present even without allocation support, so the test is wrong for it:
> it is exactly the case this patch fixes.
> 
> 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>
> Assisted-by: LLM

  [R18][R17] must-fix  Mount described as selecting the capability set
      "resctrl can be mounted with monitoring support only, with no
      allocation support." There is no mount option that does this;
      rdt_fs_parameters[] carries cdp, cdpl2, mba_MBps and debug only.
      The capability set is a property of the system, established
      during CPU detection. Use: "resctrl can be mounted on a system
      that supports monitoring but not allocation."

  [R14][R23] nit  Same term, two spellings in one clause
      "comparing a task's CLOSID against the group's closid" switches
      case mid-sentence. Pick one spelling.

  [R8][R23] should-fix  Problem statement transcribes the condition
      "is_closid_match() also requires resctrl_arch_alloc_capable() to
      be true" transcribes the condition the diff removes, in the code's
      own vocabulary down to "to be true". It conveys nothing beyond the
      hunk.
      "is_closid_match() additionally requires allocation support"
      states the same fact in the changelog's register and chains off
      the context sentence about CLOSID comparison. Let the solution
      paragraph be where resctrl_arch_alloc_capable() is first named —
      the problem is then stated semantically and the fix names the
      identifier it removes.

  [R23][RC5] nit  Vocabulary changes register between paragraphs
      Paragraph one uses the resctrl.rst terms "MON group" and
      "CTRL_MON group"; paragraph two switches to "type RDTCTRL_GROUP
      rather than RDTMON_GROUP". "because the default group is a
      CTRL_MON group, not a MON group" keeps one vocabulary and stays
      readable before the diff is opened.

  [R23] should-fix  "root tasks file" is not resctrl's vocabulary
      resctrl.rst calls the group the "default group" / "default
      resource group" and reserves "root" for the directory. The
      changelog already says "default group" in the context paragraph
      and again in the solution paragraph, so "root" in the problem
      paragraph is also inconsistent within the same text. Use "the
      default group's tasks file".

  [R12] nit  Self-reference in the closing clause
      "it is exactly the case this patch fixes" — maintainer-tip.rst asks
      changelogs to avoid "this patch". The sentence already lands
      without it; "and that is the breakage described above" works, or
      just stop at "so the test is wrong for it".

[R8][R23] should-fix  Reserved-closid detail is not part of the argument
      "the default group always has closid == RESCTRL_RESERVED_CLOSID
      and is present even without allocation support, so the test is
      wrong for it" — only the second clause supports the conclusion.

  [R13] nit  Antecedent of "it"
      "so the check breaks the default group: it unconditionally returns
      false for all tasks" — the nearest preceding noun is "the default
      group", not the check. Naming is_closid_match() again removes the
      wobble.

> ---
> 
> I fed your AI review to Claude and asked it to rewrite the changelog to
> address all the issues your AI raised. Here's the summary Claude

I suggest that you feed it Documentation/process/maintainer-tip.rst

...
> 
> Claude put the "Assisted-by:" tag after my sign-off. The tip maintainer
> documentation hasn't been updated to provide explicit guidance on where
> this should appear. Looking at upstream commits people have picked
> different spots, but immediately after the author sign-off seems common.

Previous submissions to resctrl that used AI were merged with the tag before the
Signed-off-by. For reference,
2d77f9768850 ("fs/resctrl: Prevent deadlock and use-after-free in info file handlers")
f5bcf539484d ("fs/resctrl: Prevent use-after-free in rdtgroup_kn_put()")

As I understand it has become more important to also note what AI was used for. Using
AI to write the changelog for you could be perceived different from using AI to debug
the issue and writing the patch for you. Reference on this topic is:
https://docs.kernel.org/process/generated-content.html

Reinette