[v6,02/24] x86/resctrl: kfree() rmid_ptrs from rdtgroup_exit()

  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_exit_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>
---
Changes since v5:
 * This patch is new
---
 arch/x86/kernel/cpu/resctrl/internal.h |  1 +
 arch/x86/kernel/cpu/resctrl/monitor.c  | 10 ++++++++++
 arch/x86/kernel/cpu/resctrl/rdtgroup.c |  5 +++++
 3 files changed, 16 insertions(+)
  

Hi James,

On 9/14/2023 10:21 AM, James Morse wrote:

...

> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 725344048f85..a2158c266e41 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -3867,6 +3867,11 @@ int __init rdtgroup_init(void)
>  
>  void __exit rdtgroup_exit(void)
>  {
> +	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> +
> +	if (r->mon_capable)
> +		resctrl_exit_mon_l3_config(r);
> +
>  	debugfs_remove_recursive(debugfs_resctrl);
>  	unregister_filesystem(&rdt_fs_type);
>  	sysfs_remove_mount_point(fs_kobj, "resctrl");

You did not respond to me when I requested that this be done differently [1].
Without a response letting me know the faults of my proposal or following the
recommendation I conclude that my feedback was ignored. 

Reinette 

[1] https://lore.kernel.org/lkml/1ccd6be5-1dbd-c4a5-659f-ae20761dcce7@intel.com/
  

Hi James,

On 9/14/23 12:21, 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_exit_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>
> ---
> Changes since v5:
>  * This patch is new
> ---
>  arch/x86/kernel/cpu/resctrl/internal.h |  1 +
>  arch/x86/kernel/cpu/resctrl/monitor.c  | 10 ++++++++++
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |  5 +++++
>  3 files changed, 16 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 85ceaf9a31ac..57cf1e6a57bd 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -537,6 +537,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 resctrl_exit_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 ded1fc7cb7cb..cfb3f632a4b2 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;
>  }
>  
> +void resctrl_exit_mon_l3_config(struct rdt_resource *r)
> +{
> +	mutex_lock(&rdtgroup_mutex);
> +
> +	kfree(rmid_ptrs);
> +	rmid_ptrs = NULL;
> +
> +	mutex_unlock(&rdtgroup_mutex);
> +}

What is the need for passing "rdt_resource *r" here?
Is mutex_lock required?

Thanks
Babu
  

Hi Reinette,

On 02/10/2023 18:00, Reinette Chatre wrote:
> On 9/14/2023 10:21 AM, James Morse wrote:
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 725344048f85..a2158c266e41 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -3867,6 +3867,11 @@ int __init rdtgroup_init(void)
>>  
>>  void __exit rdtgroup_exit(void)
>>  {
>> +	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>> +
>> +	if (r->mon_capable)
>> +		resctrl_exit_mon_l3_config(r);
>> +
>>  	debugfs_remove_recursive(debugfs_resctrl);
>>  	unregister_filesystem(&rdt_fs_type);
>>  	sysfs_remove_mount_point(fs_kobj, "resctrl");
> 
> You did not respond to me when I requested that this be done differently [1].
> Without a response letting me know the faults of my proposal or following the
> recommendation I conclude that my feedback was ignored. 

Not so - I just trimmed the bits that didn't need a response. I can respond 'Yes' to each
one if you prefer, but I find that adds more noise than signal.

This is my attempt at 'doing the cleanup properly', which is what you said your preference
was. (no machine on the planet can ever run this code, the __exit section is always
discarded by the linker).

Reading through again, I missed that you wanted this called from resctrl_exit(). (The
naming suggests I did this originally, but it didn't work out).
I don't think this works as the code in resctrl_exit() remains part of the arch code after
the move, but allocating rmid_ptrs[] stays part of the fs code.

resctrl_exit() in core.c gets renamed as resctrl_arch_exit(), and rdtgroup_exit() takes on
the name resctrl_exit() as its part of the exposed interface.


Thanks,

James
  

Hi Babu,

On 04/10/2023 19:00, Moger, Babu wrote:
> On 9/14/23 12:21, 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_exit_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.

>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index 85ceaf9a31ac..57cf1e6a57bd 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -537,6 +537,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 resctrl_exit_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 ded1fc7cb7cb..cfb3f632a4b2 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;
>>  }
>>  
>> +void resctrl_exit_mon_l3_config(struct rdt_resource *r)
>> +{
>> +	mutex_lock(&rdtgroup_mutex);
>> +
>> +	kfree(rmid_ptrs);
>> +	rmid_ptrs = NULL;
>> +
>> +	mutex_unlock(&rdtgroup_mutex);
>> +}

> What is the need for passing "rdt_resource *r" here?

My vain belief that monitors should be supported on something other than L3, but I agree
that isn't what resctrl does today. I'll remove it.


> Is mutex_lock required?

Reads and writes to rmid_ptrs[] are protected by that lock. This ensures no-one reads the
value while its being free()d, and after this function releases the lock, anyone trying
sees NULL.

(This is all moot as its only caller is marked __exit, so gets discarded by the linker)



Thanks,

James
  

Hi James,

On 10/5/2023 10:05 AM, James Morse wrote:
> On 02/10/2023 18:00, Reinette Chatre wrote:
>> On 9/14/2023 10:21 AM, James Morse wrote:
>>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> index 725344048f85..a2158c266e41 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> @@ -3867,6 +3867,11 @@ int __init rdtgroup_init(void)
>>>  
>>>  void __exit rdtgroup_exit(void)
>>>  {
>>> +	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>>> +
>>> +	if (r->mon_capable)
>>> +		resctrl_exit_mon_l3_config(r);
>>> +
>>>  	debugfs_remove_recursive(debugfs_resctrl);
>>>  	unregister_filesystem(&rdt_fs_type);
>>>  	sysfs_remove_mount_point(fs_kobj, "resctrl");
>>
>> You did not respond to me when I requested that this be done differently [1].
>> Without a response letting me know the faults of my proposal or following the
>> recommendation I conclude that my feedback was ignored. 
> 
> Not so - I just trimmed the bits that didn't need a response. I can respond 'Yes' to each
> one if you prefer, but I find that adds more noise than signal.

I do not expect a response to every review feedback but no response
is assumed to mean that you agree with the feedback.

> 
> This is my attempt at 'doing the cleanup properly', which is what you said your preference
> was. (no machine on the planet can ever run this code, the __exit section is always
> discarded by the linker).
> 
> Reading through again, I missed that you wanted this called from resctrl_exit(). (The

Right. And not responding to that created expectation that you agreed with the
request.

> naming suggests I did this originally, but it didn't work out).
> I don't think this works as the code in resctrl_exit() remains part of the arch code after
> the move, but allocating rmid_ptrs[] stays part of the fs code.
> 
> resctrl_exit() in core.c gets renamed as resctrl_arch_exit(), and rdtgroup_exit() takes on
> the name resctrl_exit() as its part of the exposed interface.

I expect memory allocation/free to be symmetrical. Doing otherwise
complicates the code. Having this memory freed in rdtgroup_exit() only
seems appropriate if it is allocated from rdtgroup_init().
Neither rmid_ptrs[] nor closid_num_dirty_rmid are allocated in
rdtgroup_init() so freeing it in rdtgroup_exit() is not appropriate.

If you are planning to move resctrl_exit() to be arch code then I expect
resctrl_late_init() to be split with the rmid_ptrs[]/closid_num_dirty_rmid
allocation moving to fs code. Freeing that memory can follow at that time.

Reinette
  

Hi Reinette,

On 05/10/2023 19:04, Reinette Chatre wrote:
> On 10/5/2023 10:05 AM, James Morse wrote:
>> On 02/10/2023 18:00, Reinette Chatre wrote:
>>> On 9/14/2023 10:21 AM, James Morse wrote:
>>>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> index 725344048f85..a2158c266e41 100644
>>>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> @@ -3867,6 +3867,11 @@ int __init rdtgroup_init(void)
>>>>  
>>>>  void __exit rdtgroup_exit(void)
>>>>  {
>>>> +	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>>>> +
>>>> +	if (r->mon_capable)
>>>> +		resctrl_exit_mon_l3_config(r);
>>>> +
>>>>  	debugfs_remove_recursive(debugfs_resctrl);
>>>>  	unregister_filesystem(&rdt_fs_type);
>>>>  	sysfs_remove_mount_point(fs_kobj, "resctrl");
>>>
>>> You did not respond to me when I requested that this be done differently [1].
>>> Without a response letting me know the faults of my proposal or following the
>>> recommendation I conclude that my feedback was ignored. 
>>
>> Not so - I just trimmed the bits that didn't need a response. I can respond 'Yes' to each
>> one if you prefer, but I find that adds more noise than signal.
> 
> I do not expect a response to every review feedback but no response
> is assumed to mean that you agree with the feedback.
> 
>>
>> This is my attempt at 'doing the cleanup properly', which is what you said your preference
>> was. (no machine on the planet can ever run this code, the __exit section is always
>> discarded by the linker).
>>
>> Reading through again, I missed that you wanted this called from resctrl_exit(). (The
> 
> Right. And not responding to that created expectation that you agreed with the
> request.
> 
>> naming suggests I did this originally, but it didn't work out).
>> I don't think this works as the code in resctrl_exit() remains part of the arch code after
>> the move, but allocating rmid_ptrs[] stays part of the fs code.
>>
>> resctrl_exit() in core.c gets renamed as resctrl_arch_exit(), and rdtgroup_exit() takes on
>> the name resctrl_exit() as its part of the exposed interface.
> 
> I expect memory allocation/free to be symmetrical. Doing otherwise
> complicates the code. Having this memory freed in rdtgroup_exit() only
> seems appropriate if it is allocated from rdtgroup_init().
> Neither rmid_ptrs[] nor closid_num_dirty_rmid are allocated in
> rdtgroup_init() so freeing it in rdtgroup_exit() is not appropriate.

It probably makes more sense when you see how things get split up. I was trying to reduce
the churn of adding something in one place, then moving it later.

For now I've added all the functions to make this thing symmetric.



James


> If you are planning to move resctrl_exit() to be arch code then I expect
> resctrl_late_init() to be split with the rmid_ptrs[]/closid_num_dirty_rmid
> allocation moving to fs code. Freeing that memory can follow at that time.