Hi James,
On 12/15/2023 9:43 AM, James Morse wrote:
> rmid_ptrs[] is allocated from dom_data_init() but never free()d.
>
> While the exit text ends up in the linker script's DISCARD section,
> the direction of travel is for resctrl to be/have loadable modules.
>
> Add resctrl_put_mon_l3_config() to cleanup any memory allocated
> by rdt_get_mon_l3_config().
>
> There is no reason to backport this to a stable kernel.
>
> Signed-off-by: James Morse <james.morse@arm.com>
> Tested-by: Babu Moger <babu.moger@amd.com>
> Reviewed-by: Babu Moger <babu.moger@amd.com>
> ---
> Changes since v5:
> * This patch is new
>
> Changes since v6:
> * Removed struct rdt_resource argument, added __exit markers to match the
> only caller.
> * Adedd a whole stack of functions to maintain symmetry.
>
> Changes since v7:
> * Moved the eventual kfree() call to be after rdtgroup_exit()
> ---
> arch/x86/kernel/cpu/resctrl/core.c | 6 ++++++
> arch/x86/kernel/cpu/resctrl/internal.h | 1 +
> arch/x86/kernel/cpu/resctrl/monitor.c | 15 +++++++++++++++
> 3 files changed, 22 insertions(+)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c
> index 19e0681f0435..b5d4b76f97c5 100644
> --- a/arch/x86/kernel/cpu/resctrl/core.c
> +++ b/arch/x86/kernel/cpu/resctrl/core.c
> @@ -992,8 +992,14 @@ late_initcall(resctrl_late_init);
>
> static void __exit resctrl_exit(void)
> {
> + struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> +
> cpuhp_remove_state(rdt_online);
> +
> rdtgroup_exit();
> +
> + if (r->mon_capable)
> + rdt_put_mon_l3_config(r);
> }
>
> __exitcall(resctrl_exit);
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index a4f1aa15f0a2..f68c6aecfa66 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -546,6 +546,7 @@ void closid_free(int closid);
> int alloc_rmid(void);
> void free_rmid(u32 rmid);
> int rdt_get_mon_l3_config(struct rdt_resource *r);
> +void __exit rdt_put_mon_l3_config(struct rdt_resource *r);
> bool __init rdt_cpu_has(int flag);
> void mon_event_count(void *info);
> int rdtgroup_mondata_show(struct seq_file *m, void *arg);
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index f136ac046851..5d9864919f1c 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -741,6 +741,16 @@ static int dom_data_init(struct rdt_resource *r)
> return 0;
> }
>
> +static void __exit dom_data_exit(struct rdt_resource *r)
> +{
> + mutex_lock(&rdtgroup_mutex);
> +
> + kfree(rmid_ptrs);
> + rmid_ptrs = NULL;
> +
> + mutex_unlock(&rdtgroup_mutex);
> +}
> +
> static struct mon_evt llc_occupancy_event = {
> .name = "llc_occupancy",
> .evtid = QOS_L3_OCCUP_EVENT_ID,
> @@ -830,6 +840,11 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
> return 0;
> }
>
> +void __exit rdt_put_mon_l3_config(struct rdt_resource *r)
> +{
> + dom_data_exit(r);
> +}
> +
I expected the unused resource parameter to be removed because you said ([1]) it would.
> void __init intel_rdt_mbm_apply_quirk(void)
> {
> int cf_index;
Reinette
[1] https://lore.kernel.org/lkml/c4157536-7e57-0250-dc44-8914f5965ce6@arm.com/