lkml.org 
[lkml]   [2021]   [Oct]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2 07/23] x86/resctrl: Add domain offline callback for resctrl work
From
Date
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

\
 
 \ /
  Last update: 2021-10-22 20:31    [W:0.037 / U:0.236 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site