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

[Reinette Chatre]

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.

> 
> 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