Re: [PATCH v9 00/24] x86/resctrl: monitored closid+rmid together, separate arch/fs locking

[Reinette Chatre]

On 2/20/2024 12:59 PM, Tony Luck wrote:
> On Mon, Feb 19, 2024 at 05:49:29PM +0100, Thomas Gleixner wrote:
>> On Sat, Feb 17 2024 at 11:55, Borislav Petkov wrote:
>>
>>> On Tue, Feb 13, 2024 at 06:44:14PM +0000, James Morse wrote:
>>>> Hello!
>>>>
>>>> It's been back and forth for whether this series should be rebased onto Tony's
>>>> SNC series. This version isn't, its based on tip/x86/cache.
>>>> (I have the rebased-and-tested versions if anyone needs them)
>>>
>>> The set applied ontop of tip:x86/cache gives:
>>>
>>> vmlinux.o: in function `get_domain_from_cpu':
>>> (.text+0x150f33): undefined reference to `lockdep_is_cpus_held'
>>> ld: vmlinux.o: in function `rdt_ctrl_update':
>>> (.text+0x150fbc): undefined reference to `lockdep_is_cpus_held'
>>
>> Wants to be folded into patch 24.
>>
>> Thanks,
>>
>>         tglx
>> ---
>> --- a/arch/x86/kernel/cpu/resctrl/core.c
>> +++ b/arch/x86/kernel/cpu/resctrl/core.c
>> @@ -368,8 +368,8 @@ struct rdt_domain *get_domain_from_cpu(i
>>  	 * about locks this thread holds will lead to false positives. Check
>>  	 * someone is holding the CPUs lock.
>>  	 */
>> -	if (IS_ENABLED(CONFIG_LOCKDEP))
>> -		lockdep_is_cpus_held();
>> +	if (IS_ENABLED(CONFIG_HOTPLUG_CPU) && IS_ENABLED(CONFIG_LOCKDEP))
>> +		WARN_ON_ONCE(!lockdep_is_cpus_held());
>>  
>>  	list_for_each_entry(d, &r->domains, list) {
>>  		/* Find the domain that contains this CPU */
> 
> Testing tip x86/cache that WARN fires while running
> tools/tests/selftests/resctrl/resctrl_test.
> 
> Everthing runs OK if I drop the top commit:
>   fb700810d30b ("x86/resctrl: Separate arch and fs resctrl locks")

The new WARN_ON_ONCE() is why this encountered. The comment notes that
lockdep_is_cpus_held() is used to determine if "someone is holding the
CPUs lock" but it seems that lockdep_is_cpus_held() still only checks
if "current" is holding cpu_hotplug_lock and that is not possible
when running the code via IPI.

The trace that Tony shared notes that this is triggered by get_domain_from_cpu()
called via rdt_ctrl_update(). rdt_ctrl_update() is only run via IPI:

	resctrl_arch_update_domains() {
		...
		lockdep_assert_cpus_held();
		...
		on_each_cpu_mask(cpu_mask, rdt_ctrl_update, &msr_param, 1);
		...
	}

and

	reset_all_ctrls() {
		...
		lockdep_assert_cpus_held();
		...
		on_each_cpu_mask(cpu_mask, rdt_ctrl_update, &msr_param, 1);
	}

I sprinkled some debug_show_held_locks(current) to confirm and encountered
the following when reproducing the trace using the resctrl tests:

[  202.914334] resctrl_arch_update_domains:355
[  202.919971] 4 locks held by resctrl_tests/3330:
[  202.925169]  #0: ff11001086e09408 (sb_writers#15){.+.+}-{0:0}, at: ksys_write+0x69/0x100
[  202.934375]  #1: ff110010bb653688 (&of->mutex){+.+.}-{3:3}, at: kernfs_fop_write_iter+0xf7/0x200
[  202.944348]  #2: ffffffff8346c890 (cpu_hotplug_lock){++++}-{0:0}, at: rdtgroup_kn_lock_live+0x4c/0xa0
[  202.954774]  #3: ffffffff8344ae68 (rdtgroup_mutex){+.+.}-{3:3}, at: rdtgroup_kn_lock_live+0x5a/0xa0
[  202.965030] get_domain_from_cpu:366
[  202.969087] no locks held by swapper/0/0.
[  202.973697] ------------[ cut here ]------------
[  202.978979] WARNING: CPU: 0 PID: 0 at arch/x86/kernel/cpu/resctrl/core.c:375 get_domain_from_cpu+0x6f/0x80
<SNIP>
[  203.200095] Call Trace:
[  203.202947]  <TASK>
[  203.205406]  ? __warn+0x84/0x180
[  203.209123]  ? get_domain_from_cpu+0x6f/0x80
[  203.214011]  ? report_bug+0x1c7/0x1e0
[  203.218214]  ? handle_bug+0x3c/0x80
[  203.222230]  ? exc_invalid_op+0x18/0x80
[  203.227198]  ? asm_exc_invalid_op+0x1a/0x20
[  203.232529]  ? __pfx_rdt_ctrl_update+0x20/0x20
[  203.238146]  ? get_domain_from_cpu+0x6f/0x80
[  203.243548]  rdt_ctrl_update+0x26/0x80
<SNIP>


So even though it is confirmed via lockdep_assert_cpus_held() that
resctrl_arch_update_domains() holds cpu_hotplug_lock, it does not seem possible
to have a similar lockdep check in the function called by it (resctrl_arch_update_domains())
via IPI. It thus does not look like that lockdep checking within
get_domain_from_cpu() can be accurate and I cannot see what it can be replaced with
to make it accurate. Any guidance will be appreciated. Perhaps we should just drop (but
with detailed context comments remaining) the lockdep check in get_domain_from_cpu()? 

Reinette