linux-kernel - Re: [PATCH v2 07/23] x86/resctrl: Add domain offline callback for resctrl work

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <a67d75da-3d35-7121-bbb2-87335c3b4950@arm.com>
Date:   Fri, 22 Oct 2021 19:30:16 +0100
From:   James Morse <james.morse@....com>
To:     Babu Moger <babu.moger@....com>, x86@...nel.org,
        linux-kernel@...r.kernel.org
Cc:     Fenghua Yu <fenghua.yu@...el.com>,
        Reinette Chatre <reinette.chatre@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        H Peter Anvin <hpa@...or.com>,
        shameerali.kolothum.thodi@...wei.com,
        Jamie Iles <jamie@...iainc.com>,
        D Scott Phillips OS <scott@...amperecomputing.com>,
        lcherian@...vell.com, bobo.shaobowang@...wei.com,
        tan.shaopeng@...itsu.com
Subject: Re: [PATCH v2 07/23] x86/resctrl: Add domain offline callback for
 resctrl work

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