Re: [PATCH v3 13/26] fs/resctrl: Add an architectural hook called for each mount

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

Hi Tony,

On 4/7/25 4:40 PM, Tony Luck wrote:
> Enumeration of Intel telemetry events is not complete when the
> resctrl "late_init" code is executed.
> 
> Add a hook at the beginning of the mount code that will be used
> to check for telemetry events and initialize if any are found.
> 
> The hook is called on every mount. But expectations are that
> most actions (like enumeration) will only need to be performed
> on the first call.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
>  include/linux/resctrl.h            | 3 +++
>  arch/x86/kernel/cpu/resctrl/core.c | 9 +++++++++
>  fs/resctrl/rdtgroup.c              | 2 ++
>  3 files changed, 14 insertions(+)
> 
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 8ac77b738de5..25f51a57b0b7 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -421,6 +421,9 @@ void resctrl_offline_cpu(unsigned int cpu);
>  int resctrl_set_event_attributes(enum resctrl_event_id evt,
>  				 enum resctrl_event_type type, bool any_cpu);
>  
> +/* Architecture hook called for each file system mount */

Please add some description of what architecture could use it for as well
as more specific detail of when it is called during mount. I think it is
important to highlight and make it part of agreement that resctrl fs calls
this on mount before any resctrl fs actions. Considering this, perhaps
resctrl_arch_pre_mount()?

It is also worth highlighting in the API doc that fs does not actually
call resctrl_arch_mount() on every mount but every mount *attempt* (resctrl
may already be mounted) so it is up for arch to maintain any needed state.

(Also see later comment about locking)

> +void resctrl_arch_mount(void);
> +
>  /**
>   * resctrl_arch_rmid_read() - Read the eventid counter corresponding to rmid
>   *			      for this resource and domain.
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 59844fd7105f..a066a9c54a1f 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -711,6 +711,15 @@ static int resctrl_arch_offline_cpu(unsigned int cpu)
>  	return 0;
>  }
>  
> +void resctrl_arch_mount(void)
> +{
> +	static bool only_once;
> +
> +	if (only_once)
> +		return;
> +	only_once = true;
> +}
> +
>  enum {
>  	RDT_FLAG_CMT,
>  	RDT_FLAG_MBM_TOTAL,
> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
> index bd41f7a0f416..5ca6de6a6e5c 100644
> --- a/fs/resctrl/rdtgroup.c
> +++ b/fs/resctrl/rdtgroup.c
> @@ -2564,6 +2564,8 @@ static int rdt_get_tree(struct fs_context *fc)
>  	struct rdt_resource *r;
>  	int ret;
>  
> +	resctrl_arch_mount();
> +
>  	cpus_read_lock();
>  	mutex_lock(&rdtgroup_mutex);
>  	/*

Could you please elaborate on the locking requirements here?  Worth a mention of these
expectations in changelog also. That it is called without any locks held and arch is
responsible for all locking should be documented as part of API in include/linux/resctrl.h


Reinette