Re: [PATCH v7 06/14] fs/resctrl: Add interface to disable a monitor event

["Luck, Tony" <tony.luck@intel.com>]

On Tue, Jun 09, 2026 at 04:02:55PM -0700, Reinette Chatre wrote:
> Hi Tony,
> 
> On 6/9/26 10:21 AM, Luck, Tony wrote:
> > On Mon, Jun 08, 2026 at 04:18:23PM -0700, Reinette Chatre wrote:
> >> Hi Tony,
> >>
> >> On 6/1/26 12:56 PM, Tony Luck wrote:
> >>> In preparation for re-running AET enumeration on every mount, AET code must be
> >>> able to disable events on unmount so the next mount starts from a clean slate.
> >>>
> >>> Add a file system interface for architecture to clear the enabled flag for
> >>> a given event.
> >>
> >> This just verbatim describes what the patch does. It would be helpful to describe
> >> how the flag is used by resctrl to support the first paragraph's implicit claim
> >> that the enabled flag's value is not relevant when resctrl is unmounted.
> > 
> > Revised commit:
> > ---
> > Subject: fs/resctrl: Add interface to disable a monitor event
> > 
> > In preparation for re-running AET enumeration on every mount, AET code must be
> > able to disable events on unmount so the next mount starts from a clean slate.
> > 
> > Add a file system interface for architecture to reset architecture
> > controlled fields of the given event. mon_event::enabled is only used
> > during mount and at run time to check which events to include in file
> > system objects. It is not used during unmount, so it is safe to clear
> > it as part of the unmount flow.
> 
> This introduces a general resctrl fs interface and makes some powerful
> generalized statements in support of the interface but these statements
> are only true for the AET events. Surely mon_event::enabled is used
> during unmount since domains can come and go while resctrl is not mounted
> and as the new comments explain there is significant state coordination that
> needs to be done between these event callbacks and hotplug handlers.
> 
> Similarly, the resctrl LLC occupancy worker keeps running while resctrl
> is unmounted and depends on LLC occupancy event being enabled.
> 
> Creating a resctrl fs generalized interface but motivating it with a
> highly customized lens of usage without making that clear in the changelog 
> but instead just making grand claims of how safe this is seems underhanded.

I can rewrite this commit comment to call out the limitations on event
removal. Those are listed in the new kerneldoc comments that I added to
the resctrl_disable_mon_event() declaration in <linux/resctrl.h> based on
your feedback on previous version of this patch.

Is that what you are looking for here? Or are you suggesting that the
new interface be less general?

> 
> > 
> > Add kerneldoc comments to describe limitations on when events may be enabled
> > or disabled.
> > ---
> > 
> >>
> >>> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
> >>> index 9fd901c78dc6..327e7a863614 100644
> >>> --- a/fs/resctrl/monitor.c
> >>> +++ b/fs/resctrl/monitor.c
> >>> @@ -1012,6 +1012,18 @@ bool resctrl_enable_mon_event(enum resctrl_event_id eventid, bool any_cpu,
> >>>  	return true;
> >>>  }
> >>>  
> >>> +void resctrl_disable_mon_event(enum resctrl_event_id eventid)
> >>> +{
> >>> +	if (WARN_ON_ONCE(eventid < QOS_FIRST_EVENT || eventid >= QOS_NUM_EVENTS))
> >>> +		return;
> >>> +	if (!mon_event_all[eventid].enabled) {
> >>> +		pr_warn("Repeat disable for event %d\n", eventid);
> >>> +		return;
> >>> +	}
> >>> +
> >>> +	mon_event_all[eventid].enabled = false;
> >>> +}
> >>
> >> It seems reasonable for architecture to expect that "disable" of event undoes all
> >> settings from the earlier "enable" of event. The new function only sets the "enabled"
> >> flag to false though. This is potentially confusing since it leaves the event with
> >> some lingering state, some of which is a pointer to state that an architecture may
> >> be reasonable to remove after disabling this event leaving a dangling pointer. 
> > 
> > Ok. I'll add:
> > 
> > +	mon_event_all[eventid].any_cpu = false;
> > +	mon_event_all[eventid].binary_bits = 0;
> > +	mon_event_all[eventid].arch_priv = NULL;
> 
> Thank you.
> 
> Reinette
> 

-Tony