Re: [PATCH v3 16/38] x86/resctrl: Move monitor init work to a resctrl init call

[Reinette Chatre]

Hi James,

On 6/14/24 8:00 AM, James Morse wrote:
> rdt_get_mon_l3_config() is called from the architecture's
> resctrl_arch_late_init(), and initialises both architecture specific
> fields, such as hw_res->mon_scale and resctrl filesystem fields
> by calling dom_data_init().
>
> To separate the filesystem and architecture parts of resctrl, this
> function needs splitting up.
>
> Add resctrl_mon_resource_init() to do the filesystem specific work,
> and call it from resctrl_init(). This runs later, but is still before
> the filesystem is mounted and the rmid_ptrs[] array can be used.
>
> Signed-off-by: James Morse <james.morse@xxxxxxx>
> Tested-by: Peter Newman <peternewman@xxxxxxxxxx>
> Tested-by: Shaopeng Tan <tan.shaopeng@xxxxxxxxxxxxxx>
> ---
> Changes since v2:
>   * Added error handling for the case sysfs files can't be created.
> ---
>   arch/x86/kernel/cpu/resctrl/internal.h |  1 +
>   arch/x86/kernel/cpu/resctrl/monitor.c  | 24 +++++++++++++++++-------
>   arch/x86/kernel/cpu/resctrl/rdtgroup.c |  9 ++++++++-
>   3 files changed, 26 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 9aa7f587484c..eaf458967fa1 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -542,6 +542,7 @@ int rdtgroup_mondata_show(struct seq_file *m, void *arg);
>   void mon_event_read(struct rmid_read *rr, struct rdt_resource *r,
>   		    struct rdt_domain *d, struct rdtgroup *rdtgrp,
>   		    int evtid, int first);
> +int resctrl_mon_resource_init(void);
>   void mbm_setup_overflow_handler(struct rdt_domain *dom,
>   				unsigned long delay_ms,
>   				int exclude_cpu);
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index 7d6aebce75c1..527c0e9d7b2e 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -1016,12 +1016,28 @@ static void l3_mon_evt_init(struct rdt_resource *r)
>   		list_add_tail(&mbm_local_event.list, &r->evt_list);
>   }
>   
> +int resctrl_mon_resource_init(void)

(Lack of an __init is unexpected but I assume it was done since that will be removed
in later patch anyway?)

This function needs a big warning to deter anybody from considering this to
be the place where any and all monitor related allocations happen. It needs
to warn developers that only resources that can only be touched after fs mount
may be allocated here.

Reinette