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

[Reinette Chatre]

Hi Tony,

On 4/7/26 11:40 AM, Luck, Tony wrote:
> On Mon, Apr 06, 2026 at 02:13:19PM -0700, Reinette Chatre wrote:
>> Hi Tony,
>>
>> On 4/6/26 11:35 AM, Luck, Tony wrote:
>>> On Fri, Apr 03, 2026 at 05:03:28PM -0700, Reinette Chatre wrote:
>>
>> ...
>>
>>>
>>>>                                                               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)?
>>
>> I am indeed suggesting to "drop the check for resctrl_mounted" but instead
>> of "just" doing that I think it worthwhile to add function comments to these
>> two arch helpers in include/linux/resctrl.h that describes what needs to be
>> considered when calling them. That is, describe "architecture must do the
>> right thing" with some documentation about what needs to be considered.
>> Such documentation may help us to start putting some boundaries on how
>> these helpers can/should be used to help guide any future enhancements to
>> make this more robust.
> 
> Something like this:
> 
> /*
>  * For events that require per-domain allocation, this routine must be called

Since all events are per-domain and all domains need allocation this is not clear.
(nit: it is not necessary to say "this routine" when the context is clearly
associated with the function).

I think referring to it as "per-domain state" is more accurate.

>  * before CPU hot plug state begins allocating domain structures.

Architecture has some flexibility here if a resource is discovered after
initialization, like what AET does.

>  * For other events the requirement is that the file system must not be

"For other events" -> "For all events"?

>  * mounted when enabling events.
>  */
> bool resctrl_enable_mon_event(enum resctrl_event_id eventid, bool any_cpu,
>                               unsigned int binary_bits, void *arch_priv)

Since resctrl has AET to thank for all the additional parameters and this is
another AET enhancement I think it would be appreciated if this be done properly.
Consider, for example:

/**
 * resctrl_enable_mon_event() - Enable monitoring event
 * @eventid:	ID of the event
 * @any_cpu:	True if event data can be read from any CPU.
 * @binary_bits:Number of binary places of the fixed-point value expected to
 *		back a floating point event. Can only be set for floating point
 *		events.
 * @arch_priv:	Architecture private data associated with event. Passed back to
 *		architecture when reading the event via resctrl_arch_rmid_read().
 *
 * The file system must not be mounted when enabling an event.
 *
 * Events that require per-domain (architectural and/or filesystem) state must
 * be enabled before the domain structures are allocated. For example before
 * CPU hotplug callbacks that allocate domain structures are registered. If the
 * architecture discovers a resource after initialization it should enable
 * events needing per-domain state before any domain structure allocation which
 * should be coordinated with the CPU hotplug callbacks.
 *
 * Return:
 * true if event was successfully enabled, false otherwise.
 */
bool resctrl_enable_mon_event(enum resctrl_event_id eventid, bool any_cpu,
                              unsigned int binary_bits, void *arch_priv)


> 
> ...
> 
> /*
>  * This routine must not be called for events that require per-domain allcoation.
>  * For other events the requirement is that the file system must not be
>  * mounted when disabling events.
>  */
> void resctrl_disable_mon_event(enum resctrl_event_id eventid)
> 

Similar here. Please note the "allcoation"

Reinette