Re: [PATCH v4 4/7] fs,x86/resctrl: Add architecture hooks for every mount/unmount

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

Hi Tony,

On 4/9/26 1:35 PM, Luck, Tony wrote:
> On Mon, Apr 06, 2026 at 02:16:46PM -0700, Reinette Chatre wrote:
>> Hi Tony,
>>
>> On 4/6/26 1:35 PM, Luck, Tony wrote:
>>> On Fri, Apr 03, 2026 at 05:52:30PM -0700, Reinette Chatre wrote:
>>>> On 3/30/26 2:43 PM, Tony Luck wrote:

>>>>> @@ -2900,6 +2893,30 @@ static int rdt_get_tree(struct fs_context *fc)
>>>>>  	return ret;
>>>>>  }
>>>>>  
>>>>> +static int rdt_get_tree_wrapper(struct fs_context *fc)
>>>>> +{
>>>>> +	int ret;
>>>>> +
>>>>> +	mutex_lock(&resctrl_mount_lock);
>>>>> +
>>>>> +	/*
>>>>> +	 * resctrl file system can only be mounted once.
>>>>> +	 */
>>>>> +	if (resctrl_mounted) {
>>>>> +		mutex_unlock(&resctrl_mount_lock);
>>>>> +		return -EBUSY;
>>>>> +	}
>>>>> +
>>>>
>>>> This does not look right. Here too is resctrl_mounted accessed without rdtgroup_mutex
>>>> held. This change implies that resctrl_mounted is now protected by resctrl_mount_lock
>>>> but resctrl is not changed to respect this throughout resulting in unsafe access of
>>>> resctrl_mounted.
>>>>
>>>> Does this new resctrl_mount_lock need to be in resctrl fs? It really seems as though the
>>>> needed synchronization belongs in the architecture. Could this instead be accomplished
>>>> with a private mutex within the AET code?
>>>
>>> If you dig in lore for the v3 of this patch, you'll see I had the mutex in the
>>> AET code. But there were some complications.
>>>
>>> 1) Need to acquire in intel_aet_pre_mount() and release in intel_aet_mount_result()
>>> which is legal, but makes code more complex when call chains need to be compared to
>>> check that the mutex is being released correctly.
>>
>> Why was it needed to hold mutex for so long? I cannot find explanation here or in changelog
>> of v3. I did not remember correctly and considered the AET code to be doing the domain
>> addition. Even so, I do think a mutex internal to the arch code can be used to manage
>> the synchronization. Could you please elaborate why this cannot be done?
> 
> I tried to move the locks into architecture code. But main problem is still
> handling when a user tries to mount an already mounted resctrl file system
> and gets -EBUSY.
> 
> In that case file system calls resctrl_arch_pre_mount() with the file system
> mounted. You suggested that the AET code could detect and ignore a repeat
> enumeration by noting that the event_group "(*peg)->pfg" is non-NULL, set by

It would be great if resctrl only needs to enumerate once but I do not see how that
is possible since there is no clear indication from PMT driver whether (all) its 
data is ready or not.

> the original enumeration. But that fails in this scenario:
> 
> 	# rmmod pmt_telemetry
> 	# mount -t resctrl resctrl /sys/fs/resctrl
> 		... succeeds, but without AET present
> 	# modprobe pmt_telemetry
> 	# mount -t resctrl resctrl /sys/fs/resctrl
> 		... enumeration success, but now calls resctrl_enable_mon_event()
> 		... with the file system mounted

Thank you for catching this.

> 
> I think the bast solution for this is to change definition of resctrl_arch_pre_mount()
> from "called on every mount attempt" to "called only when resctrl is NOT mounted".
> This is because architecture code cannot tell whether the file system is mounted.

Does this mean the addition of the extra locking in resctrl fs? I am not comfortable with
that asymmetrical locking.

I think a problem here is that resctrl_enable_mon_event() gives the architecture code the
capability of manipulating internal resctrl fs state directly. This is a consequence
of the original design before the arch/fs split. I see the additional resctrl fs locking
as an attempt to make it easier for the arch to manipulate fs state but I do not think we
should go in that direction, instead I think it is better to improve the arch/fs boundaries
here.

To that end, what if resctrl uses its familiar "capable" vs "enable" separation here?
That is, the architecture informs resctrl fs whether it is *capable* of a feature and
resctrl fs, based on interactions with user space, determines whether the feature is
*enabled*?

For a simpler addition resctrl could obtain a new helper, for example,
resctrl_capable_mon_event() that only marks an event as "capable". resctrl_enable_mon_event()
could remain as the "early" call that marks events as capable
and enabled but may be deprecated. Both of these *have* to be called before any domains
are created since per-domain state would now depend on whether the system is "capable"
of an event or not. resctrl fs can assume that all "capable" events needs to be enabled
and it can mark them so at the beginning of each mount, resctrl will only expose
"enabled" events.

There are likely some simplifications on top of current implementation to not make
this too invasive.

What do you think?

>>> 2) The "only mounted once" case meant extra state (AET_PRESENT, which you note
>>> in next patch may be redundant) because intel_aet_pre_mount() is called, but
>>> needs to do nothing.
>>
>> Right, I do not see need for extra state. In fact, since it is not clear to me that
>> PMT enumeration will be complete when intel_pmt_get_regions_by_feature() is called it
>> seemed worthwhile to only rely on event_group::pfg - if PMT enumeration was not complete
>> during mount N it may be complete on mount N+1? This creates a poor user interface
>> though since user would need an alternate way to know if AET is supported and then
>> a "remount until it works" approach.
> 
> The race remains, and is lost if resctrl is auto-mounted at boot from /etc/fstab.
> 
> The user can tell if AET is supported with:
> 
> 	$ grep ^ /sys/class/intel_pmt/*/guid
> 
> and checking if any of the RMID based event guids are present on the system.
> 
> Delta T for the race is small enough that delaying the mount to some other
> startup script should be sufficient. Users are likely to have such a script
> to create the CTRL_MON directories and configure schemata for their workload.
> So annoying, but easily solved.

I do not have a clear understanding of how the new implementation with registration
function will look to understand the races involved.

>>> Adding resctrl_mount_lock to the file system code made things simpler. The
>>
>> Adding complications to resctrl fs to make things simpler for x86?
> 
> I believe it is necessary, since architecture cannot tell if the file system
> is mounted.
> 
>>> pre-mount code can't be called with rdtgroup_mutex held because it needs to
>>> build the domains. That needs cpus_read_lock() + mutex_lock(&domain_list_lock);
>>
>> ack. Can an arch-specific mutex be used instead?
> 
> See above.
> 
>>> I need to add more comments on locking. resctrl_mounted is only modified when both
>>> resctrl_mount_lock AND rdtgroup_mutex are held. I believe that makes it safe to
>>> read the value of resctrl_mounted with just rdtgroup_mutex held.
>>
>> ...but not to read it with only resctrl_mount_lock held as in snippet above.
> 
> Holding either of resctrl_mount_lock or rdtgroup_mutex makes it safe to
> read the value of resctrl_mounted as it can only be modified when both
> mutexes are held.

I am concerned about this approach due to it not being symmetrical and how resctrl fs
now adds additional locking to accommodate drivers.

Reinette