Messages in this thread | | | Subject | Re: [PATCH v2 07/23] x86/resctrl: Add domain offline callback for resctrl work | From | James Morse <> | Date | Fri, 22 Oct 2021 19:30:16 +0100 |
| |
Hi Babu,
On 20/10/2021 00:19, Babu Moger wrote: > On 10/1/21 11:02 AM, James Morse wrote: >> Because domains are exposed to user-space via resctrl, the filesystem >> must update its state when CPU hotplug callbacks are triggered. >> >> Some of this work is common to any architecture that would support >> resctrl, but the work is tied up with the architecture code to >> free the memory. >> >> Move the monitor subdir removal and the cancelling of the mbm/limbo >> works into a new resctrl_offline_domain() call. These bits are not >> specific to the architecture. Grouping them in one function allows >> that code to be moved to /fs/ and re-used by another architecture.
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> index 19691f9ab061..38670bb810cb 100644 >> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> @@ -2499,14 +2499,12 @@ static int mon_addfile(struct kernfs_node *parent_kn, const char *name, >> * Remove all subdirectories of mon_data of ctrl_mon groups >> * and monitor groups with given domain id. >> */ >> -void rmdir_mondata_subdir_allrdtgrp(struct rdt_resource *r, unsigned int dom_id) >> +static void rmdir_mondata_subdir_allrdtgrp(struct rdt_resource *r, >> + unsigned int dom_id) >> { >> struct rdtgroup *prgrp, *crgrp; >> char name[32];
>> - if (!r->mon_capable) >> - return;
>> list_for_each_entry(prgrp, &rdt_all_groups, rdtgroup_list) { >> sprintf(name, "mon_%s_%02d", r->name, dom_id); >> kernfs_remove_by_name(prgrp->mon.mon_data_kn, name); >> @@ -3233,6 +3231,39 @@ static int __init rdtgroup_setup_root(void) >> return ret; >> } >> >> +void resctrl_offline_domain(struct rdt_resource *r, struct rdt_domain *d) >> +{ >> + lockdep_assert_held(&rdtgroup_mutex); > > Is this really required?
It documents that the caller must take the lock. Its not so clear how walking rdt_all_groups in rmdir_mondata_subdir_allrdtgrp() is safe now that the logic has moved to another file. (and these helpers will eventually move out to /fs/).
Who takes what lock changes later in the tree, (after this series), these annotations make it a lot clearer that these functions are changing from caller-takes-the-lock to callee-takes-the-new-lock. Otherwise that patch would be much harder to review.
>> + >> + if (!r->mon_capable) >> + return; > > I don't see the need for this check either.
It moved up from rmdir_mondata_subdir_allrdtgrp(), quoted above. All of the work that moved from domain_remove_cpu() to resctrl_offline_domain() is about monitors.
Sure, calling is_mbm_enabled(), is_llc_occupancy_enabled(), bitmap_free(NULL), and kfree(NULL) twice isn't harmful in this case, but its quicker to check the flag on the resource and return early if nothing else needs doing.
Thanks,
James
| |