Re: [PATCH v4 3/7] fs/resctrl: Add interface to disable a monitor event

[Luck, Tony]

Hi Reinette,

On Fri, Apr 03, 2026 at 05:03:28PM -0700, Reinette Chatre wrote:
> Hi Tony,
> 
> On 3/30/26 2:43 PM, Tony Luck wrote:
> > Architecture code can ask file system code to enable events. But there
> > is no way to clean up and disable events.
> 
> Missing why it is required to disable events.

Will add rationale.

> > 
> > Add resctrl_disable_mon_event().
> 
> (can be seen from patch)

Will drop this part of comment.

> > Adding/removing events is only
> > possible when the file system is not mounted. Add a WARN_ON() to
> 
> While this is accurate that it should not be possible to enable/disable
> events while resctrl is mounted, this is *not* the *only* time when it
> should not be possible. Here it is unfortunately not straight forward
> since only some events require per-domain state which requires the event
> to be enabled before any domain comes online, potentially very early in
> initialization.
> 
> To me the addition of this warning adds false security.
> 
> Also, consider that resctrl_mounted is protected by rdtgroup_mutex and
> this addition gives architecture code free access without any protection.

This is a problem.

> To do this right resctrl may need to add more state to an event but how
> that may look is unclear since an architecture may require per-domain
> architectural state for an event while resctrl fs needs none.

The extra state for an event could be a pair of pointers to file system functions
to be used by resctrl_{en,disable}_mon_event() to allocate/clean up any state needed
by file system code for each event.

But this might lead to a rabbit hole of adding complexity. May someday
be useful if we ever make resctrl a loadable module.

>                                                               At this
> time these scenarios may just fall into the "architecture must do the
> right thing" category since it has best information on how state is
> managed for the events as they are enabled/disabled.

Are you suggesting to just drop the check for resctrl_mounted (as both
a locking issue, and an incomplete solution)?

> Reinette

-Tony