Messages in this thread | | | Date | Thu, 24 Aug 2023 15:58:11 -0700 | Subject | Re: [PATCH v5 06/24] x86/resctrl: Track the number of dirty RMID a CLOSID has | From | Reinette Chatre <> |
| |
Hi James,
On 8/24/2023 9:53 AM, James Morse wrote: > Hi Reinette, > > On 09/08/2023 23:33, Reinette Chatre wrote: >> On 7/28/2023 9:42 AM, James Morse wrote: >>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c >>> index de91ca781d9f..44addc0126fc 100644 >>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c >>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c >>> @@ -43,6 +43,13 @@ struct rmid_entry { >>> */ >>> static LIST_HEAD(rmid_free_lru); >>> >>> +/** >>> + * @closid_num_dirty_rmid The number of dirty RMID each CLOSID has. >>> + * Only allocated when CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID is defined. >>> + * Indexed by CLOSID. Protected by rdtgroup_mutex. >>> + */ >>> +static int *closid_num_dirty_rmid; >>> + >> >> Will the values ever be negative? > > Nope, int is just fewer keystrokes. I'll change it to unsigned int. > > >>> /** >>> * @rmid_limbo_count count of currently unused but (potentially) >>> * dirty RMIDs. > > >>> @@ -782,13 +802,28 @@ void mbm_setup_overflow_handler(struct rdt_domain *dom, unsigned long delay_ms) >>> static int dom_data_init(struct rdt_resource *r) >>> { >>> u32 idx_limit = resctrl_arch_system_num_rmid_idx(); >>> + u32 num_closid = resctrl_arch_get_num_closid(r); >>> struct rmid_entry *entry = NULL; >>> u32 idx; >>> int i; >>> >>> + if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) { >>> + int *tmp; >>> + >>> + tmp = kcalloc(num_closid, sizeof(int), GFP_KERNEL); >>> + if (!tmp) >>> + return -ENOMEM; >>> + >>> + mutex_lock(&rdtgroup_mutex); >>> + closid_num_dirty_rmid = tmp; >>> + mutex_unlock(&rdtgroup_mutex); >>> + } >>> + >> >> It does no harm but I cannot see why the mutex is needed here. > > It's belt-and-braces to ensure that all accesses to that global variable are protected by > that lock. This avoids giving me a memory ordering headache. > rmid_ptrs and the call to __rmid_entry() that dereferences it should probably get the same > treatment.
This is fair.
> I'll move the locking to the caller as the least-churny way of covering both.
This is not clear to me. From what I can tell all the sites you mention are in dom_data_init() so keeping the locking there (but covering the additional sites) seem appropriate?
> >>> rmid_ptrs = kcalloc(idx_limit, sizeof(struct rmid_entry), GFP_KERNEL); >>> - if (!rmid_ptrs) >>> + if (!rmid_ptrs) { >>> + kfree(closid_num_dirty_rmid); >>> return -ENOMEM; >>> + } >>> >>> for (i = 0; i < idx_limit; i++) { >>> entry = &rmid_ptrs[i]; >> >> How will this new memory be freed? Actually I cannot find where >> rmid_ptrs is freed either .... is a "dom_data_free()" needed? > > Oh that's not deliberate? :P > > rmid_ptrs has been immortal since the beginning. The good news is resctrl_exit() goes in > the exitcall section, which is in the DISCARDS section of the linker script as resctrl > can't be built as a module. It isn't possible to tear resctrl down, so no-one will notice > this leak. > > Something on my eternal-todo-list is to make the filesystem parts of resctrl a loadable > module (if Tony doesn't get there first!). That would flush this sort of thing out. > Last time I triggered resctrl_exit() manually not all of the files got cleaned up - I > haven't investigated it further. > > > I agree it should probably have a kfree() call somewhere under rdtgroup_exit(), as its > only the L3 that needs any of this, I'll add resctrl_exit_mon_l3_config() for > rdtgroup_exit() to call.
I'd prefer that allocation and free are clearly symmetrical. Doing so helps to make the code easier to understand. rdtgroup_exit() is intended to clean up after rdtgroup_init(). Since this allocation does not occur within rdtgroup_init() I do not think rdtgroup_exit() is the best place for this cleanup. resctrl_exit() looks more appropriate to me. Having a dom_data_free() to clean up after a dom_data_init() also seems like an addition that will help to make the code easier to understand but that is without a clear understanding about what you have in mind for resctrl_exit_mon_l3_config().
> > Another option is to rip out all the __exit text as its discarded anyway. But if loadable > modules is the direction of travel, it probably make more sense to fix it.
My preference is to do the cleanup properly.
Reinette
| |